Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AML: Make PciRoutingTable fields public #171

Open
rw-vanc opened this issue Mar 29, 2023 · 6 comments
Open

AML: Make PciRoutingTable fields public #171

rw-vanc opened this issue Mar 29, 2023 · 6 comments

Comments

@rw-vanc
Copy link
Contributor

rw-vanc commented Mar 29, 2023

I would like to be able to provide a different interface to the PciRoutingTable. I need to map the contents to a serializable struct, but the fields are not pub. Let me know if you are ok with me making this change.

@IsaacWoods
Copy link
Member

If needed this would be okay, but generally I'm not in favour of directly exposing the ManagedSlice type if we can help it - for the MCFG in particular the iter method provides an extremely close representation to the raw table but in a nicer form - you could map each entry to a type that implements your serializable trait and collect it, for example.

Let me know if that's not sufficient, and we can explore other options.

@rw-vanc
Copy link
Contributor Author

rw-vanc commented Mar 29, 2023

Let me come at it from a broader perspective, and maybe get your insight on it. Our acpid is a task that manages access to the ACPI and AML data and services, and communicates with drivers via a serde derive set of interfaces. Some drivers will be trusted and will be able to address phys mem, and some drivers will be virtualized. Virtualized drivers may get a scrubbed serialized interface. We have an initial implementation of DMAR that we plan to serialize (we can contribute that if it helps you). Our pcid driver has it's own custom MCFG code, but I would like to pull that functionality back into acpid, and provide enough serialized info that pcid can do its job well. Do you think this is in conflict with what you are doing? What is the best way for me to achieve my goals, with PRT, with MCFG, etc.? I have no issue with re-rolling all the info into our own custom structs for serde, I think that will be needed no matter what. It's just a matter of the data being public in some way. (Caveat: Some basic stuff that needs to be done to get the kernel and filesystem running will continue to need custom code.)

@IsaacWoods
Copy link
Member

IsaacWoods commented Mar 30, 2023

Interesting! Do you have a link to the relevant code so I can understand what you're trying to achieve more?

Our pcid driver has it's own custom MCFG code, but I would like to pull that functionality back into acpid, and provide enough serialized info that pcid can do its job well

Sure - I do a similar thing but in the kernel and pass it out to a userspace driver after PCI enumeration. For your usecase, I think passing out information about each MCFG region and the buses it covers would suffice. I've realised that this code isn't actually in a published version of acpi, so apologies if that was confusing - it was introduced in #132 and will be published as part of the next major version of acpi. Are you using a published version or a custom fork - if the latter then you'll be able to use it already.

We should really get this published - I'll try and put some time aside in the week to have a look and get a new version of acpi out.

An alternative to you maintaining a separate set of types to be serialised would be to mark up types in acpi / aml with Serialize + Deserialize behind a feature flag so you can serialize our types directly. The disadvantage to this is that you'd be exposing breaking changes to our API through your userspace barrier between drivers, which might be tricky. I think either way would be fine; which would work better for you do you think?

We have an initial implementation of DMAR that we plan to serialize (we can contribute that if it helps you).

Yep, this would always be welcome!

Do you think this is in conflict with what you are doing?

Definitely not - this library exists to cater to whatever users need to do with it, basically. It's just about agreeing on the best way to expose the information in a safe + convenient way for a range of use cases. Please lmk if you have more questions / any of this doesn't make sense.

@rw-vanc
Copy link
Contributor Author

rw-vanc commented Mar 30, 2023

This is the repo for our acpi code. It's currently mixed in with several other drivers.
https://gitlab.redox-os.org/redox-os/drivers

acpid is the ACPI driver. We are not currently using any code from the acpi crate because we did not yet try to get it working in userspace. We are using a fork of the aml crate that just contains all of the PRs that I submitted, so I can continue to move forward with debugging. You can see the fork reference in acpids Cargo.toml.

The directory amlserde is the serialization library. There's a bunch of things that we are currently doing that make directly deriving within the crate viable. The format of symbol names, needing to map AmlHandle references to names, etc. But I don't have your or Jeremy's level of expertise in ACPI; if we take a step back at identifying what's actually needed for device drivers, it might be good for both of us to have a serializable interface that meets that need.

@IsaacWoods
Copy link
Member

Ahh, my apologies for the confusion. With the discussion of the MCFG I completely missed over the fact that you were talking about the PRT here. We could totally make the fields of PciRoute and PciRoutingTable public - then it should be possible to serialize and deserialize them between the drivers and use PciRoutingTable::route in pcid if you'd like, since it's non-trivial functionality.

Looking at amlscheme, I definitely think a lot of the boilerplate could be moved into aml itself under a serde feature with some #[cfg(features = "serde", derive(Serialize, Deserialize)]s, which doesn't sound too onerous. Basically we can apply it to everything that can be easily serialized (the only potential issue would be the handle -> name conversion, I'm not sure there's an easy way to do that without a wrapper type around AmlValue or something), and then you'd just need to make sure you were using compatible versions between the drivers, which it doesn't sound like there's be more than a handful of?

@rw-vanc
Copy link
Contributor Author

rw-vanc commented Mar 30, 2023

Thanks. There's interest in MCFG as well, so I'm going to ask for more input on this topic from the Redox team. Once we decide how we want to move forward, I will continue to file issues and/or make PRs with any changes we require, and you can steer me in the right direction if I do something that doesn't fit with your view.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants