LinuxLists.cc - [PATCH v4 4/4] vfio: pci: Using a device region to retrieve zPCI information

2019-09-08 11:17:33

Subject: [PATCH v4 4/4] vfio: pci: Using a device region to retrieve zPCI information

From: Pierre Morel <[email protected]>

We define a new configuration entry for VFIO/PCI, VFIO_PCI_ZDEV

When the VFIO_PCI_ZDEV feature is configured we initialize
a new device region, VFIO_REGION_SUBTYPE_ZDEV_CLP, to hold
the information from the ZPCI device the use

Signed-off-by: Pierre Morel <[email protected]>
Signed-off-by: Matthew Rosato <[email protected]>
---
drivers/vfio/pci/Kconfig | 7 +++
drivers/vfio/pci/Makefile | 1 +
drivers/vfio/pci/vfio_pci.c | 9 ++++
drivers/vfio/pci/vfio_pci_private.h | 10 +++++
drivers/vfio/pci/vfio_pci_zdev.c | 85 +++++++++++++++++++++++++++++++++++++
5 files changed, 112 insertions(+)
create mode 100644 drivers/vfio/pci/vfio_pci_zdev.c

diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
index ac3c1dd..d4562a8 100644
--- a/drivers/vfio/pci/Kconfig
+++ b/drivers/vfio/pci/Kconfig
@@ -45,3 +45,10 @@ config VFIO_PCI_NVLINK2
depends on VFIO_PCI && PPC_POWERNV
help
VFIO PCI support for P9 Witherspoon machine with NVIDIA V100 GPUs
+
+config VFIO_PCI_ZDEV
+ bool "VFIO PCI Generic for ZPCI devices"
+ depends on VFIO_PCI && S390
+ default y
+ help
+ VFIO PCI support for S390 Z-PCI devices
diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
index f027f8a..781e080 100644
--- a/drivers/vfio/pci/Makefile
+++ b/drivers/vfio/pci/Makefile
@@ -3,5 +3,6 @@
vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
vfio-pci-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o
+vfio-pci-$(CONFIG_VFIO_PCI_ZDEV) += vfio_pci_zdev.o

obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 703948c..b40544a 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -356,6 +356,15 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev)
}
}

+ if (IS_ENABLED(CONFIG_VFIO_PCI_ZDEV)) {
+ ret = vfio_pci_zdev_init(vdev);
+ if (ret) {
+ dev_warn(&vdev->pdev->dev,
+ "Failed to setup ZDEV regions\n");
+ goto disable_exit;
+ }
+ }
+
vfio_pci_probe_mmaps(vdev);

return 0;
diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
index ee6ee91..08e02f5 100644
--- a/drivers/vfio/pci/vfio_pci_private.h
+++ b/drivers/vfio/pci/vfio_pci_private.h
@@ -186,4 +186,14 @@ static inline int vfio_pci_ibm_npu2_init(struct vfio_pci_device *vdev)
return -ENODEV;
}
#endif
+
+#ifdef CONFIG_VFIO_PCI_ZDEV
+extern int vfio_pci_zdev_init(struct vfio_pci_device *vdev);
+#else
+static inline int vfio_pci_zdev_init(struct vfio_pci_device *vdev)
+{
+ return -ENODEV;
+}
+#endif
+
#endif /* VFIO_PCI_PRIVATE_H */
diff --git a/drivers/vfio/pci/vfio_pci_zdev.c b/drivers/vfio/pci/vfio_pci_zdev.c
new file mode 100644
index 0000000..22e2b60
--- /dev/null
+++ b/drivers/vfio/pci/vfio_pci_zdev.c
@@ -0,0 +1,85 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * VFIO ZPCI devices support
+ *
+ * Copyright (C) IBM Corp. 2019. All rights reserved.
+ * Author: Pierre Morel <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+#include <linux/io.h>
+#include <linux/pci.h>
+#include <linux/uaccess.h>
+#include <linux/vfio.h>
+#include <linux/vfio_zdev.h>
+
+#include "vfio_pci_private.h"
+
+static size_t vfio_pci_zdev_rw(struct vfio_pci_device *vdev,
+ char __user *buf, size_t count, loff_t *ppos,
+ bool iswrite)
+{
+ struct vfio_region_zpci_info *region;
+ struct zpci_dev *zdev;
+ unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
+ loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
+
+ if (!vdev->pdev->bus)
+ return -ENODEV;
+
+ zdev = vdev->pdev->bus->sysdata;
+ if (!zdev)
+ return -ENODEV;
+
+ if (pos >= sizeof(*region) || iswrite)
+ return -EINVAL;
+
+ region = vdev->region[index - VFIO_PCI_NUM_REGIONS].data;
+ region->dasm = zdev->dma_mask;
+ region->start_dma = zdev->start_dma;
+ region->end_dma = zdev->end_dma;
+ region->msi_addr = zdev->msi_addr;
+ region->flags = VFIO_PCI_ZDEV_FLAGS_REFRESH;
+ region->gid = zdev->pfgid;
+ region->mui = zdev->fmb_update;
+ region->noi = zdev->max_msi;
+ memcpy(region->util_str, zdev->util_str, CLP_UTIL_STR_LEN);
+
+ count = min(count, (size_t)(sizeof(*region) - pos));
+ if (copy_to_user(buf, region, count))
+ return -EFAULT;
+
+ return count;
+}
+
+static void vfio_pci_zdev_release(struct vfio_pci_device *vdev,
+ struct vfio_pci_region *region)
+{
+ kfree(region->data);
+}
+
+static const struct vfio_pci_regops vfio_pci_zdev_regops = {
+ .rw = vfio_pci_zdev_rw,
+ .release = vfio_pci_zdev_release,
+};
+
+int vfio_pci_zdev_init(struct vfio_pci_device *vdev)
+{
+ struct vfio_region_zpci_info *region;
+ int ret;
+
+ region = kmalloc(sizeof(*region) + CLP_UTIL_STR_LEN, GFP_KERNEL);
+ if (!region)
+ return -ENOMEM;
+
+ ret = vfio_pci_register_dev_region(vdev,
+ PCI_VENDOR_ID_IBM | VFIO_REGION_TYPE_PCI_VENDOR_TYPE,
+ VFIO_REGION_SUBTYPE_ZDEV_CLP,
+ &vfio_pci_zdev_regops, sizeof(*region) + CLP_UTIL_STR_LEN,
+ VFIO_REGION_INFO_FLAG_READ, region);
+
+ return ret;
+}
--
1.8.3.1

2019-09-19 21:31:46

by Cornelia Huck

[permalink] [raw]

Subject: Re: [PATCH v4 4/4] vfio: pci: Using a device region to retrieve zPCI information

2019-09-20 06:46:05

by Matthew Rosato

[permalink] [raw]

Subject: Re: [PATCH v4 4/4] vfio: pci: Using a device region to retrieve zPCI information

On 9/19/19 11:25 AM, Cornelia Huck wrote:
> On Fri, 6 Sep 2019 20:13:51 -0400
> Matthew Rosato <[email protected]> wrote:
>
>> From: Pierre Morel <[email protected]>
>>
>> We define a new configuration entry for VFIO/PCI, VFIO_PCI_ZDEV
>>
>> When the VFIO_PCI_ZDEV feature is configured we initialize
>> a new device region, VFIO_REGION_SUBTYPE_ZDEV_CLP, to hold
>> the information from the ZPCI device the use
>>
>> Signed-off-by: Pierre Morel <[email protected]>
>> Signed-off-by: Matthew Rosato <[email protected]>
>> ---
>> drivers/vfio/pci/Kconfig | 7 +++
>> drivers/vfio/pci/Makefile | 1 +
>> drivers/vfio/pci/vfio_pci.c | 9 ++++
>> drivers/vfio/pci/vfio_pci_private.h | 10 +++++
>> drivers/vfio/pci/vfio_pci_zdev.c | 85 +++++++++++++++++++++++++++++++++++++
>> 5 files changed, 112 insertions(+)
>> create mode 100644 drivers/vfio/pci/vfio_pci_zdev.c
>>
>> diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
>> index ac3c1dd..d4562a8 100644
>> --- a/drivers/vfio/pci/Kconfig
>> +++ b/drivers/vfio/pci/Kconfig
>> @@ -45,3 +45,10 @@ config VFIO_PCI_NVLINK2
>> depends on VFIO_PCI && PPC_POWERNV
>> help
>> VFIO PCI support for P9 Witherspoon machine with NVIDIA V100 GPUs
>> +
>> +config VFIO_PCI_ZDEV
>> + bool "VFIO PCI Generic for ZPCI devices"
>> + depends on VFIO_PCI && S390
>> + default y
>> + help
>> + VFIO PCI support for S390 Z-PCI devices
>
>>From that description, I'd have no idea whether I'd want that or not.
> Is there any downside to enabling it?
>

:) Not really, you're just getting information from the hardware vs
using hard-coded defaults. The only reason I could think of to turn it
off would be if you wanted/needed to restore this hard-coded behavior.

bool "VFIO PCI support for generic ZPCI devices" ?

"Support for sharing ZPCI hardware device information between the host
and guests." ?

>> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
>> index f027f8a..781e080 100644
>> --- a/drivers/vfio/pci/Makefile
>> +++ b/drivers/vfio/pci/Makefile
>> @@ -3,5 +3,6 @@
>> vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
>> vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
>> vfio-pci-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o
>> +vfio-pci-$(CONFIG_VFIO_PCI_ZDEV) += vfio_pci_zdev.o
>>
>> obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
>> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
>> index 703948c..b40544a 100644
>> --- a/drivers/vfio/pci/vfio_pci.c
>> +++ b/drivers/vfio/pci/vfio_pci.c
>> @@ -356,6 +356,15 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev)
>> }
>> }
>>
>> + if (IS_ENABLED(CONFIG_VFIO_PCI_ZDEV)) {
>> + ret = vfio_pci_zdev_init(vdev);
>> + if (ret) {
>> + dev_warn(&vdev->pdev->dev,
>> + "Failed to setup ZDEV regions\n");
>> + goto disable_exit;
>> + }
>> + }
>> +
>> vfio_pci_probe_mmaps(vdev);
>>
>> return 0;
>> diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
>> index ee6ee91..08e02f5 100644
>> --- a/drivers/vfio/pci/vfio_pci_private.h
>> +++ b/drivers/vfio/pci/vfio_pci_private.h
>> @@ -186,4 +186,14 @@ static inline int vfio_pci_ibm_npu2_init(struct vfio_pci_device *vdev)
>> return -ENODEV;
>> }
>> #endif
>> +
>> +#ifdef CONFIG_VFIO_PCI_ZDEV
>> +extern int vfio_pci_zdev_init(struct vfio_pci_device *vdev);
>> +#else
>> +static inline int vfio_pci_zdev_init(struct vfio_pci_device *vdev)
>> +{
>> + return -ENODEV;
>
> If you really want to have this configurable, why not just return 0
> here and skip the IS_ENABLED check above?
>

I agree that it functionally has the same result, but in this case I
think Pierre was repeating the same thing the other init() functions
here (IGD, etc) are doing. Though I guess the other cases have at least
1 other condition they care about besides IS_ENABLED... OK, I can make
this change.

>> +}
>> +#endif
>> +
>> #endif /* VFIO_PCI_PRIVATE_H */
>
> (...)
>

2019-09-20 21:26:59

by Alex Williamson

[permalink] [raw]

Subject: Re: [PATCH v4 4/4] vfio: pci: Using a device region to retrieve zPCI information

On Fri, 6 Sep 2019 20:13:51 -0400
Matthew Rosato <[email protected]> wrote:

> From: Pierre Morel <[email protected]>
>
> We define a new configuration entry for VFIO/PCI, VFIO_PCI_ZDEV
>
> When the VFIO_PCI_ZDEV feature is configured we initialize
> a new device region, VFIO_REGION_SUBTYPE_ZDEV_CLP, to hold
> the information from the ZPCI device the use
>
> Signed-off-by: Pierre Morel <[email protected]>
> Signed-off-by: Matthew Rosato <[email protected]>
> ---
> drivers/vfio/pci/Kconfig | 7 +++
> drivers/vfio/pci/Makefile | 1 +
> drivers/vfio/pci/vfio_pci.c | 9 ++++
> drivers/vfio/pci/vfio_pci_private.h | 10 +++++
> drivers/vfio/pci/vfio_pci_zdev.c | 85 +++++++++++++++++++++++++++++++++++++
> 5 files changed, 112 insertions(+)
> create mode 100644 drivers/vfio/pci/vfio_pci_zdev.c
>
> diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
> index ac3c1dd..d4562a8 100644
> --- a/drivers/vfio/pci/Kconfig
> +++ b/drivers/vfio/pci/Kconfig
> @@ -45,3 +45,10 @@ config VFIO_PCI_NVLINK2
> depends on VFIO_PCI && PPC_POWERNV
> help
> VFIO PCI support for P9 Witherspoon machine with NVIDIA V100 GPUs
> +
> +config VFIO_PCI_ZDEV
> + bool "VFIO PCI Generic for ZPCI devices"
> + depends on VFIO_PCI && S390
> + default y
> + help
> + VFIO PCI support for S390 Z-PCI devices
> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> index f027f8a..781e080 100644
> --- a/drivers/vfio/pci/Makefile
> +++ b/drivers/vfio/pci/Makefile
> @@ -3,5 +3,6 @@
> vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> vfio-pci-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o
> +vfio-pci-$(CONFIG_VFIO_PCI_ZDEV) += vfio_pci_zdev.o
>
> obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 703948c..b40544a 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -356,6 +356,15 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev)
> }
> }
>
> + if (IS_ENABLED(CONFIG_VFIO_PCI_ZDEV)) {
> + ret = vfio_pci_zdev_init(vdev);
> + if (ret) {
> + dev_warn(&vdev->pdev->dev,
> + "Failed to setup ZDEV regions\n");
> + goto disable_exit;
> + }
> + }
> +
> vfio_pci_probe_mmaps(vdev);
>
> return 0;
> diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
> index ee6ee91..08e02f5 100644
> --- a/drivers/vfio/pci/vfio_pci_private.h
> +++ b/drivers/vfio/pci/vfio_pci_private.h
> @@ -186,4 +186,14 @@ static inline int vfio_pci_ibm_npu2_init(struct vfio_pci_device *vdev)
> return -ENODEV;
> }
> #endif
> +
> +#ifdef CONFIG_VFIO_PCI_ZDEV
> +extern int vfio_pci_zdev_init(struct vfio_pci_device *vdev);
> +#else
> +static inline int vfio_pci_zdev_init(struct vfio_pci_device *vdev)
> +{
> + return -ENODEV;
> +}
> +#endif
> +
> #endif /* VFIO_PCI_PRIVATE_H */
> diff --git a/drivers/vfio/pci/vfio_pci_zdev.c b/drivers/vfio/pci/vfio_pci_zdev.c
> new file mode 100644
> index 0000000..22e2b60
> --- /dev/null
> +++ b/drivers/vfio/pci/vfio_pci_zdev.c
> @@ -0,0 +1,85 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * VFIO ZPCI devices support
> + *
> + * Copyright (C) IBM Corp. 2019. All rights reserved.
> + * Author: Pierre Morel <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +#include <linux/io.h>
> +#include <linux/pci.h>
> +#include <linux/uaccess.h>
> +#include <linux/vfio.h>
> +#include <linux/vfio_zdev.h>
> +
> +#include "vfio_pci_private.h"
> +
> +static size_t vfio_pci_zdev_rw(struct vfio_pci_device *vdev,
> + char __user *buf, size_t count, loff_t *ppos,
> + bool iswrite)
> +{
> + struct vfio_region_zpci_info *region;
> + struct zpci_dev *zdev;
> + unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
> + loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
> +
> + if (!vdev->pdev->bus)
> + return -ENODEV;
> +
> + zdev = vdev->pdev->bus->sysdata;
> + if (!zdev)
> + return -ENODEV;
> +
> + if (pos >= sizeof(*region) || iswrite)
> + return -EINVAL;
> +
> + region = vdev->region[index - VFIO_PCI_NUM_REGIONS].data;
> + region->dasm = zdev->dma_mask;
> + region->start_dma = zdev->start_dma;
> + region->end_dma = zdev->end_dma;
> + region->msi_addr = zdev->msi_addr;
> + region->flags = VFIO_PCI_ZDEV_FLAGS_REFRESH;

Even more curious what this means, why do we need a flag that's always
set? Maybe NOREFRESH if it were ever to exist.

> + region->gid = zdev->pfgid;
> + region->mui = zdev->fmb_update;
> + region->noi = zdev->max_msi;
> + memcpy(region->util_str, zdev->util_str, CLP_UTIL_STR_LEN);

Just checking, I assume this is dynamic based on it being recreated
every time, otherwise you'd have created it in the init function and
just do the below on read, right? The fields that I can guess what they
might be don't seem like they'd change. Comments would be good.
Thanks,

Alex

> +
> + count = min(count, (size_t)(sizeof(*region) - pos));
> + if (copy_to_user(buf, region, count))
> + return -EFAULT;
> +
> + return count;
> +}
> +
> +static void vfio_pci_zdev_release(struct vfio_pci_device *vdev,
> + struct vfio_pci_region *region)
> +{
> + kfree(region->data);
> +}
> +
> +static const struct vfio_pci_regops vfio_pci_zdev_regops = {
> + .rw = vfio_pci_zdev_rw,
> + .release = vfio_pci_zdev_release,
> +};
> +
> +int vfio_pci_zdev_init(struct vfio_pci_device *vdev)
> +{
> + struct vfio_region_zpci_info *region;
> + int ret;
> +
> + region = kmalloc(sizeof(*region) + CLP_UTIL_STR_LEN, GFP_KERNEL);
> + if (!region)
> + return -ENOMEM;
> +
> + ret = vfio_pci_register_dev_region(vdev,
> + PCI_VENDOR_ID_IBM | VFIO_REGION_TYPE_PCI_VENDOR_TYPE,
> + VFIO_REGION_SUBTYPE_ZDEV_CLP,
> + &vfio_pci_zdev_regops, sizeof(*region) + CLP_UTIL_STR_LEN,
> + VFIO_REGION_INFO_FLAG_READ, region);
> +
> + return ret;
> +}

2019-09-22 06:39:38

by Cornelia Huck

[permalink] [raw]

Subject: Re: [PATCH v4 4/4] vfio: pci: Using a device region to retrieve zPCI information

On Thu, 19 Sep 2019 16:57:10 -0400
Matthew Rosato <[email protected]> wrote:

> On 9/19/19 11:25 AM, Cornelia Huck wrote:
> > On Fri, 6 Sep 2019 20:13:51 -0400
> > Matthew Rosato <[email protected]> wrote:
> >
> >> From: Pierre Morel <[email protected]>
> >>
> >> We define a new configuration entry for VFIO/PCI, VFIO_PCI_ZDEV
> >>
> >> When the VFIO_PCI_ZDEV feature is configured we initialize
> >> a new device region, VFIO_REGION_SUBTYPE_ZDEV_CLP, to hold
> >> the information from the ZPCI device the use
> >>
> >> Signed-off-by: Pierre Morel <[email protected]>
> >> Signed-off-by: Matthew Rosato <[email protected]>
> >> ---
> >> drivers/vfio/pci/Kconfig | 7 +++
> >> drivers/vfio/pci/Makefile | 1 +
> >> drivers/vfio/pci/vfio_pci.c | 9 ++++
> >> drivers/vfio/pci/vfio_pci_private.h | 10 +++++
> >> drivers/vfio/pci/vfio_pci_zdev.c | 85 +++++++++++++++++++++++++++++++++++++
> >> 5 files changed, 112 insertions(+)
> >> create mode 100644 drivers/vfio/pci/vfio_pci_zdev.c
> >>
> >> diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
> >> index ac3c1dd..d4562a8 100644
> >> --- a/drivers/vfio/pci/Kconfig
> >> +++ b/drivers/vfio/pci/Kconfig
> >> @@ -45,3 +45,10 @@ config VFIO_PCI_NVLINK2
> >> depends on VFIO_PCI && PPC_POWERNV
> >> help
> >> VFIO PCI support for P9 Witherspoon machine with NVIDIA V100 GPUs
> >> +
> >> +config VFIO_PCI_ZDEV
> >> + bool "VFIO PCI Generic for ZPCI devices"
> >> + depends on VFIO_PCI && S390
> >> + default y
> >> + help
> >> + VFIO PCI support for S390 Z-PCI devices
> >
> >>From that description, I'd have no idea whether I'd want that or not.
> > Is there any downside to enabling it?
> >
>
> :) Not really, you're just getting information from the hardware vs
> using hard-coded defaults. The only reason I could think of to turn it
> off would be if you wanted/needed to restore this hard-coded behavior.

I'm not really sure whether that's worth adding a Kconfig switch for.
Won't older versions simply ignore the new region anyway?

Also, I don't think we have any migration compatibility issues, as
vfio-pci devices are not (yet) migrateable anyway.

>
> bool "VFIO PCI support for generic ZPCI devices" ?

"Support zPCI-specific configuration for VFIO PCI" ?

>
> "Support for sharing ZPCI hardware device information between the host
> and guests." ?

"Enabling this options exposes a region containing hardware
configuration for zPCI devices. This enables userspace (e.g. QEMU) to
supply proper configuration values instead of hard-coded defaults for
zPCI devices passed through via VFIO on s390.

Say Y here."

?

2019-09-22 22:20:52

by Matthew Rosato

[permalink] [raw]

Subject: Re: [PATCH v4 4/4] vfio: pci: Using a device region to retrieve zPCI information

On 9/19/19 6:57 PM, Alex Williamson wrote:
> On Fri, 6 Sep 2019 20:13:51 -0400
> Matthew Rosato <[email protected]> wrote:
>
>> From: Pierre Morel <[email protected]>
>>
>> We define a new configuration entry for VFIO/PCI, VFIO_PCI_ZDEV
>>
>> When the VFIO_PCI_ZDEV feature is configured we initialize
>> a new device region, VFIO_REGION_SUBTYPE_ZDEV_CLP, to hold
>> the information from the ZPCI device the use
>>
>> Signed-off-by: Pierre Morel <[email protected]>
>> Signed-off-by: Matthew Rosato <[email protected]>
>> ---
>> drivers/vfio/pci/Kconfig | 7 +++
>> drivers/vfio/pci/Makefile | 1 +
>> drivers/vfio/pci/vfio_pci.c | 9 ++++
>> drivers/vfio/pci/vfio_pci_private.h | 10 +++++
>> drivers/vfio/pci/vfio_pci_zdev.c | 85 +++++++++++++++++++++++++++++++++++++
>> 5 files changed, 112 insertions(+)
>> create mode 100644 drivers/vfio/pci/vfio_pci_zdev.c
>>
>> diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
>> index ac3c1dd..d4562a8 100644
>> --- a/drivers/vfio/pci/Kconfig
>> +++ b/drivers/vfio/pci/Kconfig
>> @@ -45,3 +45,10 @@ config VFIO_PCI_NVLINK2
>> depends on VFIO_PCI && PPC_POWERNV
>> help
>> VFIO PCI support for P9 Witherspoon machine with NVIDIA V100 GPUs
>> +
>> +config VFIO_PCI_ZDEV
>> + bool "VFIO PCI Generic for ZPCI devices"
>> + depends on VFIO_PCI && S390
>> + default y
>> + help
>> + VFIO PCI support for S390 Z-PCI devices
>> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
>> index f027f8a..781e080 100644
>> --- a/drivers/vfio/pci/Makefile
>> +++ b/drivers/vfio/pci/Makefile
>> @@ -3,5 +3,6 @@
>> vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
>> vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
>> vfio-pci-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o
>> +vfio-pci-$(CONFIG_VFIO_PCI_ZDEV) += vfio_pci_zdev.o
>>
>> obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
>> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
>> index 703948c..b40544a 100644
>> --- a/drivers/vfio/pci/vfio_pci.c
>> +++ b/drivers/vfio/pci/vfio_pci.c
>> @@ -356,6 +356,15 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev)
>> }
>> }
>>
>> + if (IS_ENABLED(CONFIG_VFIO_PCI_ZDEV)) {
>> + ret = vfio_pci_zdev_init(vdev);
>> + if (ret) {
>> + dev_warn(&vdev->pdev->dev,
>> + "Failed to setup ZDEV regions\n");
>> + goto disable_exit;
>> + }
>> + }
>> +
>> vfio_pci_probe_mmaps(vdev);
>>
>> return 0;
>> diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
>> index ee6ee91..08e02f5 100644
>> --- a/drivers/vfio/pci/vfio_pci_private.h
>> +++ b/drivers/vfio/pci/vfio_pci_private.h
>> @@ -186,4 +186,14 @@ static inline int vfio_pci_ibm_npu2_init(struct vfio_pci_device *vdev)
>> return -ENODEV;
>> }
>> #endif
>> +
>> +#ifdef CONFIG_VFIO_PCI_ZDEV
>> +extern int vfio_pci_zdev_init(struct vfio_pci_device *vdev);
>> +#else
>> +static inline int vfio_pci_zdev_init(struct vfio_pci_device *vdev)
>> +{
>> + return -ENODEV;
>> +}
>> +#endif
>> +
>> #endif /* VFIO_PCI_PRIVATE_H */
>> diff --git a/drivers/vfio/pci/vfio_pci_zdev.c b/drivers/vfio/pci/vfio_pci_zdev.c
>> new file mode 100644
>> index 0000000..22e2b60
>> --- /dev/null
>> +++ b/drivers/vfio/pci/vfio_pci_zdev.c
>> @@ -0,0 +1,85 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * VFIO ZPCI devices support
>> + *
>> + * Copyright (C) IBM Corp. 2019. All rights reserved.
>> + * Author: Pierre Morel <[email protected]>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + */
>> +#include <linux/io.h>
>> +#include <linux/pci.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/vfio.h>
>> +#include <linux/vfio_zdev.h>
>> +
>> +#include "vfio_pci_private.h"
>> +
>> +static size_t vfio_pci_zdev_rw(struct vfio_pci_device *vdev,
>> + char __user *buf, size_t count, loff_t *ppos,
>> + bool iswrite)
>> +{
>> + struct vfio_region_zpci_info *region;
>> + struct zpci_dev *zdev;
>> + unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
>> + loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
>> +
>> + if (!vdev->pdev->bus)
>> + return -ENODEV;
>> +
>> + zdev = vdev->pdev->bus->sysdata;
>> + if (!zdev)
>> + return -ENODEV;
>> +
>> + if (pos >= sizeof(*region) || iswrite)
>> + return -EINVAL;
>> +
>> + region = vdev->region[index - VFIO_PCI_NUM_REGIONS].data;
>> + region->dasm = zdev->dma_mask;
>> + region->start_dma = zdev->start_dma;
>> + region->end_dma = zdev->end_dma;
>> + region->msi_addr = zdev->msi_addr;
>> + region->flags = VFIO_PCI_ZDEV_FLAGS_REFRESH;
>
> Even more curious what this means, why do we need a flag that's always
> set? Maybe NOREFRESH if it were ever to exist.>

This flag also has a hardware structure counterpart -- this is
associated with Pierre's comment from the cover letter:

"Note that in the current state we do not use the CLP instructions to
access the firmware but get the information directly from the zdev
device. <...> But we will need this later, eventually in the next
iteration to retrieve values not being saved inside the zdev structure.
like maxstbl and the PCI supported version"

Since this data isn't stored in the zdev, a subsequent patch that pulls
the flag info from the CLP data would set this value intelligently vs
the current hard-coded value.

>> + region->gid = zdev->pfgid;
>> + region->mui = zdev->fmb_update;
>> + region->noi = zdev->max_msi;
>> + memcpy(region->util_str, zdev->util_str, CLP_UTIL_STR_LEN);
>
> Just checking, I assume this is dynamic based on it being recreated
> every time, otherwise you'd have created it in the init function and
> just do the below on read, right? The fields that I can guess what they
> might be don't seem like they'd change. Comments would be good.

I think you're right and this can be done in init, I'll have a look.

> Thanks,
>
> Alex
>
>> +
>> + count = min(count, (size_t)(sizeof(*region) - pos));
>> + if (copy_to_user(buf, region, count))
>> + return -EFAULT;
>> +
>> + return count;
>> +}
>> +
>> +static void vfio_pci_zdev_release(struct vfio_pci_device *vdev,
>> + struct vfio_pci_region *region)
>> +{
>> + kfree(region->data);
>> +}
>> +
>> +static const struct vfio_pci_regops vfio_pci_zdev_regops = {
>> + .rw = vfio_pci_zdev_rw,
>> + .release = vfio_pci_zdev_release,
>> +};
>> +
>> +int vfio_pci_zdev_init(struct vfio_pci_device *vdev)
>> +{
>> + struct vfio_region_zpci_info *region;
>> + int ret;
>> +
>> + region = kmalloc(sizeof(*region) + CLP_UTIL_STR_LEN, GFP_KERNEL);
>> + if (!region)
>> + return -ENOMEM;
>> +
>> + ret = vfio_pci_register_dev_region(vdev,
>> + PCI_VENDOR_ID_IBM | VFIO_REGION_TYPE_PCI_VENDOR_TYPE,
>> + VFIO_REGION_SUBTYPE_ZDEV_CLP,
>> + &vfio_pci_zdev_regops, sizeof(*region) + CLP_UTIL_STR_LEN,
>> + VFIO_REGION_INFO_FLAG_READ, region);
>> +
>> + return ret;
>> +}
>
>

2019-09-23 04:28:42

by Matthew Rosato

[permalink] [raw]

Subject: Re: [PATCH v4 4/4] vfio: pci: Using a device region to retrieve zPCI information

On 9/20/19 10:26 AM, Cornelia Huck wrote:
> On Thu, 19 Sep 2019 16:57:10 -0400
> Matthew Rosato <[email protected]> wrote:
>
>> On 9/19/19 11:25 AM, Cornelia Huck wrote:
>>> On Fri, 6 Sep 2019 20:13:51 -0400
>>> Matthew Rosato <[email protected]> wrote:
>>>
>>>> From: Pierre Morel <[email protected]>
>>>>
>>>> We define a new configuration entry for VFIO/PCI, VFIO_PCI_ZDEV
>>>>
>>>> When the VFIO_PCI_ZDEV feature is configured we initialize
>>>> a new device region, VFIO_REGION_SUBTYPE_ZDEV_CLP, to hold
>>>> the information from the ZPCI device the use
>>>>
>>>> Signed-off-by: Pierre Morel <[email protected]>
>>>> Signed-off-by: Matthew Rosato <[email protected]>
>>>> ---
>>>> drivers/vfio/pci/Kconfig | 7 +++
>>>> drivers/vfio/pci/Makefile | 1 +
>>>> drivers/vfio/pci/vfio_pci.c | 9 ++++
>>>> drivers/vfio/pci/vfio_pci_private.h | 10 +++++
>>>> drivers/vfio/pci/vfio_pci_zdev.c | 85 +++++++++++++++++++++++++++++++++++++
>>>> 5 files changed, 112 insertions(+)
>>>> create mode 100644 drivers/vfio/pci/vfio_pci_zdev.c
>>>>
>>>> diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
>>>> index ac3c1dd..d4562a8 100644
>>>> --- a/drivers/vfio/pci/Kconfig
>>>> +++ b/drivers/vfio/pci/Kconfig
>>>> @@ -45,3 +45,10 @@ config VFIO_PCI_NVLINK2
>>>> depends on VFIO_PCI && PPC_POWERNV
>>>> help
>>>> VFIO PCI support for P9 Witherspoon machine with NVIDIA V100 GPUs
>>>> +
>>>> +config VFIO_PCI_ZDEV
>>>> + bool "VFIO PCI Generic for ZPCI devices"
>>>> + depends on VFIO_PCI && S390
>>>> + default y
>>>> + help
>>>> + VFIO PCI support for S390 Z-PCI devices
>>>
>>> >From that description, I'd have no idea whether I'd want that or not.
>>> Is there any downside to enabling it?
>>>
>>
>> :) Not really, you're just getting information from the hardware vs
>> using hard-coded defaults. The only reason I could think of to turn it
>> off would be if you wanted/needed to restore this hard-coded behavior.
>
> I'm not really sure whether that's worth adding a Kconfig switch for.
> Won't older versions simply ignore the new region anyway?
>

Yes, you have a point here... This switch showed up in v3 of this
series when Pierre changed to using a region to pass this info and I
haven't yet found a 'why' he decided to add the Kconfig switch. If I
can't convince myself of a reason to keep it, I'll just remove it from
the next version.

> Also, I don't think we have any migration compatibility issues, as
> vfio-pci devices are not (yet) migrateable anyway.
>
>>
>> bool "VFIO PCI support for generic ZPCI devices" ?
>
> "Support zPCI-specific configuration for VFIO PCI" ?
>
>>
>> "Support for sharing ZPCI hardware device information between the host
>> and guests." ?
>
> "Enabling this options exposes a region containing hardware
> configuration for zPCI devices. This enables userspace (e.g. QEMU) to
> supply proper configuration values instead of hard-coded defaults for
> zPCI devices passed through via VFIO on s390.
>
> Say Y here."
>
> ?
>

Your descriptions are much better - thanks for the feedback!