Re: [PATCH v5 2/2] PCI: s390: Support mmap() of BARs and replace VFIO_PCI_MMAP by a device flag

[Niklas Schnelle <schnelle@linux.ibm.com>]

On Wed, 2025-02-12 at 13:28 -0700, Alex Williamson wrote:
> On Wed, 12 Feb 2025 16:28:32 +0100
> Niklas Schnelle <schnelle@linux.ibm.com> wrote:
> 
> > On s390 there is a virtual PCI device called ISM which has a few
> > peculiarities. For one, it presents a 256 TiB PCI BAR whose size leads
> > to any attempt to ioremap() the whole BAR failing. This is problematic
> > since mapping the whole BAR is the default behavior of for example
> > vfio-pci in combination with QEMU and VFIO_PCI_MMAP enabled.
> > 
> > Even if one tried to map this BAR only partially, the mapping would not
> > be usable without extra precautions on systems with MIO support enabled.
> > This is because of another oddity, in that this virtual PCI device does
> > not support the newer memory I/O (MIO) PCI instructions and legacy PCI
> > instructions are not accessible through writeq()/readq() when MIO is in
> > use.
> > 
> > In short the ISM device's BAR is not accessible through memory mappings.
> > Indicate this by introducing a new non_mappable_bars flag for the ISM
> > device and set it using a PCI quirk. Use this flag instead of the
> > VFIO_PCI_MMAP Kconfig option to block mapping with vfio-pci. This was
> > the only use of the Kconfig option so remove it. Note that there are no
> > PCI resource sysfs files on s390x already as HAVE_PCI_MMAP is currently
> > not set. If this were to be set in the future pdev->non_mappable_bars
> > can be used to prevent unusable resource files for ISM from being
> > created.
> 
> I think we should also look at it from the opposite side, not just
> s390x maybe adding HAVE_PCI_MMAP in the future, but the fact that we're
> currently adding a generic PCI device flag which isn't honored by the
> one mechanism that PCI core provides to mmap MMIO BARs to userspace.
> It seems easier to implement it in pci_mmap_resource() now rather than
> someone later discovering there's no enforcement outside of the very
> narrow s390x use case.  Thanks,
> 
> Alex

That is a very good point! I did try enabling HAVE_PCI_MMAP for s390 a
while back and I believe that ran into trouble with ISM devices too. So
I just did a quick test of enabling HAVE_PCI_MMAP with
ARCH_GENERIC_PCI_MMAP_RESOURCE for s390. Then added a check for 
pdev->non_mappable_bars to pci_create_resource_files() and
proc_bus_pci_mmap(). I pondered adding it to pci_mmap_resource() too
but felt like not showing the resource at all, like we do now with
!HAVE_PCI_MMAP is cleaner.

Using a little test program that just mmap()s BAR 0 of an NVMe and
reads the NVMe version at offset 8 using our PCI MIO load instruction
works. Also, as expected I don't get resourceX files for ISM devices
with the added check. 

I still have to test the /proc/bus/pci based mmap() but would expect
that to work too. So I'd be open to adding another patch which adds
HAVE_PCI_MMAP for s390, if we see too much risk with that, we could
alternatively add just the pdev->non_mappable_bars but then they would
be untested, still better than hoping that someone remembers to add
that in the future.

Thanks,
Niklas