acpi: add DSDT device type #1165
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,14 @@ | |
| //! | ||
| //! Structs that implement the [`DsdtGenerator`] trait can be passed to | ||
| //! [`DsdtConfig`] and their AML code will be added to the scope they selected. | ||
| //! [`DsdtGenerator`] implementations can also use [`DsdtDeviceType`] to | ||
| //! indicate that the code being generated represents a specific type of | ||
| //! device that should be placed in specific parts of the table. | ||
| //! | ||
| //! # Panics | ||
| //! | ||
| //! Table generation panics if a [`DsdtGenerator`] is defined in [`DsdtConfig`] | ||
| //! but it is not used during table generation. | ||
|
|
||
| // The DSDT and SSDT tables generated here are kept consistent with the | ||
| // original EDK2 static tables. | ||
|
|
@@ -26,6 +34,8 @@ | |
| // | ||
| // This pattern is already used for some devices when possible. | ||
|
|
||
| use std::cell::RefCell; | ||
|
|
||
| use super::aml::{devids, methods, names, paths, *}; | ||
| use super::{ | ||
| AcpiVariant, GPE0_BLK_ADDR, GPE0_BLK_LEN, IO_APIC_ADDR, IO_APIC_LEN, | ||
|
|
@@ -50,13 +60,34 @@ pub enum DsdtScope { | |
| Lpc, // \_SB.PCI0.LPC scope. | ||
| } | ||
|
|
||
| /// The types of devices represented in specific parts of the DSDT. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub enum DsdtDeviceType { | ||
| PS2Ctrl, | ||
| Uart, | ||
| } | ||
|
|
||
| /// An implementer of DsdtGenerator is able to generate AML code to be loaded | ||
| /// into the DSDT ACPI table. | ||
| pub trait DsdtGenerator { | ||
| /// Returns the scope of the DSDT table in which the generated AML code | ||
| /// should be placed. | ||
| fn dsdt_scope(&self) -> DsdtScope; | ||
|
|
||
| /// Returns the device type this generator represents. | ||
| /// | ||
| /// This value can be used to place the generated code into specific | ||
| /// positions in the DSDT. | ||
| /// | ||
| /// If `None`, the generated code is not guaranteed have a stable position. | ||
| /// | ||
| /// If there is more than one generator for a given device type, their | ||
| /// generated code will be positioned in the same order as their generators | ||
| /// are defined in DsdtConfig. | ||
| fn device_type(&self) -> Option<DsdtDeviceType> { | ||
| None | ||
| } | ||
|
|
||
| fn to_aml_bytes( | ||
| &self, | ||
| acpi_variant: AcpiVariant, | ||
|
|
@@ -68,19 +99,26 @@ pub trait DsdtGenerator { | |
| /// Wraps a list of DsdtGenerators to help generate their AML code in places | ||
| /// where a `&dyn Aml` is needed. | ||
| struct DsdtGeneratorAml<'a> { | ||
| dsdt: &'a Dsdt<'a>, | ||
| scope: DsdtScope, | ||
| device_type: Option<DsdtDeviceType>, | ||
| } | ||
| impl<'a> Aml for DsdtGeneratorAml<'a> { | ||
| fn to_aml_bytes(&self, sink: &mut dyn AmlSink) { | ||
| self.dsdt | ||
| .config | ||
| .generators | ||
| .iter() | ||
| .enumerate() | ||
| .filter(|&(_, &g)| { | ||
| g.dsdt_scope() == self.scope | ||
| && g.device_type() == self.device_type | ||
| }) | ||
| .for_each(|(i, &g)| { | ||
| self.dsdt.generator_used(i); | ||
| g.to_aml_bytes( | ||
| self.dsdt.config.acpi_variant, | ||
| self.dsdt.config.device_metadata, | ||
| sink, | ||
| ) | ||
| }); | ||
|
|
@@ -116,11 +154,40 @@ pub struct DsdtConfig<'a> { | |
| /// ACPI rev. 6.6 section 5.2.11.1 "Differentiated System Description Table (DSDT)" | ||
| pub struct Dsdt<'a> { | ||
| config: DsdtConfig<'a>, | ||
|
|
||
| /// Track which generators have been used during table generation to ensure | ||
| /// all of them are called at least once. | ||
| /// | ||
| /// Table generation is a sequential single-threaded process that writes | ||
| /// to a common sink, so generators are guaranteed to be called (and | ||
| /// `borrow_mut()` this `RefCell`) one at a time. | ||
| generators_used: RefCell<Vec<bool>>, | ||
| } | ||
|
|
||
| impl<'a> Dsdt<'a> { | ||
| pub fn new(config: DsdtConfig<'a>) -> Self { | ||
| let generators_used = | ||
| RefCell::new(vec![false; config.generators.len()]); | ||
| Self { config, generators_used } | ||
| } | ||
|
|
||
| fn generator_used(&self, i: usize) { | ||
| self.generators_used.borrow_mut()[i] = true | ||
| } | ||
| } | ||
|
|
||
| impl<'a> Drop for Dsdt<'a> { | ||
| fn drop(&mut self) { | ||
| let leftovers: Vec<_> = self | ||
| .generators_used | ||
| .borrow() | ||
| .iter() | ||
| .enumerate() | ||
| .filter_map(|(i, &used)| if used { None } else { Some(i) }) | ||
| .collect(); | ||
| if leftovers.len() > 0 { | ||
| panic!("DSDT generators not called: {:?}", leftovers); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -152,10 +219,11 @@ impl<'a> Aml for Dsdt<'a> { | |
| aml::Scope::new( | ||
| "_SB_".into(), | ||
| vec![ | ||
| &PciRootBridge { dsdt: &self }, | ||
| &DsdtGeneratorAml { | ||
| dsdt: &self, | ||
| scope: DsdtScope::SystemBus, | ||
| device_type: None, | ||
|
Member
There was a problem hiding this comment. the main thought i've got here is: while i think this gives Propolis good control over how to lay out items in the tables (and so, broadly, i'm a fan!) if we have a device on i'm imagining that maybe we could have a bit on devices to report if we've printed their AML, though
Member
Author
There was a problem hiding this comment. Yeah, that's a good point. Is 8d38895 more or less what you had in mind?
Member
Author
There was a problem hiding this comment. Looking at this with fresh eyes today, I think it makes more sense to keep state in |
||
| }, | ||
| ], | ||
| ) | ||
|
|
@@ -170,7 +238,7 @@ impl<'a> Aml for Dsdt<'a> { | |
|
|
||
| /// PCI root bridge namespace (\_SB.PCI0). | ||
| struct PciRootBridge<'a> { | ||
| dsdt: &'a Dsdt<'a>, | ||
| } | ||
|
|
||
| impl<'a> Aml for PciRootBridge<'a> { | ||
|
|
@@ -184,10 +252,11 @@ impl<'a> Aml for PciRootBridge<'a> { | |
| &names::uid(&aml::ZERO), | ||
| &PciRootBridgeCrs {}, | ||
| &PciRootBridgePrt {}, | ||
| &PciRootBridgeLpc { dsdt: self.dsdt }, | ||
| &DsdtGeneratorAml { | ||
| dsdt: self.dsdt, | ||
| scope: DsdtScope::PciRoot, | ||
| device_type: None, | ||
| }, | ||
| ], | ||
| ) | ||
|
|
@@ -538,7 +607,7 @@ const NMISC_PORT: u16 = 0x61; | |
| /// | ||
| /// <https://github.com/oxidecomputer/edk2/blob/f33871f488bfbbc080e0f7e3881e04d0db0b6367/OvmfPkg/AcpiTables/Dsdt.asl#L299-L303> | ||
| struct PciRootBridgeLpc<'a> { | ||
| dsdt: &'a Dsdt<'a>, | ||
| } | ||
|
|
||
| // This table is currently kept the same as the original EDK2 static table, but | ||
|
|
@@ -663,7 +732,7 @@ impl<'a> Aml for PciRootBridgeLpc<'a> { | |
| // table. Its IO ports are not actually handled anywhere. | ||
| // It can be removed in the future . | ||
| &AcpiVariantFilter::new( | ||
| self.dsdt.config.acpi_variant, | ||
| vec![AcpiVariant::V0], | ||
| &aml::Device::new( | ||
| "DMAC".into(), | ||
|
|
@@ -730,7 +799,7 @@ impl<'a> Aml for PciRootBridgeLpc<'a> { | |
| // table. Its IO ports are not actually handled anywhere. | ||
| // It can be removed in the future . | ||
| &AcpiVariantFilter::new( | ||
| self.dsdt.config.acpi_variant, | ||
| vec![AcpiVariant::V0], | ||
| &aml::Device::new( | ||
| "FPU_".into(), | ||
|
|
@@ -804,8 +873,14 @@ impl<'a> Aml for PciRootBridgeLpc<'a> { | |
| ], | ||
| ), | ||
| &DsdtGeneratorAml { | ||
| dsdt: self.dsdt, | ||
| scope: DsdtScope::Lpc, | ||
| device_type: Some(DsdtDeviceType::PS2Ctrl), | ||
| }, | ||
| &DsdtGeneratorAml { | ||
| dsdt: self.dsdt, | ||
| scope: DsdtScope::Lpc, | ||
| device_type: Some(DsdtDeviceType::Uart), | ||
| }, | ||
| // QEMU panic device. | ||
| // | ||
|
|
@@ -859,6 +934,11 @@ impl<'a> Aml for PciRootBridgeLpc<'a> { | |
| ), | ||
| ], | ||
| ), | ||
| &DsdtGeneratorAml { | ||
| dsdt: self.dsdt, | ||
| scope: DsdtScope::Lpc, | ||
| device_type: None, | ||
| }, | ||
| ], | ||
| ) | ||
| .to_aml_bytes(sink); | ||
|
|
@@ -1043,12 +1123,17 @@ mod test { | |
|
|
||
| struct MockDsdtGenerator { | ||
| scope: DsdtScope, | ||
| device_type: Option<DsdtDeviceType>, | ||
| } | ||
| impl DsdtGenerator for MockDsdtGenerator { | ||
| fn dsdt_scope(&self) -> DsdtScope { | ||
| self.scope | ||
| } | ||
|
|
||
| fn device_type(&self) -> Option<DsdtDeviceType> { | ||
| self.device_type | ||
| } | ||
|
|
||
| fn to_aml_bytes( | ||
| &self, | ||
| acpi_variant: AcpiVariant, | ||
|
|
@@ -1089,11 +1174,20 @@ mod test { | |
|
|
||
| #[test] | ||
| fn dsdt_generator_aml() { | ||
| let sb = Arc::new(MockDsdtGenerator { | ||
| scope: DsdtScope::SystemBus, | ||
| device_type: None, | ||
| }); | ||
| let pci = Arc::new(MockDsdtGenerator { | ||
| scope: DsdtScope::PciRoot, | ||
| device_type: None, | ||
| }); | ||
| let lpc_ps2 = Arc::new(MockDsdtGenerator { | ||
| scope: DsdtScope::Lpc, | ||
| device_type: Some(DsdtDeviceType::PS2Ctrl), | ||
| }); | ||
|
|
||
| let generators: Vec<&dyn DsdtGenerator> = vec![&*sb, &*pci, &*lpc_ps2]; | ||
|
|
||
| let mut got = Vec::new(); | ||
| let mut expected = Vec::new(); | ||
|
|
@@ -1104,17 +1198,21 @@ mod test { | |
| device_metadata | ||
| .insert(&pci, Box::new(MockDsdtGeneratorMetadata { data: 2 })); | ||
| device_metadata | ||
| .insert(&lpc_ps2, Box::new(MockDsdtGeneratorMetadata { data: 3 })); | ||
|
|
||
| let dsdt = Dsdt::new(DsdtConfig { | ||
| acpi_variant: AcpiVariant::V0, | ||
| generators: &generators, | ||
| device_metadata: &device_metadata, | ||
| }); | ||
|
|
||
| // Filter by SystemBus. | ||
| DsdtGeneratorAml { | ||
| dsdt: &dsdt, | ||
| scope: DsdtScope::SystemBus, | ||
| device_type: None, | ||
| } | ||
| .to_aml_bytes(&mut got); | ||
|
|
||
| aml::ResourceTemplate::new(vec![ | ||
| &aml::Name::new("SCOP".into(), &"SystemBus"), | ||
|
|
@@ -1129,8 +1227,12 @@ mod test { | |
| got.clear(); | ||
| expected.clear(); | ||
|
|
||
| DsdtGeneratorAml { | ||
| dsdt: &dsdt, | ||
| scope: DsdtScope::PciRoot, | ||
| device_type: None, | ||
| } | ||
| .to_aml_bytes(&mut got); | ||
|
|
||
| aml::ResourceTemplate::new(vec![ | ||
| &aml::Name::new("SCOP".into(), &"PciRoot"), | ||
|
|
@@ -1141,12 +1243,24 @@ mod test { | |
|
|
||
| assert_eq!(expected, got); | ||
|
|
||
| // Filter by Lpc scope and device type. | ||
| got.clear(); | ||
| expected.clear(); | ||
|
|
||
| DsdtGeneratorAml { | ||
| dsdt: &dsdt, | ||
| scope: DsdtScope::Lpc, | ||
| device_type: Some(DsdtDeviceType::PS2Ctrl), | ||
| } | ||
| .to_aml_bytes(&mut got); | ||
|
|
||
| // Filter on device type without generators. | ||
| DsdtGeneratorAml { | ||
| dsdt: &dsdt, | ||
| scope: DsdtScope::Lpc, | ||
| device_type: Some(DsdtDeviceType::Uart), | ||
| } | ||
| .to_aml_bytes(&mut got); | ||
|
|
||
| aml::ResourceTemplate::new(vec![ | ||
| &aml::Name::new("SCOP".into(), &"Lpc"), | ||
|
|
@@ -1158,18 +1272,49 @@ mod test { | |
| assert_eq!(expected, got); | ||
| } | ||
|
|
||
| #[test] | ||
| #[should_panic(expected = "DSDT generators not called")] | ||
|
|
||
| fn panic_if_generator_not_called() { | ||
| let lpc_uart = Arc::new(MockDsdtGenerator { | ||
| scope: DsdtScope::Lpc, | ||
| device_type: Some(DsdtDeviceType::Uart), | ||
| }); | ||
| let generators: Vec<&dyn DsdtGenerator> = vec![&*lpc_uart]; | ||
| let device_metadata = DeviceMetadataMap::new(); | ||
| let dsdt = Dsdt::new(DsdtConfig { | ||
| acpi_variant: AcpiVariant::V0, | ||
| generators: &generators, | ||
| device_metadata: &device_metadata, | ||
| }); | ||
|
|
||
| // Generate AML code without calling the lpc_uart generator. | ||
| let mut sink = Vec::new(); | ||
| DsdtGeneratorAml { | ||
| dsdt: &dsdt, | ||
| scope: DsdtScope::SystemBus, | ||
| device_type: None, | ||
| } | ||
| .to_aml_bytes(&mut sink); | ||
| } | ||
|
|
||
| #[test] | ||
| fn dsdt_valid_aml() { | ||
| let device_metadata = DeviceMetadataMap::new(); | ||
| let dsdt = Dsdt::new(DsdtConfig { | ||
| acpi_variant: AcpiVariant::V0, | ||
| generators: &[ | ||
| &MockDsdtGenerator { | ||
| scope: DsdtScope::SystemBus, | ||
| device_type: None, | ||
| }, | ||
| &MockDsdtGenerator { | ||
| scope: DsdtScope::PciRoot, | ||
| device_type: None, | ||
| }, | ||
| &MockDsdtGenerator { scope: DsdtScope::Lpc, device_type: None }, | ||
| ], | ||
| device_metadata: &device_metadata, | ||
| }); | ||
| let mut aml = Vec::new(); | ||
| dsdt.to_aml_bytes(&mut aml); | ||
|
|
||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so.. I'm realizing one wrinkle we'll still have is that we're at the whim of SpecKey ordering for a given
device_type. that is, if something sendspropolis-serveran InstanceSpec which has four serial ports namedcom{1,2,3,4}, then changes how it adds serial ports such that they're namedcom{4,3,2,1}but name ports in ascending order, we'll dutifully reverse the order of those ports' AML.we can probably demand that items implement
Ordso we can sort the filtered generators? but this is all pretty niche compared to the current issue you're fixing. and it's not like we have anything else that's present multiple times in the ACPI tables. so as emphatically as I can say: I think we should land this with an issue and note about the remainingSpecKeysensitivityThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum...right. Although the generators are sorted by
SpecKey, so you would need to have a weird combination where the serial port key didn't quite match the port number. Weird, but still possible 😄I added a note about it in a2f33de and I will open a follow-up issue.