[PATCH v4 0/4] vfio/pci: s390: Fix issues preventing VFIO_PCI_MMAP=y for s390 and enable it

[PATCH v4 0/4] vfio/pci: s390: Fix issues preventing VFIO_PCI_MMAP=y for s390 and enable it

[Niklas Schnelle]

With the introduction of memory I/O (MIO) instructions enbaled in commit
71ba41c9b1d9 ("s390/pci: provide support for MIO instructions") s390
gained support for direct user-space access to mapped PCI resources.
Even without those however user-space can access mapped PCI resources
via the s390 specific MMIO syscalls. There is thus nothing fundamentally
preventing s390 from supporting VFIO_PCI_MMAP allowing user-space drivers
to access PCI resources without going through the pread() interface.
To actually enable VFIO_PCI_MMAP a few issues need fixing however.

Firstly the s390 MMIO syscalls do not cause a page fault when
follow_pte() fails due to the page not being present. This breaks
vfio-pci's mmap() handling which lazily maps on first access.

Secondly on s390 there is a virtual PCI device called ISM which has
a few oddities. For one it claims to have a 256 TiB PCI BAR (not a typo)
which leads to any attempt to mmap() it fail with the following message:

    vmap allocation for size 281474976714752 failed: use vmalloc=<size> to increase size

Even if one tried to map this BAR only partially the mapping would not
be usable on systems with MIO support enabled. So just block mapping
BARs which don't fit between IOREMAP_START and IOREMAP_END.

Note:
For your convenience the code is also available in the tagged
b4/vfio_pci_mmap branch on my git.kernel.org site below:
https: //git.kernel.org/pub/scm/linux/kernel/git/niks/linux.git/

Thanks,
Niklas

Link: https://lore.kernel.org/all/c5ba134a1d4f4465b5956027e6a4ea6f6beff969.camel@linux.ibm.com/
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
Changes in v4:
- Overhauled and split up patch 2 which caused errors on ppc due to
  unexported __kernel_io_end. Replaced it with a minimal s390 PCI fixup
  harness to set pdev->non_compliant_bars for ISM plus ignoring devices
  with this flag in vfio-pci. Idea for using PCI quirks came from
  Christoph Hellwig, thanks. Dropped R-bs for patch 2 accordingly.
- Rebased on v6.10-rc5 which includes the vfio-pci mmap fault handler
  fix to the issue I stumbled over independently in v3
- Link to v3: https://lore.kernel.org/r/20240529-vfio_pci_mmap-v3-0-cd217d019218@linux.ibm.com

Changes in v3:
- Rebased on v6.10-rc1 requiring change to follow_pte() call
- Use current->mm for fixup_user_fault() as seems more common
- Collected new trailers
- Link to v2: https://lore.kernel.org/r/20240523-vfio_pci_mmap-v2-0-0dc6c139a4f1@linux.ibm.com

Changes in v2:
- Changed last patch to remove VFIO_PCI_MMAP instead of just enabling it
  for s390 as it is unconditionally true with s390 supporting PCI resource mmap() (Jason)
- Collected R-bs from Jason
- Link to v1: https://lore.kernel.org/r/20240521-vfio_pci_mmap-v1-0-2f6315e0054e@linux.ibm.com

---
Niklas Schnelle (4):
      s390/pci: Fix s390_mmio_read/write syscall page fault handling
      s390/pci: Add quirk support and set pdev-non_compliant_bars for ISM devices
      vfio/pci: Disable mmap() non-compliant BARs
      vfio/pci: Enable PCI resource mmap() on s390 and remove VFIO_PCI_MMAP

 arch/s390/Kconfig                |  4 +---
 arch/s390/pci/Makefile           |  2 +-
 arch/s390/pci/pci_fixup.c        | 23 +++++++++++++++++++++++
 arch/s390/pci/pci_mmio.c         | 18 +++++++++++++-----
 drivers/s390/net/ism_drv.c       |  1 -
 drivers/vfio/pci/Kconfig         |  4 ----
 drivers/vfio/pci/vfio_pci_core.c |  8 ++------
 include/linux/pci_ids.h          |  1 +
 8 files changed, 41 insertions(+), 20 deletions(-)
---
base-commit: f2661062f16b2de5d7b6a5c42a9a5c96326b8454
change-id: 20240503-vfio_pci_mmap-1549e3d02ca7

Best regards,
-- 
Niklas Schnelle

[PATCH v4 1/4] s390/pci: Fix s390_mmio_read/write syscall page fault handling

[Niklas Schnelle]

The s390 MMIO syscalls when using the classic PCI instructions do not
cause a page fault when follow_pte() fails due to the page not being
present. Besides being a general deficiency this breaks vfio-pci's mmap()
handling once VFIO_PCI_MMAP gets enabled as this lazily maps on first
access. Fix this by following a failed follow_pte() with
fixup_user_page() and retrying the follow_pte().

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
 arch/s390/pci/pci_mmio.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/arch/s390/pci/pci_mmio.c b/arch/s390/pci/pci_mmio.c
index 5398729bfe1b..80c21b1a101c 100644
--- a/arch/s390/pci/pci_mmio.c
+++ b/arch/s390/pci/pci_mmio.c
@@ -170,8 +170,12 @@ SYSCALL_DEFINE3(s390_pci_mmio_write, unsigned long, mmio_addr,
 		goto out_unlock_mmap;
 
 	ret = follow_pte(vma, mmio_addr, &ptep, &ptl);
-	if (ret)
-		goto out_unlock_mmap;
+	if (ret) {
+		fixup_user_fault(current->mm, mmio_addr, FAULT_FLAG_WRITE, NULL);
+		ret = follow_pte(vma, mmio_addr, &ptep, &ptl);
+		if (ret)
+			goto out_unlock_mmap;
+	}
 
 	io_addr = (void __iomem *)((pte_pfn(*ptep) << PAGE_SHIFT) |
 			(mmio_addr & ~PAGE_MASK));
@@ -305,12 +309,16 @@ SYSCALL_DEFINE3(s390_pci_mmio_read, unsigned long, mmio_addr,
 	if (!(vma->vm_flags & (VM_IO | VM_PFNMAP)))
 		goto out_unlock_mmap;
 	ret = -EACCES;
-	if (!(vma->vm_flags & VM_WRITE))
+	if (!(vma->vm_flags & VM_READ))
 		goto out_unlock_mmap;
 
 	ret = follow_pte(vma, mmio_addr, &ptep, &ptl);
-	if (ret)
-		goto out_unlock_mmap;
+	if (ret) {
+		fixup_user_fault(current->mm, mmio_addr, 0, NULL);
+		ret = follow_pte(vma, mmio_addr, &ptep, &ptl);
+		if (ret)
+			goto out_unlock_mmap;
+	}
 
 	io_addr = (void __iomem *)((pte_pfn(*ptep) << PAGE_SHIFT) |
 			(mmio_addr & ~PAGE_MASK));

-- 
2.40.1

[PATCH v4 2/4] s390/pci: Add quirk support and set pdev-non_compliant_bars for ISM devices

[Niklas Schnelle]

On s390 there is a virtual PCI device called ISM which has a few
pecularities. For one it claims to have a 256 TiB PCI BAR which leads to
any attempt to ioremap() in its entirety failing. This is problematic
since mapping the whole BAR is the default behavior of for example QEMU
with VFIO_PCI_MMAP enabled.

Even if one tried to map this BAR only partially the mapping would not
be usable on systems with MIO support enabled unless using the function
handle based PCI instructions directly. 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() or by user-space when MIO is in use.

Indicate that ISM's BAR is special and does not conform to PCI BAR
expectations by setting pdev->non_compliant_bars such that drivers not
specifically developed for ISM can easily ignore it. To this end add
basic PCI quirks support modeled after x86's arch/x86/pci/fixup.c and
move the ISM device's PCI ID to the common header to make it accessible.
Also enable CONFIG_PCI_QUIRKS whenever CONFIG_PCI is enabled.

Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
 arch/s390/Kconfig          |  4 +---
 arch/s390/pci/Makefile     |  2 +-
 arch/s390/pci/pci_fixup.c  | 23 +++++++++++++++++++++++
 drivers/s390/net/ism_drv.c |  1 -
 include/linux/pci_ids.h    |  1 +
 5 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index c59d2b54df49..8332ba71465e 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -41,9 +41,6 @@ config AUDIT_ARCH
 config NO_IOPORT_MAP
 	def_bool y
 
-config PCI_QUIRKS
-	def_bool n
-
 config ARCH_SUPPORTS_UPROBES
 	def_bool y
 
@@ -234,6 +231,7 @@ config S390
 	select PCI_DOMAINS		if PCI
 	select PCI_MSI			if PCI
 	select PCI_MSI_ARCH_FALLBACKS	if PCI_MSI
+	select PCI_QUIRKS		if PCI
 	select SPARSE_IRQ
 	select SWIOTLB
 	select SYSCTL_EXCEPTION_TRACE
diff --git a/arch/s390/pci/Makefile b/arch/s390/pci/Makefile
index 0547a10406e7..03d96c76c21a 100644
--- a/arch/s390/pci/Makefile
+++ b/arch/s390/pci/Makefile
@@ -5,5 +5,5 @@
 
 obj-$(CONFIG_PCI)	+= pci.o pci_irq.o pci_clp.o pci_sysfs.o \
 			   pci_event.o pci_debug.o pci_insn.o pci_mmio.o \
-			   pci_bus.o pci_kvm_hook.o
+			   pci_bus.o pci_kvm_hook.o pci_fixup.o
 obj-$(CONFIG_PCI_IOV)	+= pci_iov.o
diff --git a/arch/s390/pci/pci_fixup.c b/arch/s390/pci/pci_fixup.c
new file mode 100644
index 000000000000..61045348f20a
--- /dev/null
+++ b/arch/s390/pci/pci_fixup.c
@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Exceptions for specific devices,
+ *
+ * Copyright IBM Corp. 2024
+ *
+ * Author(s):
+ *   Niklas Schnelle <schnelle@linux.ibm.com>
+ */
+#include <linux/pci.h>
+
+static void zpci_ism_bar_no_mmap(struct pci_dev *pdev)
+{
+	/*
+	 * ISM's BAR is special. Drivers written for ISM know
+	 * how to handle this but others need to be aware of their
+	 * special nature e.g. to prevent attempts to mmap() it.
+	 */
+	pdev->non_compliant_bars = 1;
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_IBM,
+			PCI_DEVICE_ID_IBM_ISM,
+			zpci_ism_bar_no_mmap);
diff --git a/drivers/s390/net/ism_drv.c b/drivers/s390/net/ism_drv.c
index e36e3ea165d3..d32633ed9fa8 100644
--- a/drivers/s390/net/ism_drv.c
+++ b/drivers/s390/net/ism_drv.c
@@ -20,7 +20,6 @@
 MODULE_DESCRIPTION("ISM driver for s390");
 MODULE_LICENSE("GPL");
 
-#define PCI_DEVICE_ID_IBM_ISM 0x04ED
 #define DRV_NAME "ism"
 
 static const struct pci_device_id ism_device_table[] = {
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 942a587bb97e..d730fce2113f 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -517,6 +517,7 @@
 #define PCI_DEVICE_ID_IBM_ICOM_V2_ONE_PORT_RVX_ONE_PORT_MDM	0x0251
 #define PCI_DEVICE_ID_IBM_ICOM_V2_ONE_PORT_RVX_ONE_PORT_MDM_PCIE 0x0361
 #define PCI_DEVICE_ID_IBM_ICOM_FOUR_PORT_MODEL	0x252
+#define PCI_DEVICE_ID_IBM_ISM		0x04ED
 
 #define PCI_SUBVENDOR_ID_IBM		0x1014
 #define PCI_SUBDEVICE_ID_IBM_SATURN_SERIAL_ONE_PORT	0x03d4

-- 
2.40.1

Re: [PATCH v4 2/4] s390/pci: Add quirk support and set pdev-non_compliant_bars for ISM devices

[Niklas Schnelle]

On Wed, 2024-06-26 at 13:15 +0200, Niklas Schnelle wrote:
> On s390 there is a virtual PCI device called ISM which has a few
> pecularities. For one it claims to have a 256 TiB PCI BAR which leads to
> any attempt to ioremap() in its entirety failing. This is problematic
> since mapping the whole BAR is the default behavior of for example QEMU
> with VFIO_PCI_MMAP enabled.
> 
> Even if one tried to map this BAR only partially the mapping would not
> be usable on systems with MIO support enabled unless using the function
> handle based PCI instructions directly. 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() or by user-space when MIO is in use.
> 
> Indicate that ISM's BAR is special and does not conform to PCI BAR
> expectations by setting pdev->non_compliant_bars such that drivers not
> specifically developed for ISM can easily ignore it. To this end add
> basic PCI quirks support modeled after x86's arch/x86/pci/fixup.c and
> move the ISM device's PCI ID to the common header to make it accessible.
> Also enable CONFIG_PCI_QUIRKS whenever CONFIG_PCI is enabled.
> 
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> ---
> 


Fixed the '-' instead of '->' typo in the patch subject locally.
Chances are we get a v5 anyway.

Thanks,
Niklas

Re: [PATCH v4 2/4] s390/pci: Add quirk support and set pdev-non_compliant_bars for ISM devices

[Alexandra Winter]

On 26.06.24 13:20, Niklas Schnelle wrote:
> On Wed, 2024-06-26 at 13:15 +0200, Niklas Schnelle wrote:
>> On s390 there is a virtual PCI device called ISM which has a few
>> pecularities. For one it claims to have a 256 TiB PCI BAR which leads to
>> any attempt to ioremap() in its entirety failing. This is problematic
>> since mapping the whole BAR is the default behavior of for example QEMU
>> with VFIO_PCI_MMAP enabled.
>>
>> Even if one tried to map this BAR only partially the mapping would not
>> be usable on systems with MIO support enabled unless using the function
>> handle based PCI instructions directly. 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() or by user-space when MIO is in use.
>>
>> Indicate that ISM's BAR is special and does not conform to PCI BAR
>> expectations by setting pdev->non_compliant_bars such that drivers not
>> specifically developed for ISM can easily ignore it. To this end add
>> basic PCI quirks support modeled after x86's arch/x86/pci/fixup.c and
>> move the ISM device's PCI ID to the common header to make it accessible.
>> Also enable CONFIG_PCI_QUIRKS whenever CONFIG_PCI is enabled.
>>
>> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
>> ---
>>
> 
> 
> Fixed the '-' instead of '->' typo in the patch subject locally.
> Chances are we get a v5 anyway.
> 
> Thanks,
> Niklas
> 

For a v5:
I am not an expert in English grammar, but I think some commas (',')
may help with reading your very good commit messages.

Acked-by: Alexandra Winter <wintera@linux.ibm.com>

[PATCH v4 3/4] vfio/pci: Disable mmap() non-compliant BARs

[Niklas Schnelle]

When VFIO_PCI_MMAP is enabled for s390 in a future commit and the ISM
device is passed-through to a KVM guest QEMU attempts to eagerly mmap()
its BAR. This fails because the 256 TiB large BAR does not fit in the
virtual map. Besides this issue mmap() of the ISM device's BAR is not
useful either as even a partial mapping won't be usable from user-space
without a vfio-pci variant driver. A previous commit ensures that pdev->
non_compliant_bars is set for ISM so use this to disallow mmap() with
the expecation that mmap() of non-compliant BARs is not advisable in the
general case either.

Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
 drivers/vfio/pci/vfio_pci_core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 987c7921affa..0e9d46575776 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -128,10 +128,9 @@ static void vfio_pci_probe_mmaps(struct vfio_pci_core_device *vdev)
 
 		/*
 		 * The PCI core shouldn't set up a resource with a
-		 * type but zero size. But there may be bugs that
-		 * cause us to do that.
+		 * type but zero size or non-compliant BARs.
 		 */
-		if (!resource_size(res))
+		if (!resource_size(res) || vdev->pdev->non_compliant_bars)
 			goto no_mmap;
 
 		if (resource_size(res) >= PAGE_SIZE) {

-- 
2.40.1

Re: [PATCH v4 3/4] vfio/pci: Disable mmap() non-compliant BARs

[Alex Williamson]

On Wed, 26 Jun 2024 13:15:50 +0200
Niklas Schnelle <schnelle@linux.ibm.com> wrote:

> When VFIO_PCI_MMAP is enabled for s390 in a future commit and the ISM
> device is passed-through to a KVM guest QEMU attempts to eagerly mmap()
> its BAR. This fails because the 256 TiB large BAR does not fit in the
> virtual map. Besides this issue mmap() of the ISM device's BAR is not
> useful either as even a partial mapping won't be usable from user-space
> without a vfio-pci variant driver. A previous commit ensures that pdev->
> non_compliant_bars is set for ISM so use this to disallow mmap() with
> the expecation that mmap() of non-compliant BARs is not advisable in the
> general case either.
> 
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> ---
>  drivers/vfio/pci/vfio_pci_core.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 987c7921affa..0e9d46575776 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -128,10 +128,9 @@ static void vfio_pci_probe_mmaps(struct vfio_pci_core_device *vdev)
>  
>  		/*
>  		 * The PCI core shouldn't set up a resource with a
> -		 * type but zero size. But there may be bugs that
> -		 * cause us to do that.
> +		 * type but zero size or non-compliant BARs.
>  		 */
> -		if (!resource_size(res))
> +		if (!resource_size(res) || vdev->pdev->non_compliant_bars)
>  			goto no_mmap;
>  
>  		if (resource_size(res) >= PAGE_SIZE) {
> 

The non_compliant_bars flag causes pci_read_bases() to exit, shouldn't
that mean the resource is not setup and resource_size() is zero and
explicitly testing the non_compliant_bars flag is redundant?  Or does
s390 do this somewhere else?

The non_compliant_bars flag is defined as /* Broken BARs; ignore them */
so it'd be pretty strange if they had a resource size and we chose to
still expose them with read-write access... why wouldn't we just
deny-list the device from use with vfio-pci?

Also probably worth an explicit comment in the commit log why pci-sysfs
mmap support doesn't need to be bypassed on s390.  Thanks,

Alex

Re: [PATCH v4 3/4] vfio/pci: Disable mmap() non-compliant BARs

[Niklas Schnelle]

On Thu, 2024-06-27 at 16:05 -0600, Alex Williamson wrote:
> On Wed, 26 Jun 2024 13:15:50 +0200
> Niklas Schnelle <schnelle@linux.ibm.com> wrote:
> 
> > When VFIO_PCI_MMAP is enabled for s390 in a future commit and the ISM
> > device is passed-through to a KVM guest QEMU attempts to eagerly mmap()
> > its BAR. This fails because the 256 TiB large BAR does not fit in the
> > virtual map. Besides this issue mmap() of the ISM device's BAR is not
> > useful either as even a partial mapping won't be usable from user-space
> > without a vfio-pci variant driver. A previous commit ensures that pdev->
> > non_compliant_bars is set for ISM so use this to disallow mmap() with
> > the expecation that mmap() of non-compliant BARs is not advisable in the
> > general case either.
> > 
> > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > ---
> >  drivers/vfio/pci/vfio_pci_core.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> > index 987c7921affa..0e9d46575776 100644
> > --- a/drivers/vfio/pci/vfio_pci_core.c
> > +++ b/drivers/vfio/pci/vfio_pci_core.c
> > @@ -128,10 +128,9 @@ static void vfio_pci_probe_mmaps(struct vfio_pci_core_device *vdev)
> >  
> >  		/*
> >  		 * The PCI core shouldn't set up a resource with a
> > -		 * type but zero size. But there may be bugs that
> > -		 * cause us to do that.
> > +		 * type but zero size or non-compliant BARs.
> >  		 */
> > -		if (!resource_size(res))
> > +		if (!resource_size(res) || vdev->pdev->non_compliant_bars)
> >  			goto no_mmap;
> >  
> >  		if (resource_size(res) >= PAGE_SIZE) {
> > 
> 
> The non_compliant_bars flag causes pci_read_bases() to exit, shouldn't
> that mean the resource is not setup and resource_size() is zero and
> explicitly testing the non_compliant_bars flag is redundant?  Or does
> s390 do this somewhere else?
> 
> The non_compliant_bars flag is defined as /* Broken BARs; ignore them */
> so it'd be pretty strange if they had a resource size and we chose to
> still expose them with read-write access... why wouldn't we just
> deny-list the device from use with vfio-pci?
> 
> Also probably worth an explicit comment in the commit log why pci-sysfs
> mmap support doesn't need to be bypassed on s390.  Thanks,
> 
> Alex
> 

Uhh, you're right. With pdev->non_compliant_bars we don't get the
resources filled in at all due to the check in pci_read_bases(), the
structs are however still allocated. Consequently IORESOURCE_MEM is
unset (and resource_size(res) is 0) and the existing IORESOURCE_MEM
check already ignores the ISM device here. Funnily enough this doesn't
seem to cause issues for the ISM driver itself because of the way it
bypasses normal PCI access and doesn't use ioremap() nor the MMIO
access helpers. Honestlym I think pdev->non_compliant_bars might still
be the right thing. This BAR is special enough that anything other than
the ISM driver is probably served best by just ignoring it. We could
then just drop this patch. I think this would also take care of pci-
sysfs mmap ignoring ISM if s390x were to set HAVE_PCI_MMAP.

Thanks,
Niklas

[PATCH v4 4/4] vfio/pci: Enable PCI resource mmap() on s390 and remove VFIO_PCI_MMAP

[Niklas Schnelle]

With the introduction of memory I/O (MIO) instructions enbaled in commit
71ba41c9b1d9 ("s390/pci: provide support for MIO instructions") s390
gained support for direct user-space access to mapped PCI resources.
Even without those however user-space can access mapped PCI resources
via the s390 specific MMIO syscalls. Thus mmap() can and should be
supported on all s390 systems with native PCI. Since VFIO_PCI_MMAP
enablement for s390 would make it unconditionally true and thus
pointless just remove it entirely.

Link: https://lore.kernel.org/all/c5ba134a1d4f4465b5956027e6a4ea6f6beff969.camel@linux.ibm.com/
Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
 drivers/vfio/pci/Kconfig         | 4 ----
 drivers/vfio/pci/vfio_pci_core.c | 3 ---
 2 files changed, 7 deletions(-)

diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
index bf50ffa10bde..c3bcb6911c53 100644
--- a/drivers/vfio/pci/Kconfig
+++ b/drivers/vfio/pci/Kconfig
@@ -7,10 +7,6 @@ config VFIO_PCI_CORE
 	select VFIO_VIRQFD
 	select IRQ_BYPASS_MANAGER
 
-config VFIO_PCI_MMAP
-	def_bool y if !S390
-	depends on VFIO_PCI_CORE
-
 config VFIO_PCI_INTX
 	def_bool y if !S390
 	depends on VFIO_PCI_CORE
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 0e9d46575776..c08d0f7bb500 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -120,9 +120,6 @@ static void vfio_pci_probe_mmaps(struct vfio_pci_core_device *vdev)
 
 		res = &vdev->pdev->resource[bar];
 
-		if (!IS_ENABLED(CONFIG_VFIO_PCI_MMAP))
-			goto no_mmap;
-
 		if (!(res->flags & IORESOURCE_MEM))
 			goto no_mmap;
 

-- 
2.40.1

Re: [PATCH v4 4/4] vfio/pci: Enable PCI resource mmap() on s390 and remove VFIO_PCI_MMAP

[Alex Williamson]

On Wed, 26 Jun 2024 13:15:51 +0200
Niklas Schnelle <schnelle@linux.ibm.com> wrote:

> With the introduction of memory I/O (MIO) instructions enbaled in commit
> 71ba41c9b1d9 ("s390/pci: provide support for MIO instructions") s390
> gained support for direct user-space access to mapped PCI resources.
> Even without those however user-space can access mapped PCI resources
> via the s390 specific MMIO syscalls. Thus mmap() can and should be
> supported on all s390 systems with native PCI. Since VFIO_PCI_MMAP
> enablement for s390 would make it unconditionally true and thus
> pointless just remove it entirely.
> 
> Link: https://lore.kernel.org/all/c5ba134a1d4f4465b5956027e6a4ea6f6beff969.camel@linux.ibm.com/
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> ---
>  drivers/vfio/pci/Kconfig         | 4 ----
>  drivers/vfio/pci/vfio_pci_core.c | 3 ---
>  2 files changed, 7 deletions(-)

I think you're planning a v5 which drops patch 3/ of this series and
finesses the commit log of patch 2/ a bit.  This has become much less a
vfio series, so if you want to commit through s390,

Acked-by: Alex Williamson <alex.williamson@redhat.com>

Thanks,
Alex

> 
> diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
> index bf50ffa10bde..c3bcb6911c53 100644
> --- a/drivers/vfio/pci/Kconfig
> +++ b/drivers/vfio/pci/Kconfig
> @@ -7,10 +7,6 @@ config VFIO_PCI_CORE
>  	select VFIO_VIRQFD
>  	select IRQ_BYPASS_MANAGER
>  
> -config VFIO_PCI_MMAP
> -	def_bool y if !S390
> -	depends on VFIO_PCI_CORE
> -
>  config VFIO_PCI_INTX
>  	def_bool y if !S390
>  	depends on VFIO_PCI_CORE
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 0e9d46575776..c08d0f7bb500 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -120,9 +120,6 @@ static void vfio_pci_probe_mmaps(struct vfio_pci_core_device *vdev)
>  
>  		res = &vdev->pdev->resource[bar];
>  
> -		if (!IS_ENABLED(CONFIG_VFIO_PCI_MMAP))
> -			goto no_mmap;
> -
>  		if (!(res->flags & IORESOURCE_MEM))
>  			goto no_mmap;
>  
> 

Re: [PATCH v4 4/4] vfio/pci: Enable PCI resource mmap() on s390 and remove VFIO_PCI_MMAP

[Niklas Schnelle]

On Thu, 2024-07-04 at 10:16 -0600, Alex Williamson wrote:
> On Wed, 26 Jun 2024 13:15:51 +0200
> Niklas Schnelle <schnelle@linux.ibm.com> wrote:
> 
> > With the introduction of memory I/O (MIO) instructions enbaled in commit
> > 71ba41c9b1d9 ("s390/pci: provide support for MIO instructions") s390
> > gained support for direct user-space access to mapped PCI resources.
> > Even without those however user-space can access mapped PCI resources
> > via the s390 specific MMIO syscalls. Thus mmap() can and should be
> > supported on all s390 systems with native PCI. Since VFIO_PCI_MMAP
> > enablement for s390 would make it unconditionally true and thus
> > pointless just remove it entirely.
> > 
> > Link: https://lore.kernel.org/all/c5ba134a1d4f4465b5956027e6a4ea6f6beff969.camel@linux.ibm.com/
> > Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> > Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > ---
> >  drivers/vfio/pci/Kconfig         | 4 ----
> >  drivers/vfio/pci/vfio_pci_core.c | 3 ---
> >  2 files changed, 7 deletions(-)
> 
> I think you're planning a v5 which drops patch 3/ of this series and
> finesses the commit log of patch 2/ a bit.  This has become much less a
> vfio series, so if you want to commit through s390,
> 
> Acked-by: Alex Williamson <alex.williamson@redhat.com>
> 
> Thanks,
> Alex
> 

Thank you! Yes I will send a v5. I actually already pushed a changed
version to my git.kernel.org branch but we're still discussing
internally because pdev->non_compliant_bars respectively the resulting
removal of the resources is interfering with future work on user-space
vfio-pci use of the ISM device with a vfio-pci-ism variant driver.

Thanks,
Niklas

Re: [PATCH v4 0/4] vfio/pci: s390: Fix issues preventing VFIO_PCI_MMAP=y for s390 and enable it

[Christoph Hellwig]

Thanks Niklas,

I like the quirk version much better than the magic check for the
ioremap range.