Add ACPI support for x86_64

[bchalios]

Changes

This is a WIP rough draft of ACPI support for x86. Opening this PR for testing the PR itself but also the CI artifacts.

Reason

TBD

License Acceptance

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check CONTRIBUTING.md.

PR Checklist

• [ ] If a specific issue led to this PR, this PR closes the issue.
• [ ] The description of changes is clear and encompassing.
• [ ] Any required documentation changes (code and docs) are included in this PR.
• [ ] API changes follow the Runbook for Firecracker API changes.
• [ ] User-facing changes are mentioned in CHANGELOG.md.
• [ ] All added/changed functionality is tested.
• [ ] New TODOs link to an issue.
• [ ] Commits meet contribution quality standards.

---

• [ ] This functionality cannot be added in rust-vmm.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 94.20168% with 69 lines in your changes are missing coverage. Please review.

Project coverage is 82.01%. Comparing base (138a189) to head (a2663cc).

:exclamation: Current head a2663cc differs from pull request most recent head df09c5d. Consider uploading reports for the commit df09c5d to get more accurate results

Files	Patch %	Lines
src/acpi-tables/src/aml.rs	95.04%	29 Missing :warning:
src/acpi-tables/src/lib.rs	60.41%	19 Missing :warning:
src/vmm/src/acpi/mod.rs	94.11%	5 Missing :warning:
src/acpi-tables/src/madt.rs	93.93%	4 Missing :warning:
src/acpi-tables/src/rsdp.rs	84.61%	4 Missing :warning:
src/vmm/src/device_manager/mmio.rs	95.31%	3 Missing :warning:
src/acpi-tables/src/dsdt.rs	97.36%	1 Missing :warning:
src/acpi-tables/src/xsdt.rs	97.67%	1 Missing :warning:
src/vmm/src/arch/x86_64/mptable.rs	90.90%	1 Missing :warning:
src/vmm/src/builder.rs	95.65%	1 Missing :warning:
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4428      +/-   ##
==========================================
+ Coverage   81.56%   82.01%   +0.45%     
==========================================
  Files         243      253      +10     
  Lines       29899    31026    +1127     
==========================================
+ Hits        24387    25447    +1060     
- Misses       5512     5579      +67     

Flag	Coverage Δ
4.14-c5n.metal	79.47% <94.42%> (+0.60%)	:arrow_up:
4.14-c7g.metal	?
4.14-m5d.metal	?
4.14-m5n.metal	79.46% <94.42%> (+0.60%)	:arrow_up:
4.14-m6a.metal	78.68% <94.42%> (+0.65%)	:arrow_up:
4.14-m6g.metal	76.65% <68.91%> (-0.32%)	:arrow_down:
4.14-m6i.metal	79.46% <94.42%> (+0.60%)	:arrow_up:
4.14-m7g.metal	76.65% <68.91%> (-0.32%)	:arrow_down:
5.10-c5n.metal	82.01% <94.42%> (+0.49%)	:arrow_up:
5.10-c7g.metal	?
5.10-m5d.metal	?
5.10-m5n.metal	82.00% <94.42%> (+0.49%)	:arrow_up:
5.10-m6a.metal	81.30% <94.42%> (+0.54%)	:arrow_up:
5.10-m6g.metal	79.43% <68.91%> (-0.41%)	:arrow_down:
5.10-m6i.metal	82.00% <94.42%> (+0.49%)	:arrow_up:
5.10-m7g.metal	79.43% <68.91%> (-0.41%)	:arrow_down:
6.1-c5n.metal	82.01% <94.42%> (+0.49%)	:arrow_up:
6.1-c7g.metal	?
6.1-m5d.metal	?
6.1-m5n.metal	82.00% <94.42%> (+0.49%)	:arrow_up:
6.1-m6a.metal	81.30% <94.42%> (+0.54%)	:arrow_up:
6.1-m6g.metal	79.43% <68.91%> (-0.41%)	:arrow_down:
6.1-m6i.metal	82.00% <94.42%> (+0.49%)	:arrow_up:
6.1-m7g.metal	79.43% <68.91%> (-0.41%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[bchalios]

This is in a much better state now. Things that I still want to do is:

1. Add unit-tests for the newly introduced code.
2. Check if we really need to add AML code for legacy devices.
3. Re-work the commit messages
4. Sanitize logging

[bchalios]

Thanks for your review Egor! I think your comments make sense. Let me write what my thinking was behind the design choices.

• Combining allocators for different resources into 1 struct seems unnecessary, especially considerring that in the end this sturct has different methods to allocate resources from each allocator separately.

I would probably agree if it wasn't for the GSI allocations. The next PR, will add support for the VMGenID device. Both MMIO and ACPI/system devices need IRQs. Unless we have a common allocator we would need to statically divide the IRQ numbers between the two types of devices. We could do that, after all we only have one such device now (VMGenID) but I think it would be much more clanky to do so.

• Sharing allocators between Vmm and MMIODeviceManager is counter intuitive. Vmm should not allocate anything, it has MMIODeviceManager for this

Vmm doesn't, ResourceAllocator does. It just lives inside the Vmm struct (same as MMIODeviceManager). My intention was to create a component that handles guest resources, i.e. MMIO address space, System address space and IRQs, in one place. The main motivation was the fact that we need to share IRQs between VirtIO and ACPI devices.

• Making Vmm as an entity to create acpi tables is also unnecessary mixing of responsibilities. As I can see to create acpi tables, only guest_memory and mmio_infos are needed. It is simper to just pass them to some acpi table function that will do all the work and leave Vmm out of this.

On this I initially had the exact same though as you. I'm not sure I like it 100% either. The reason why I went with this approach is the separation between x86 and aarch64 (eventually, we will want to add support for ARM as well). In x86 we need guest memory, VirtIO devices, number of vCPUS, PIO devices and (in the next PR) VMGenID and Generic Event Device info (that's 6 arguments). On ARM we won't need PIO devices but we might need to pass over the interrupt controller information.

I really dislike the following pattern:

#[cfg(target_arch = "x86_64")]
call_some_function(put, 6, parameters, in, here, but)
#[cfg(target_arch = "aarch64")]
call_some_function(put, 7, other, parameters, in, here, instead)

I believe that architecture specific code should be isolated in architecture specific code files. It's partly an aesthetic thing for me, but I do believe it makes the code more readable.

If you have an alternative that avoids this pattern, I'm happy to follow.

• Spreading Vmm impementation between files (not just in acpi/mod.rs but also in acpi/x86.rs) degrades the code quality. It makes it harder to understand what is Vmm actually should do because you need to jump though the hoops to find all imps for it. Also as I can see, only setup_arch_dsdt method actually uses self, but then pio_device_manager impl for append_aml_bytes retuns a constant data, so this whole implementation for Vmm actually does not need Vmm.

On spreading impl Vmm I don't have strong feelings. I typically like to organize code in files based on functionality rather than the name of the thing, but once we agree on the previous comments, we can revisit that. I think it's less important.

[ShadowCurse]

I haven't explored the idea throughout, but what if we combine all "device managers" (pio_device_manager, mmio_device_manager, acpi_device_manager) into 1 DeviceManager? This way it can contain all resource allocators and will contain all needed info for acpi tables.

pub struct Vmm {
  ...
  device_manager: DeviceManager,
}

pub fn configure_system_for_boot(...) {
  ...
  // I don't like attaching this to `Vmm`, but it has all needed data for it.
  vmm.create_acpi_tables(vcpus.len())?;
  ...
}

impl Vmm {
  pub fn create_acpi_tables(&self, vcpus: usize) -> ... {
    #[cfg(target_arch = "x86_64")]
    acpi_table::x86::create_acpi_tables(&self.guest_memory, &self.device_manager, ..., vcpus.len())?;
  }
}

[bchalios]

I haven't explored the idea throughout, but what if we combine all "device managers" (pio_device_manager, mmio_device_manager, acpi_device_manager) into 1 DeviceManager? This way it can contain all resource allocators and will contain all needed info for acpi tables.

pub struct Vmm {
  ...
  device_manager: DeviceManager,
}

pub fn configure_system_for_boot(...) {
  ...
  // I don't like attaching this to `Vmm`, but it has all needed data for it.
  vmm.create_acpi_tables(vcpus.len())?;
  ...
}

impl Vmm {
  pub fn create_acpi_tables(&self, vcpus: usize) -> ... {
    #[cfg(target_arch = "x86_64")]
    acpi_table::x86::create_acpi_tables(&self.guest_memory, &self.device_manager, ..., vcpus.len())?;
  }
}

You have raised two issues in your initial comment, essentially:

1. The unification of different allocators in a single container and the fact that Vmm holds a reference to this allocator
2. The fact that ACPI table creation logic holds a reference to the Vmm object.

I don't see how the DeviceManager container changes the code structure much with either problem. Here's why:

Regarding the ResourceAllocator aspect, ACPI and MMIO devices will still need to share the IRQ allocator. With your proposed design, ResourceAllocator will not live in the Vmm object, but in the DeviceManager, but I don't see any practical difference between the two, especially since the latter will still live in the Vmm object.

We still need a single IRQ allocatorfor both types of devices. My argument is that we can have a single point where we allocate resources for the guest in a way that some sort of firmware would (that's what we do when we assign IRQs and allocate system memory (for ACPI or MMIO)).

Regarding the second problem, you don't need a DeviceManager to do that:

impl Vmm {
  pub fn create_acpi_tables(&self, vcpus: usize) -> ... {
    #[cfg(target_arch = "x86_64")]
    acpi_table::x86::crate_acpi_tables(&self.guest_memory, &self.mmio_device_manager, &self.pio_device_manager, ...)?;
    #[cfg(target_arch = "aarch64")]
    acpi_table::aarch64::create_acpi_tables(&self.guest_memory, &self.mmio_device_manager, ...)?;
  }
}

We can already do that. The reason why I didn't want to do that, is that 90% of what create_acpi_tables does is common between x86 and aarch64, so it would be just us replicating code. I wanted to be able to pass to create_acpi_tables the context which is common between the two architectures, so we can de-duplicate code and diverge only when it is needed.

If you feel so strongly about the impl Vmm thing, I think I can get rid of it (your comment about the PIO AML being constant helped me think of that). I will post a v2 based on this idea and let's discuss based on that.

[bchalios]

Changes from v1 to v2:

• Re-worked the logic that writes the tables to guest memory. The functions taking care of that are not members of the Vmm type any more
• Removed the Send and Sync marker traits for Vmm. This was a left over. Silenced the related clippy warnings, adding comments why.
• Re-worked assert!(matches!(...)) blocks to print better messages in case of an error.
• Removed underscores from struct field names in ACPI types. Silenced the clippy for unused code for these structs.
• Added missing copyrights in acpi-tables files
• Small fixes in acpi-tables/src/aml.rs; making code rust idiomatic.

[bchalios]

Changes from v2 to v3:

• Fixed aarch64 build error which was introduced in v2 due to importing of acpi module. This needs to be behind a feature gate (x86 only)

[bchalios]

Changes v3 to v4:

• Added back support for MPTable and passing VirtIO devices via kernel command line
• MPTable uses the memory allocator to get space in guest address memory
• Renamed ACPI memory allocator to "System" memory allocator, to reflect the fact that we do not only allocate memory for ACPI.

[bchalios]

Changes v4 to v5:

• Fixed outdated comments regarding the location of ACPI data and e820 entries
• We now use a dedicated function for create AML data for VirtIO devices that receives exactly one IRQ. This way we are sure at compile time that we only created AML data for VirtIO devices (and not other MMIO devices such as the boot timer).
• Passing ResourceAllocator as a simple reference rather than an Rc to the ACPI module.
• Added a CHANGELOG entry, regarding ACPI support and announcing MPTable deprecation period.

[bchalios]

Changes from v5 to v6:

• Dropped implementation of Aml trait for MMIODeviceManager. We now access the AML data directly from MMIODeviceManager::dsdt_data.
• Fixes in CHANGELOG wording.

[bchalios]

Changes from v6 to v7:

• Do not store the ResourceAllocator inside the MMIODeviceManager. Pass it only when we register devices. This allows us to only keep the ResourceAllocator in Vmm and not have to store it inside an Rc.

[bchalios]

Changes from v7 to v8:

• Fixed test_net_change_mac_address to work both with and without ACPI support.
• Dropped the temporary switch to ACPI-enabled CI artifacts.
• Added documentation about kernel configurations needed for booting guest kernels with ACPI.

[bchalios]

Changes from v8 to v9:

• Pinned 5.10 guest kernel to 5.10.210 for our CI artifacts. Kernels after that don't build with our configuration. Will open issue to track the resolution of this
• Added a kernel configuration with ACPI disabled, so that we can test MPTables as well on x86.
• Mention MPTable deprecation in DEPRECATED.md
• Small fixes in resource manager documentation
• Add descriptions about the various ACPI data structures in acpi-tables crate pointing also to the latest ACPI specification
• Fixes in CHANGELOG wording
• Added a bit more info in docs/kernel-policy.md regarding our supported configurations during the MPTable deprecation period.

[wearyzen]

Thanks for posting CHANGELOG for each version!