WIP of BUS_MAP_RESOURCE.

[jhb]

Summary

This is a WIP (not yet a commit candidate, and if it does become one it
will likely be split up a bit with MI bits separate from x86, etc.).

I decided to use a helper struct for optional attributes of a mapping
ala kib's recent args for make_dev_s(). This also includes an
implementation of RF_UNMAPPED as its implementation becomes fairly
trivial as each bus driver is fixed to move tag/handle setup/teardown
out into the new methods.

This has most of the implementation for x86 (but untested). A problem
with the x86 bits is that there's no easy way to get the size of a
memory mapping in the non-PC98 case, so the size passed to pmap_unmapdev
is wrong. We could fix this by changing the x86 handle to be a struct
that includes the pointer and length instead of just being the pointer.
That hoses the ABI though.

Some other options are to require the mapping args passed to map_resource()
be passed to unmap_resource(), or to define a new MD type that is a
"mapping request" and contains (at minimum) the tag/handle. You could
define this 'struct resource_mapping' with the same names as
'struct resource' for the tag/handle so you can use the shorter
bus_read_X(), etc. macros with it which might be nice.

[cem]

sys/kern/bus_if.m

349–351 ↗

(On Diff #13162)

I think we should be explicit about whether or not this will unmap as part of deactivating the resource (it should tear down any mappings as part of deactivation, right?).

sys/sys/bus.h

292 ↗

(On Diff #13162)

sizeof() yields size_t. Might make 'size' unsigned, at least, if not just size_t. (Yes, this tiny struct will never need to be >4GB or even >2GB, this is just pedanticism.)

sys/x86/x86/nexus.c

464–465 ↗

(On Diff #13162)

Surely it's at least uintptr_t?

547 ↗

(On Diff #13162)

same

573–576 ↗

(On Diff #13162)

Would it make sense to, instead:

(1) Disallow mapping subsets of a resource
(2) Allow creating sub-resources, and
(3) Just map those?

I guess "not really."

Other options:
(1) Break the ABI and make non-PC98 bus_space_handle_t a struct, as you suggest.
(2) Keep a set of active mappings (start, len pairs) tracked in 'struct resource' itself (disallowing overlapping mappings).

I'm fine with (1) if that's most straightforward.

[jhb]

sys/kern/bus_if.m

349–351 ↗

(On Diff #13162)

Well, that's a harder problem to solve (but I do wish to solve it eventually). In particular, I eventually want to store a list of all of the mappings of a given resource in the 'struct resource' and have deactivate resource remove all of those mappings (not just the implied mapping). However, that needs additional changes. I think this will need to go into the tree in stages and even if bus_deactivate_resource() is fixed in this manner, it will be more to clean up after drivers that don't properly tear down mappings during detach (we already have some support for that in the PCI bus driver and this is an extension of those). However, deactivate does not currently do that, so I'm hesitant to guarantee that when it isn't true.

sys/sys/bus.h

292 ↗

(On Diff #13162)

Mmm, I was trying to model this on make_dev_args, but that does indeed use a size_t. Given that rman_res_t is soon to be uintmax_t the space would be there as padding anyway.

sys/x86/x86/nexus.c

464–465 ↗

(On Diff #13162)

The comments are copied and pasted from other places. It is uint64_t on amd64 and 'u_int' on i386.

573–576 ↗

(On Diff #13162)

No. In fact, we currently do create sub-resources behind PCI-PCI bridges and then pass those up to nexus and "hope" it can cope with activating a resource it didn't create. This is a bug, not a feature. One of the use cases I want to use this for is that when a PCI-PCI bridge wants to map a resource for a child device, it turns that into a request to map the sub-range of the resource it allocated from its parent for the I/O decoding window. This becomes more important when you consider the case of a 'Foo -> PCI' bridge that uses an address translation mechanism (e.g. if the PCI addresses all have a fixed offset added to them in the CPU-visible address space). (Note that in these cases the bridge would use RF_UNMAPPED when allocating its resources, right now they don't use RF_ACTIVE and we have a hack in the PCI-PCI bridge driver to ensure decoding is active (which RF_ACTIVE would normally do)).

Long term as I mentioned above I do plan on tracking active mappings in the resource to allow for cleaning up behind broken drivers, but I also want to make bus_unmap_resource() work, so I had not planned on using that for this. In particular, in the case of the PCI-PCI bridge above I planned on storing the mapping of a range of the parent resource in the mapping list of the sub-resource of the child device behind the bridge (so that it gets cleaned up when the child device detaches, not only when the bridge detaches (which is basically never)). OTOH, I suppose I could store it in both places (yuck), and I'd still need some sort of structure to hold additional fields (like the size) to hold the storage if I don't change the bus handle on x86.

[jhb]

Some general notes on uses cases for this interface (since Ravi asked):

1. Replace the bus_get_bus_tag() proposal that aims to have an MI OFW PCI bridge driver by having the driver push mapping requests up to the parent nexus instead of duplicating MD logic in the bridge driver but using a method to fetch an MD parameter.

2. Allow us to support bridges that perform address translation. (Mentioned in my earlier reply to cem@)

3. Allow us to remove some hacks from the NEW_PCIB PCI-PCI bridge driver. (Also mentioned earlier)

4. Provide a saner interface for using non-default mapping attributes for a resource (such as mapping a BAR WC instead of UC).

[cem]

sys/x86/x86/nexus.c

464–465 ↗

(On Diff #13162)

Right, I just mean we shouldn't keep an obviously wrong comment. Either kill it or s/u_int/uintptr_t/.

[imp]

sys/kern/bus_if.m

349–351 ↗

(On Diff #13162)

There's also issues on non-coherent architectures with aliasing multiple virtual pages to the same physical page. But on those, multiple PA -> VA mappings are prohibited. However, different PAs within the range could still be mapped if the resource was large enough.

I'd modify the note: "While this may unmap the memory region from the kernel's virtual address space, it is not currently guaranteed to do so." and then change it into the future to either "This will unmap all mappings" or "This will not unmap
all mappings" or something more specific. Just in case our plans go awry between here and your final vision.

sys/kern/subr_bus.c

4036 ↗

(On Diff #13162)

Do we want to make this and the unmap one the default value for BUS_MAP_RESOURCE and BUS_UNMAP_RESOURCE?

4379–4409 ↗

(On Diff #13162)

Except for comments, these are identical to the generic versions. Why not implement one in terms of the other?

sys/x86/x86/nexus.c

464–465 ↗

(On Diff #13162)

At some point, we should update the IBM-PC references to something more modern. But clearly beyond the scope of these changes.

[jhb]

sys/kern/bus_if.m

349–351 ↗

(On Diff #13162)

For those architectures the onus is on pmap_mapdev() to return the same mapping I think, though that caching could also be done in the nexus in bus_map_resource if needed. In that case it would need to keep a reference count or some such for "duplicate" requests and just drop a reference on unmap. In that sense, this method would always "unmap" any mappings created via bus_map_resource() (including the implicit mapping for !RF_UNMAPPED)). However, those mappings are distinct from not having any mapping at all. (e.g. on amd64 all of these mappings come from the direct map when possible and those are never unmapped either in the pmap sense, just in the bus sense.)

[jhb]

sys/kern/subr_bus.c

4036 ↗

(On Diff #13162)

Ugh, I might have to. It's actually hackish. What I want to do (long term) is have a 'bus_generic' driver that bus drivers can inherit from that uses all the bus_generic methods. I think that is cleaner than having non-bus devices implement this method (which is what the default does)

4379–4409 ↗

(On Diff #13162)

1. This is the pattern of all the other bus methods.

2. They are not identical. These are wrappers that only accept the child device and lookup the child's parent. The other methods pass the request to the parent of the bus (not the parent of the child).

[cem]

sys/x86/x86/nexus.c

571–574 ↗

(On Diff #13162)

What about adding in KASSERT(vaddr == rman_get_start(r), ...) just to get this API in? (With the obvious limitation that some architectures (amd64, i386-non-PC98) can't map subranges of a BAR. The assert can even go in nexus_map_resource().)

This isn't a regression from today. The future change to track active mappings can enable that use of the API without breaking ABI.

[jhb]

• Add a struct resource_map object to describe a mapping.
• Stick with r_vaddr.
  
  This structure serves a couple of purposes. First, it holds the tag and handle used to actually access the mapping of a resource. However, I've named the structure so that it matches 'struct resource' and can be used with the 'bus_read_X()', etc. helper macros. Second, I've saved the size of the mapping for later use in unmap_resource (which fixes the issue I had with x86). bus_space_unmap() requires the caller to pass in the size and does not expect to be able to infer the size from the tag and handle, so this seems consistent. Finally, now that the mapping is described by an object, we can actually choose to continue supporting direct virtual addresses if we wish by having a 'r_vaddr' that is equivalent to rman_get_virtual().

[jhb]

I decided to go with adding a 'struct reource_map' to hold a mapping instead of a bare tag/handle. This lets you use the convenience API (e.g. bus_read_X), and it lets me stick the 'size' somewhere to fix the issue I ran into on x86.

We may find that we want to add some helpers for some of the logic in x86. One I can think of is something to generate a mapping from the implicit mapping in 'struct resource'. However, we can do that as we start implementing this interface on other platforms. However, I think I like the API now.

[jhb]

• Rebase.
• Document RF_UNMAPPED and cross-reference bus_map_resource.9.
• Add a manpage for bus_map_resource().

[jhb]

• Add rman_get/set_mapping wrappers.
• Compile.
• Compile.
• Fix various issues to get further in boot on x86.
• Fail mapping requests for unsupported resource types.

[jhb]

• Document rman_get/set_mapping and add Xrefs to rman.9.

[cem]

sys/x86/x86/nexus.c

450 ↗

(On Diff #16606)

(rman_get_flags(r) & RF_UNMAPPED) == 0 &&

474 ↗

(On Diff #16606)

same style nit

[jhb]

sys/x86/x86/nexus.c

450 ↗

(On Diff #16606)

I think we've discussed this before, but testing flags in a bitmask can use a bare '&' without '== 0' in style(9) and is fairly common in the kernel though there is a bit of a mix. (Multi-bit fields must always use an explicit == against a desired value)

[cem]

sys/x86/x86/nexus.c

450 ↗

(On Diff #16606)

Huh. In FreeBSD, I've always been told to compare to zero, even for single bit flags. Not a big deal.