Fix the port to the x86 Architecture #4385

ioanaculic · 2025-03-26T08:48:49Z

Pull Request Overview

This pull request fixes the #4171:

removes the external dependency to the x86 crate
replaces the x86 structures using tock-registers
fixes some lints

Testing Strategy

This pull request was tested by @ioanaculic.

TODO or Help Wanted

Rebase and feeback.

I would like to get some feedback before I rebase this.

Documentation Updated

Updated the relevant files in /docs, or no updates are required.

Formatting

Ran make prepush.

bradjc

This is awesome!!

Note, I haven't reviewed any of the code, but the general directory structure looks good to me.

bradjc · 2025-03-26T15:43:19Z

chips/pc/Cargo.toml

I don't know what the right name is, but "pc" seems too generic. Is there something reasonable that is more specific we could call this crate?

I don't know why this is marked outdated. It looks like this is still a relevant question.

The intention of this chip crate is to provide drivers for peripherals found on what I'd call "legacy PC BIOS" platforms - Mostly older I/O port-based devices which are available in QEMU (and possibly still found on modern PC systems?) - but which are not strictly part of the ISA itself. Totally open to suggestions on what a better name could be, maybe pc_bios?

Might also be worth debating whether this is actually a "chip" crate, or perhaps the drivers belong in the board folder somewhere to be shared by other boards that fall into the "PC BIOS" category.

My take on this is that the chips crate represents an SoC. In the case of x86, the SoC does not actually exist, but I mostly composed out of the procesor and the mainboard (chipset + default peripherals). I would suggest renaming it to x86_q35.

What's in the chip crate does the things that chip crates do, so I think that is the right place.

It's fine to have two crates, maybe pc and q35, with q35 just re-exporting what is in pc. Then the board includes q35. The pc crate can contain a host of peripherals, and more specific chip crates include what is relevant.

For now I would suggest to leave one single crate as we have only q35 supported and if another x86 chip is added we can do the split.

This is similar to how the STM chips where added.

What is that crate called?

stm32f4xx and nrf5x

But what would it be called in this PR?

I would suggest to name it x86_q35, as @alexandruradovici suggested.

boards/qemu_i486_q35/src/main.rs

alevy · 2025-03-26T23:27:20Z

@reynoldsbd et al, are you able to maybe take a look at this? In particular, you might have the ability to test functional correctness with a downstream with more functionality elsewhere.

In general, my sense is that this looks like what we would want upstream, and if it otherwise smells right (basically works and doesn't have soundness etc issues), we should merge and fix functional issues as they arise (potentially as downstream users find them when moving to this implementation).

boards/qemu_i486_q35/Makefile

Co-authored-by: Brad Campbell <bradjc5@gmail.com>

reynoldsbd · 2025-04-01T22:36:14Z

This is awesome, thanks so much @ioanaculic for taking this closer to the finish line! :)

This all looks sensible on first glance. My team is doing some internal validation as well to make sure this all continues to work on our platforms. I'll follow up once we have some results to share. cc @alevy

One last topic: Apart from the x86 dependency, it's not immediately clear to me whether any of the other feedback or pending opens from the original PR have been addressed in this new PR. There was some good feedback over there and I'd hate for it to slip through the cracks. It'd also be nice to have the original PR description copied over to this one; I personally place high value on leaving a good trail in Git history, but I recognize that's not a top priority for everyone.

lschuermann · 2025-04-02T16:34:30Z

We've discussed this briefly on the core call today.

@reynoldsbd It seems like this already includes all of the other changes which you made on the original PR in response to feedback, and is a strict superset of the commits of that PR.

Therefore, it seems easiest to reference back to #4171, perhaps take some of the original PR's description and feedback integrate it into this one, and finally push this new PR over the finish line.

One of our remaining concerns is around testing these changes, especially running userspace applications, and testing the memory protection implementation. One thing that could allow us to better test these changes is if you could provide a small dummy / test application (either in source or binary form), with instructions on how to load it?

This all looks sensible on first glance. My team is doing some internal validation as well to make sure this all continues to work on our platforms. I'll follow up once we have some results to share. cc @alevy

It's great to hear that you're testing these changes internally. We'd thus holding off on merging this for a couple more days, and can use that time to gather more reviews generally.

bradjc · 2025-04-02T21:31:16Z

We need to document how to get qemu. The getting started guide doesn't actually include those instructions.

reynoldsbd · 2025-04-05T14:52:02Z

Good news, we confirmed this is all running smoothly on real silicon!

@HMiyaziwala has opened a PR in the libtock-rs repo to add support for x86. That should allow you to use one of the example apps for testing.

ioanaculic · 2025-04-11T15:33:11Z

We need to document how to get qemu. The getting started guide doesn't actually include those instructions.

Done.

arch/x86/src/boundary/return_from_user.s

…te from pc to x86_q35

…tubs.

boards/Makefile.common

bradjc · 2025-04-29T20:55:37Z

boards/Makefile.common

-  TARGET := $(patsubst "%",%,$(TARGET_QUOTES))
+  TARGET := $(patsubst "%",%,$(TARGET_QUOTES)) 
+  TARGET := $(subst .json,,$(patsubst "%",%,$(TARGET_QUOTES)))
 endif


alevy

Apart from my and @bradjc's comments, I think this is good to go.

alevy · 2025-04-29T23:27:20Z

arch/x86/README.md

@@ -0,0 +1,2 @@
+x86 Architecture Support Components
+===================================


This would be a good place to specify what's supported (e.g. that it's 32 bit) as well as any caveats or specific decisions (which features are used, any relationship to specific x86 variants, such as 486 vs 386 vs ...), perhaps specifically how segmentation vs. virtual memory are used in this implementation.

alevy · 2025-04-29T23:28:50Z

arch/x86/src/boundary/boundary_impl.rs

+    /// - 4 dwords for initial upcall arguments
+    /// - 1 dword for initial upcall return address (although this will be zero for init_fn)
+    /// - 4 dwords of scratch space for invoking memop syscalls
+    const MIN_APP_BRK: u32 = 9 * 4;


I wonder if this should be 9 * core::mem::size_of::<usize>() instead of a hard-coded 4, so it would more likely not require changes for 64 bit as well in the future?

alevy · 2025-04-29T23:36:37Z

arch/x86/src/boundary/boundary_impl.rs

+        // - the pointers are properly aligned. This is guaranteed because the
+        //   pointers are all offset multiples of 4 bytes from the stack
+        //   pointer, which is guaranteed to be properly aligned after
+        //   exception entry on Cortex-M. See


on Cortex-M.

Oops...

reynoldsbd and others added 16 commits October 25, 2024 15:18

make: handle custom target json

7299cd0

x86: Initial Arch Crate

85d7741

PC: Initial Chip Crate

b49873f

Add qemu_i486_q35 Virtualized Board

74bdbcd

remove x86 dependency

6b15e0f

working x86

4af2cbe

lints 1

98ba1a8

x86 working

1f49460

Merge branch 'master' into delete_x86_dependency

ffd51e2

fix lint

f721b68

update t\o MachineRegister

ab1b586

fix lints

81c0a5d

add board readme

fb42b48

prevent global_asm from being included with cargo test

50c9926

add boad to boards readme

1c1582e

added chip and arch readme files

07d1e8b

bradjc previously approved these changes Mar 26, 2025

View reviewed changes

alevy reviewed Mar 26, 2025

View reviewed changes

boards/qemu_i486_q35/Makefile Outdated Show resolved Hide resolved

github-actions bot assigned lschuermann Mar 27, 2025

Update boards/qemu_i486_q35/src/main.rs

fa7269a

Co-authored-by: Brad Campbell <bradjc5@gmail.com>

ioanaculic dismissed bradjc’s stale review via fa7269a March 27, 2025 13:24

brghena mentioned this pull request Apr 3, 2025

tock_registers::peripheral!: Sound and testable register definitions #4001

Draft

documentation improvements

2d42848

bradjc reviewed Apr 22, 2025

View reviewed changes

arch/x86/src/boundary/return_from_user.s Outdated Show resolved Hide resolved

removed asm files and replaced them with global asm; renamed chip cra…

e5f144a

…te from pc to x86_q35

ioanaculic force-pushed the delete_x86_dependency branch from eaeeec8 to e5f144a Compare April 25, 2025 10:21

mdoify python script to write correctly formatted interrupt handler s…

20f7357

…tubs.

bradjc reviewed Apr 28, 2025

View reviewed changes

boards/Makefile.common Outdated Show resolved Hide resolved

fixed target with ".json" extension in makefile

cf0f8c8

bradjc reviewed Apr 29, 2025

View reviewed changes

alevy requested changes Apr 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the port to the x86 Architecture #4385

Fix the port to the x86 Architecture #4385

ioanaculic commented Mar 26, 2025

bradjc left a comment

bradjc Mar 26, 2025

bradjc Apr 2, 2025

reynoldsbd Apr 3, 2025

alexandruradovici Apr 10, 2025

bradjc Apr 10, 2025

alexandruradovici Apr 15, 2025

bradjc Apr 16, 2025

alexandruradovici Apr 16, 2025

bradjc Apr 18, 2025

ioanaculic Apr 25, 2025

alevy commented Mar 26, 2025

reynoldsbd commented Apr 1, 2025

lschuermann commented Apr 2, 2025

bradjc commented Apr 2, 2025

reynoldsbd commented Apr 5, 2025

ioanaculic commented Apr 11, 2025

bradjc Apr 29, 2025

alevy left a comment

alevy Apr 29, 2025

alevy Apr 29, 2025

alevy Apr 29, 2025

		@@ -0,0 +1,2 @@
		x86 Architecture Support Components
		===================================

Fix the port to the x86 Architecture #4385

Are you sure you want to change the base?

Fix the port to the x86 Architecture #4385

Conversation

ioanaculic commented Mar 26, 2025

Pull Request Overview

Testing Strategy

TODO or Help Wanted

Documentation Updated

Formatting

bradjc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alevy commented Mar 26, 2025

reynoldsbd commented Apr 1, 2025

lschuermann commented Apr 2, 2025

bradjc commented Apr 2, 2025

reynoldsbd commented Apr 5, 2025

ioanaculic commented Apr 11, 2025

Choose a reason for hiding this comment

alevy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment