On Wed, 12 Feb 2025 16:28:32 +0100
Niklas Schnelle <schnelle@xxxxxxxxxxxxx> 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
> As s390x has no PCI quirk handling add basic 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@xxxxxxxxxxxxx>
> ---
> arch/s390/Kconfig | 4 +---
> arch/s390/pci/Makefile | 2 +-
> arch/s390/pci/pci_fixup.c | 23 +++++++++++++++++++++++
> drivers/s390/net/ism_drv.c | 1 -
> drivers/vfio/pci/Kconfig | 4 ----
> drivers/vfio/pci/vfio_pci_core.c | 2 +-
> include/linux/pci.h | 1 +
> include/linux/pci_ids.h | 1 +
> 8 files changed, 28 insertions(+), 10 deletions(-)
>
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index 9c9ec08d78c71b4d227beeafab1b82d6434cb5c7..e48741e001476f765e8aba0037a1b386df393683 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
>
> @@ -258,6 +255,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 df73c5182990ad3ae4ed5a785953011feb9a093c..1810e0944a4ed9d31261788f0f6eb341e5316546 100644
> --- a/arch/s390/pci/Makefile
> +++ b/arch/s390/pci/Makefile
> @@ -5,6 +5,6 @@
>
> obj-$(CONFIG_PCI) += pci.o pci_irq.o pci_clp.o \
> pci_event.o pci_debug.o pci_insn.o pci_mmio.o \
> - pci_bus.o pci_kvm_hook.o pci_report.o
> + pci_bus.o pci_kvm_hook.o pci_report.o pci_fixup.o
> obj-$(CONFIG_PCI_IOV) += pci_iov.o
> obj-$(CONFIG_SYSFS) += pci_sysfs.o
> diff --git a/arch/s390/pci/pci_fixup.c b/arch/s390/pci/pci_fixup.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..35688b645098329f082d0c40cc8c59231c390eaa
> --- /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. 2025
> + *
> + * Author(s):
> + * Niklas Schnelle <schnelle@xxxxxxxxxxxxx>
> + */
> +#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_mappable_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 e36e3ea165d3b2b01d68e53634676cb8c2c40220..d32633ed9fa80c1764724f493b363bfd6cb4f9cf 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/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
> index bf50ffa10bdea9e52a9d01cc3d6ee4cade39a08c..c3bcb6911c538286f7985f9c5e938d587fc04b56 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 586e49efb81be32ccb50ca554a60cec684c37402..c8586d47704c74cf9a5256d65bbf888db72b2f91 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -116,7 +116,7 @@ static void vfio_pci_probe_mmaps(struct vfio_pci_core_device *vdev)
>
> res = &vdev->pdev->resource[bar];
>
> - if (!IS_ENABLED(CONFIG_VFIO_PCI_MMAP))
> + if (vdev->pdev->non_mappable_bars)
> goto no_mmap;
>
> if (!(res->flags & IORESOURCE_MEM))
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 47b31ad724fa5bf7abd7c3dc572947551b0f2148..7192b9d78d7e337ce6144190325458fe3c0f1696 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -476,6 +476,7 @@ struct pci_dev {
> unsigned int no_command_memory:1; /* No PCI_COMMAND_MEMORY */
> unsigned int rom_bar_overlap:1; /* ROM BAR disable broken */
> unsigned int rom_attr_enabled:1; /* Display of ROM attribute enabled? */
> + unsigned int non_mappable_bars:1; /* BARs can't be mapped to user-space */
> pci_dev_flags_t dev_flags;
> atomic_t enable_cnt; /* pci_enable_device has been called */
>
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index de5deb1a0118fcf56570d461cbe7a501d4bd0da3..ec6d311ed12e174dc0bad2ce8c92454bed668fee 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -518,6 +518,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
>
