Mmap device BAR into userspace.

[kib]

Summary

Add the ioctl PCIOCBARMMAP to conveniently create userspace mapping of a PCI device BAR. Add sample use of it to pciconf. This is enormously superior to read the BAR value with PCIOCREAD and then try to mmap /dev/mem.

Issues to discuss:

1. Is the use of pci_map.pm_value correct ? I am not sure, but it seems to contain the copy of the BAR value, then don't we need to mask out type low bits ? (Seems to work fine).
2. If the BAR is not activated, do we want to activate it ? I am not sure how to do it, esp. in a way which would not conflict with a driver attach.
3. Right now, for each user request to mmap, new sg pager is created. Instead, I can store the managed device pager in pci_map. Advantages would be that on the bar deactivation, I can revoke all mappings. But it is not clear to me how to protect the pointer.

TODO:

1. Non-amd64 pmap_is_valid_memattr()
2. Allow to specify memattr for pciconf(8).
3. Man pages, will follow after the code is finalized.

[imp]

Generally, I like this. It mirrors efforts to have userland drivers. The only area I see missing after this is interrupt delivery to userland processes.

sys/amd64/amd64/pmap.c

1487 ↗

(On Diff #43036)

Like this change. I'd be tempted to commit it separately

sys/dev/pci/pci_user.c

728 ↗

(On Diff #43036)

What's to prevent concurrent access with other drivers?

732 ↗

(On Diff #43036)

I'm not sure this is quite the right thing to do here. This just says to check the enabled bits for io or memory based on the type of BAR it is... In a normal device's resource activation, we turn on these bits. I'd likely turn on the mem bit in the config register if PCIIO_BAR_MMAP_ACTIVATE is set, but I'd be even more likely to turn it on just always.

I'd also be tempted to filter PCIR_IS_BIOS bars as well.

sys/sys/pciio.h

146 ↗

(On Diff #43036)

Should this be vm_offset_t?

usr.sbin/pciconf/pciconf.c

37 ↗

(On Diff #43036)

I'd be tempted to commit these bits separately as well.

[kib]

sys/amd64/amd64/pmap.c

1487 ↗

(On Diff #43036)

This change requires more work to implement pmap_is_valid_memattr() on all arches. Until the pci bar mapping patch is finished, it is premature to spend time on it.

After that, it will be a separate commit, certainly.

sys/dev/pci/pci_user.c

728 ↗

(On Diff #43036)

I am not sure what do you mean there. What other parallel action is undesirable there ?

Note that I listed in the questions the driver behavior WRT pci device disappearing, but I do not think that this was your question.

732 ↗

(On Diff #43036)

This was another question. Do we always allocate resources for BARs, even for devices which do not have the driver attached ? How attaching a driver later and attempting to activate the BAR would interact with this ?

sys/sys/pciio.h

146 ↗

(On Diff #43036)

No, this value is small integer < PAGE_SIZE. vm_offset_t would make this structure non-invariant for ILP32/LP64 and require ABI translation for compat32.

usr.sbin/pciconf/pciconf.c

37 ↗

(On Diff #43036)

Sure, after the whole patch is finalized.

[jhb]

I would perhaps let dumpbar take a start offset and length relative to the BAR of what to dump similar to the args to -r.

[imp]

this got lost in my todo inbox. sorry for my delay.

sys/dev/pci/pci_user.c

728 ↗

(On Diff #43036)

My original worry was an in-kernel driver racing against this operation if somehow a driver was loaded.

But the other worry is what happens if there's user programs run, perhaps accidentally, at the same time.

732 ↗

(On Diff #43036)

Yes and no. If the BIOS allocates things, we mostly honor those allocations. If not, we do a lazy assignment of resources. Some devices come up with the IO/MEM bits in their command register set, others require the driver to set them. If this code is executed in cooperation with some driver, then this will be a good test. If not, then it's hit or miss depending on the specific hardware, I believe. My knowledge here may be a bit dated as I've seen this mostly on pre-pcie cards.

[kib]

Add optional start:count for pciconf -D.
Update pciio(4) and pciconf(8) man pages.

[kib]

sys/dev/pci/pci_user.c

728 ↗

(On Diff #43036)

IMO it is up to user/app/driver to coordinate.

My own use case for this functionality was driver-less bitstream update for some PCIe card with FPGA, where the bits must be uploaded before real driver can attach.

732 ↗

(On Diff #43036)

I and jhb discussed this, it seems that the completely correct approach is to make BAR activation from mapping transparent to the device driver, so that bus_activate_resource() does not fail if the BAR is already active due to mapping. Also, deactivation of resources should not result in BAR deactivation while mmapings still exist. I.e. the resource should be refcounted.

I also think that the vm object should be stored in pci_map structure.

Anyway, this functionality is completely missing from the patch, and there is an opinion that just providing the ioctl is a start.

[kib]

Ping ?

FYI, I am going to make the pmap change for all arches and want to commit the ioctl(2) + pciconf(8) ASAP.

[kib]

Implement pmap_is_valid_memattr(9) for all arches (tinderbox is just started).
Stop pretending that uint64_t solves the void * marshalling for compat32, it does not for big-endian machines. Provide real compat32.

[kib]

Make MIPS machine/vm.h usable.
Fix gcc 32bit arches.

[kib]

Fix libkvm build after mips.

This patch passed tinderbox.

[alc]

sys/vm/pmap.h

145 ↗

(On Diff #44345)

"int" -> "vm_memattr_t"

[kib]

Change type of the mode argument for pmap_is_valid_memattr() to vm_memattr_t.