acpi: add DSDT device type#1165
Conversation
Allow `DsdtGenerator` to specify the type of device it is generating code for. The DSDT can then organize devices types in a stable order, regardless of the component name and position in the device map.
| &PciRootBridge { config: &self.config }, | ||
| &DsdtGeneratorAml { | ||
| scope: DsdtScope::SystemBus, | ||
| device_type: None, |
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 SystemBus and forget to add a stanza to emit the AML here, it'll probably be really confusing and frustrating to debug.
i'm imagining that maybe we could have a bit on devices to report if we've printed their AML, though to_aml_bytes takes &self, so we'd either change that or otherwise do interior mutability. then finally we could have an impl Drop for DsdtConfig that checks our work (and warns or panics or something if there are devices we've forgotten about).
There was a problem hiding this comment.
Yeah, that's a good point. Is 8d38895 more or less what you had in mind?
There was a problem hiding this comment.
Looking at this with fresh eyes today, I think it makes more sense to keep state in Dsdt rather DsdtConfig. That way DsdtConfig is just a container for configuration values. I moved the generator state in 42acb24
iximeow
left a comment
iximeow
left a comment
There was a problem hiding this comment.
nice, lets do it! nit is only because I'd want to make sure we don't forget about that last outstanding detail but I really don't think it's worth effort at the moment. thank you for the attention here 🙏
| .filter(|&(_, &g)| { | ||
| g.dsdt_scope() == self.scope | ||
| && g.device_type() == self.device_type | ||
| }) |
There was a problem hiding this comment.
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 sends propolis-server an InstanceSpec which has four serial ports named com{1,2,3,4}, then changes how it adds serial ports such that they're named com{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 Ord so 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 remaining SpecKey sensitivity
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| #[test] | ||
| #[should_panic(expected = "DSDT generators not called")] |
2 participants
Allow
DsdtGeneratorto specify the type of device it is generating code for. The DSDT can then organize devices types in a stable order, regardless of the component name and position in the device map.