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?).

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.)

Surely it's at least uintptr_t?

same

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.

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.

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.

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

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.

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

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.

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

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

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

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.)

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)

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).

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)

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