working together better with redox

[wt]

Hi, I have been working on redox for a bit, and I came across your library. We can't utilize your lib for ACPI handling because the data is accessed in a very different way on redox. Basically, redox exposes the ACPI tables in as a file. For example, an MCFG might be exposed as the file "acpi:/tables/MCFG-424f43485320-425850434d434647". However, the acpi crate takes physical memory location. I would love to see the acpi somehow support this use case so that we could take advantage of this work.

Do you have any ideas of how this might be achieved?

I'd be happy to submit some patches if I can figure out a way to do it.

Thanks for any help!

[IsaacWoods]

Firstly, I'm very much in favour of centralising efforts across Rust's ACPI users, so if Redox moves to using this library, that's great :)

Basically, redox exposes the ACPI tables in as a file. For example, an MCFG might be exposed as the file "acpi:/tables/MCFG-424f43485320-425850434d434647".

I think the first thing I need to understand is what Redox's overall plan to support ACPI looks like. From the above quote, it sounds like raw ACPI tables are exposed to userspace, which then does the parsing? Will the kernel do any parsing at all, or just go as far as the RSDT/XSDT, find all SDTs and pass them out to userspace?

I ask this because I originally had the same idea for the OS I'm primarily developing acpi against (Pebble), and found it wildly impractical. I get the initial appeal of parsing ACPI in userspace (and think that parsing AML in userspace is both practical and has merit, which is why the acpi and aml crates are separate in the first place), but I don't think the impracticality weighs up against the potential safety gains of parsing static content in kernelspace.

Particularly, I've found that I need information from the static tables very early in kernel initialization. From what I can see, Redox also currently seems to rely on ACPI parsing happening very early on.

In summary, there appear to be two parts of this:

• Providing an interface that basically gives you a list of raw tables, which you could in theory expose to userspace using a VFS / other system
• Using the types in acpi to parse data that userspace been told is a raw table.

Does that match your understanding of the problem?

I'm not necessarily against providing an interface that allows you to do these things, but I'd want to see some more planning to make sure that this would be a useful interface. I'm not sure parsing the static tables outside the kernel is actually a realistic prospect in a non-toy microkernel, and so I'd want to see Redox commit to that route before changing acpi to support it. Hopefully that sounds reasonable :)

Thanks for your interest, and I hope we can come to some sort of understanding where Redox could use at least parts of the acpi and aml ecosystems!

[wt]

This is great.

To answer some of you questions, yes, redox does access acpi from there kernel during the initial spinup to activate other processors and whatnot. However, most of what the kernel does is find tables and expose the raw data to userspace through the files I mentioned so that some other service like pcid (which manages the pcie and pci bus ennumeration and running the drivers to manage devices) can use that information.

I couldn't have stated our needs better myself, so I just use your words:

Providing an interface that basically gives you a list of raw tables, which you could in theory expose to userspace using a VFS / other system

Yes, this would be pretty great to have on the kernel side. It looks like your lib is capable of finding many more of tables than our implementation currently. Also, because our kernel does have access to physical member, this might be easier to adapt to the library's method of providing essentially memory addresses. I would like to be able to expose all the tables that linux exposes in /sys/firmware/acpi/tables/.

Using the types in acpi to parse data that userspace been told is a raw table.

This is the part that I would like for the pcid work that I am doing.

In terms of planning, what are you looking for?

[wt]

To be more clear, our plan is to parse as little as possible in the kernel. In fact, doing some research, it looks like l4re works without initializing acpi in-kernel (caveat, I haven't looked at the code). But redox has a very similar goal to do as little ACPI handling in kernel as possible.

[jackpot51]

In Redox, I believe that mapping of physical addresses will be done by the same userspace process that parses the tables. This means that the current model of this crate will not need to change.

[wt]

I did manage to get the kernel to compile with the acpi. It a ways from replacing anything, but that is a start.

[IsaacWoods]

You're right that the PhysicalMapping interface would allow you to use the library from userspace (acpi only actually cares about the virtual address, the other fields are only provided for nice bookeeping for the library user), but I don't see how the library's current form would allow you to split the parsing of different tables between kernel and userspace.

I think the minimal change that would allow this would be to split the library into effectively two "layers": one that parses the RSDP, then the RSDT, to create a sort of SdtIter, which contains a list of signatures and the corresponding physical addresses. The second would then take this iterator and use it to do what we currently do: amalgamate all the tables into the one Acpi structure. Redox could then consume the SdtIter in a different way.

I think this approach has a few disadvantages:

• It is a large breaking change to the public API
• It complicates the library for most usecases ("Please parse all the static tables. I don't care what a MADT is, I just want to set up the local APIC"), to provide for an (I think) unusual usecase
• It sets up a footgun around ACPI-reclaimable memory but making it more confusing about when the ACPI tables become unneeded. At the moment, once you have a Acpi structure, you can reuse that physical memory. Redox would either have to pass a signal back from userspace somehow that it'd finished with the tables, or just never get to reclaim that physical memory.
• It also complicates the SDT dispatching code. I'm not yet sure how this could be made to work.

I also wonder if Redox will end up parsing most of the static info in-kernel anyways. As I'm sure you're aware, you need this info to robustly set up, at least:

• External interrupt control
• The HPET or local APIC timers (are you really going to bootstrap userspace off the PIT?)
• Bringup of application processors
• Respect for NUMA in e.g. your physical memory manager
• Power and thermal management

So far, it sounds like the main things you want in userspace are 1) PCI handling and 2) AML parsing. Has the option of parsing the MCFG in kernel, and passing out a scheme of PCI device information been considered (this could also help with making PCI management on the userspace side platform-agnostic)? Another scheme could present the bytecode of the AML tables as files, which could then be parsed in userspace.

FWIW, I also don't really understand the safety concerns of parsing the static tables in kernelspace. AML is a different beast, but the tables we're talking about are literally either just structures, or a dynamic number of entries. If Redox wants to donate person-hours to the existing Rust ACPI ecosystem, I believe it would benefit more from careful audit and fuzzing of a kernelspace implementation than trying to move it out to userspace.

[wt]

You're right, Redox does currently do a lot of parsing in the kernel. I think that will change over time given Jeremy's and my interest in changing that. I don't know if we'll take it as far as you describe or find some middle ground.

I also appreciate your ideas on how to most help the community, and I will give them thought when coming up with plans. However, I am not convinced Redox currently headed in a wrong direction at this point. We're trying to make practical decisions, and I would like to find a way to incorporate acpi and centralize some logic there, if possible, so that we can both benefit.

With regard to splitting the lib into two parts, I think there is value there. Parsing the table data with pcid in use space is valuable instead of developing some other serialization to export it from the kernel and then read from user space. Exporting the table data directly will enable using other acpi related tools with them. Even Linux makes the acpi table data directly available in sysfs. There are told that can work with that data that I want to have access to on Redox as well.

And to be clear, today the kernel does provide the table bytes for the mcfg directly to use space for the pcid. Pcid will serve the scheme of pci devices information your taking about, not the kernel.

What I want today is a library that isn't so picky about where we choose to draw the line. I think a separation of concerns would help there.

Thanks for working with me on this, and helping me figure it out.

[wt]

I have a question about the Physical mapping structure. Would it be possible to document what exactly the different field should be set to and maybe and example an example in the docs.

My interpretation is the following:

• physical_start: the actual start of the physical page
• virtual_start: a pointer to directly what was referenced
• region_length: the length of memory actually mapped, so (# pages * page size)
• mapped_length: unused AFAICT, so I set it to same as region length

So, if I ask for pointer to a u16 at 0x40e, I would get map the memory and return the following:

• physical_start: 0
• virtual_start: NonNull::new_unchecked(0x40e)
• region_length: 4096 (assuming 4k page size)
• mapped_length: 4096

Does that look correct?

[IsaacWoods]

What I want today is a library that isn't so picky about where we choose to draw the line. I think a separation of concerns would help there.

Hm fair enough, I've thought about this a bit more and I think we can make this work. It does however, require a pretty much complete overhaul of the public API we present, and so I'd like to move fairly slowly on this and make sure we get it right.

Would it be possible to document what exactly the different field should be set to and maybe and example an example in the docs.

If we're making a new major version, I think we can rework how this works / the field names to make it more obvious. When I wrote it, I didn't think it was particularly confusing, but there have been a fair number of people who have found it difficult to understand what acpi wants. Adding some extra documentation would also be a good idea.

So PhysicalMapping is effectively an "owned" mapping created by the library to a piece of physical memory. The only field required for correctness is virtual_start, but the others are provided as information the kernel might want to track to help it unmap the mapping later (this is nice in a limited environment such as a bootloader, because you don't have to track the mappings acpi makes yourself).

I tried to consider two options when designing the structure:

• You actually have to map the physical address to an effectively arbitrary, free, virtual address. In this case, the returned structure for your example might be:

    physical_start: 0x40e,  // This is just a copy of the physical address asked for. It's mainly useful for diagnostics.
    virtual_start: 0xffff_ffff_9420_040e,  // This is the virtual address that `acpi` will access the structure at, padded to the right amount. `0xffff_ffff_9420_0000` was selected as a free page in the kernel virtual address space, maybe by a dynamic allocator or something.
    region_length: 2,  // Region length is the actual length of the thing that was mapped (e.g. the u16)
    mapped_length: 4096,  // This is the actual mapped length. Provided so you know how to much to unmap later.

• You already have a full physical mapping. Lots of kernels like to just keep the full physical address space mapped at some high virtual address (for this example, 0xffff_ff80_0000_0000). In this case, there's no need to unmap, so we can simply do something like this:

    physical_start: 0x40e,
    virtual_start: 0xffff_ff80_0000_0000 + 0x40e,
    region_length: 2,
    mapped_length: 2, // We can access physical memory at the byte granularity, so we can set this to 2 as well

I hope that helps, please ask if you have any more questions :)

[wt]

Okay, so I have been playing with the long enough that I have a few questions.

1. The search_for_rsdp_bios function appears to go to some effort to only map 4k at time. Redox's ACPI implementation just maps the whole search space and scans. Is there some disadvantage to mapping the whole space? I actually rewrote the logic mapping each range entirely. The logic is a lot simpler in my opinion.
2. Why does it try to map memory that is already mapped in the search_for_rsdp_bios? In fact, it appears to do this in many places. Redox does not like to map the same memory twice. Also, I'm not sure I like that design.
3. I see that there is asymmetry in map/unmap operation, like where memory is allocated for the RSDT in search_for_rsdp_bios but released in the function is calls, which is parse_validated_rsdp.
4. Why is the map_physical_region method of the acpi::handler::AcpiHandler trait marked as unsafe. Shouldn't implementations of the trait make sure that's safe?

[Restioson]

I've been very inactive here for a while (sorry!) but I wrote the search_for_rsdp_bios function a long time ago :) It was taken and modified from here.

On 1: I don't think there was a particular reason why I wrote it like this. It was quite some time ago. It was probably simpler for some or other reason at the time in the original context (maybe because you don't have to find a large free virtual memory area).
On 2, I'm not sure what you mean exactly.

[IsaacWoods]

@Restioson no worries, hope you've been well :)

1

+1 to what Restioson said - it's a compromise between how large of a contiguous virtual range we ask the kernel to map and the overhead of paging. I can understand why if those calls turn out as being system calls or whatever in Redox, you'd want to map the entire range, so I wouldn't be aversed to that.

A PR changing to mapping each range in its entirity before searching through it would likely be accepted. This could be changed in the current system as we can keep the same algorithm if it's moved into some sort of "collection of SDTs" API in the future.

2

The case in search_for_rsdp_bios is special and slightly nuanced: if we find the RSDP's signature but the rest of it turns out not to be a valid table, we continue searching (this is done here). Actually this doesn't unmap rsdp_mapping in the failed case - that is a genuine bug we managed to miss! However, this path should be very uncommon, so the majority of the time, it will only construct a single rsdp_mapping which is passed on into parse_validated_rsdp.

Redox does not like to map the same memory twice. Also, I'm not sure I like that design.

This sounds like a limitation of Redox's paging code, unless I'm misunderstanding. We don't create extra work for the library consumer for no reason, so there's usually good reasons for jumping through these hoops. The two cases I can think of are:

• The rsdp_mapping thing discussed above. This is a nice safe way of detecting the case of a correct signature but invalid RSDP. If you want to rewrite this to use raw pointer arithmetic to create and validate a Rsdp instance without constructing a second mapping, I'd accept that PR.
• Peeking of SDT signatures - this creates a mapping to peek at the signature and length of a SDT before creating its real mapping. As we don't know how large a particular SDT is before we see its header, this is a cleanest way I can see of making the library user's life easy. Imagine for example if an SDT straddled a page boundary - we map the header, see it's x bytes long and try and access it, and read from unmapped memory because the user has only mapped a single page (just an example, you get my point hopefully).

3

yeah it can be confusing, as demonstrated by the bug I found while writing this post ;) One change I've been tempted by for a while would be to store a AcpiHandler in every PhysicalMapping, so we could call unmap_physical_region when it's dropped. This would require AcpiHandler's methods to take &self, and so users would have to manage state using interior mutability, but it could be a change worth considering to avoid having to remember to call unmap yourself.

The actual reason that they're sometimes asymmetrical is that we pass them around as "owned" pieces of physical memory. If we didn't do this and always created and unmapped every mapping at each step, then there would be a bunch more duplicated mappings.

4

There was a lot of discussion at the time about whether it suited it more for it to be safe or unsafe, and who's responsibility it was to request "safe" physical addresses. The nice thing about it being unsafe is that it makes it unsafe to map arbitrary physical addresses inside the library, which nicely highlights places we could make an addressing mistake (it is unsafe to create an invalid mapping, as it is safe to deref the resulting virtual address from inside acpi). I agree it might seem weird from outside the library though.

[IsaacWoods]

I think the changes made in #75 should allow Redox to use our crates. Please let us know how it goes, and reopen this issue / open new ones for any further queries :)