The expect message is not very helpful.

Sorry, some leftover debug messages.

Maybe it's explained in the missing part of the commit message, but it's not clear why these two setup steps are so different. Is it an error on x86_64 to create the vcpus earlier? Why can't the first step be load_kernel on both?

I updated the commit message and also put a comment before branching the implementation. Take a look.

Looks much cleaner now.

Could it be possible to further reduce duplication by doing the following:

            let entry_addr = self.load_kernel()?;

            #[cfg(target_arch = "aarch64")]
            vcpus = self.create_vcpus(entry_addr, request_ts)?;

            self.setup_interrupt_controller()?;
            self.attach_virtio_devices()?;
            self.attach_legacy_devices()?;

            #[cfg(target_arch = "x86_64")]
            vcpus = self.create_vcpus(entry_addr, request_ts)?;

nit: Would be nice to also add a comment for both like:

// On aarch64 we need to create vcpus before creating the interrupt controller.
#[cfg(target_arch = "aarch64")]
vcpus = self.create_vcpus(entry_addr, request_ts)?;
...
// On x86_64 we need to create vcpus after setting up everything else.
#[cfg(target_arch = "x86_64")]
vcpus = self.create_vcpus(entry_addr, request_ts)?;

(Or smth similar)

Why are register_events and configure_system swapped?

No reason and I also just realized that there is no reason in having them inside the aarch64 cfg. I removed them from there. Take a look.

Is this test not run on aarch64? Wouldn't it need len=4 on arm?

Same question for all following tests.

It is run on aarch64 too, but given that this is an unit test it will hit the same code like on x86.
What makes a difference here is the enable_earlycon (from a later commit) function that indeed is specific to aarch64 (not even specific it is just that on aarch64 we need to do it through MMIO addreses os the data.len will be 4).
TO be honest, what I see worth unit testing here is checking that the METRICS.uart.missed_read_count.inc();
stays 0 after a read that has the same length as the length to what the device was instructed to accept inside the constructor.
Another thing that we could test here is that when we initiliaze the length to some different length different than 1, the device knows to read data only of that length.

Exercised all of the above with test_serial_data_len. Take a look.

nit: Maybe dev_type instead of type_?

I thought about this but dev_type inside MMIODeviceInfo?

I thought about this but dev_type inside MMIODeviceInfo?

Sounds good to me 😆, but like I said, it's a nit.

nit: reorder suggestion: put len right after addr since it's a familiar pattern.

I tried to maintain alphabetical order :(

Still a nit, but addr and len are logically tied, they describe the device address space slice, and that trumps alphabetical order for me.

Doesn't need cloning. You're already cloning when calling this function. Either:

• make id param as id: Option<&String> and clone it here (callers of the func would not clone id anymore),
  or
• leave id param as it is, and consume it here instead of cloning.

Done, I actually made id mandatory (not an Option anymore). Leaving it as Option made the code very prone to programming errors since on aarch64, non mentioning an id would not make it part of the FDT. Take a look.

type_ would look better as an enum clearly stating virtio or serial instead of an u32 on which we map certain numbers to certain types.

Done, take a look.

nit: looks like some extra indentation slipped in here

Done.

Haven't looked at the implications, but would like to see this returning a reference instead of a clone.

I can indeed return a reference here. Take a look.

Commit af4d5a0b292dfc6ef6b381d75f76c4f5ff2c3701 has an incomplete description.

Solved.

Leftover commented line.

Solved.

What do you think about making type_ in MMIODeviceInfo also an enum, ideally the same enum as FDTDeviceType.

I think you can call this something generic (not fdt related) and define it for both x86_64 and aarch64 in order to use it everywhere.

Good suggestion. Take a look.