Note: These patches by Pierre got lost in the ether a few months back as he has been unavailable to carry them forward. I've made changes based upon comments received on his last version. We define a new configuration entry for VFIO/PCI, VFIO_PCI_ZDEV to configure access to a zPCI region dedicated for retrieving zPCI features. 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 userland needs to give to a guest driving the zPCI function. 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. -This means that the patch 1, "s390: pci: Exporting access to CLP PCI function and PCI group" is not used and can be let out of this series without denying the good working of the other patches. - 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 To share the code with arch/s390/pci/pci_clp.c the original functions in pci_clp.c to query PCI functions and PCI functions group are modified so that they can be exported. A new function clp_query_pci() replaces clp_query_pci_fn() and the previous calls to clp_query_pci_fn() and clp_query_pci_fngrp() are replaced with calls to zdev_query_pci_fn() and zdev_query_pci_fngrp() using a zdev pointer as argument. Changes since v3: - New patch: define maxstbl - Remove CLP_UTIL_STR_LEN references from uapi header - Fix broken ifdef CONFIG_VFIO_PCI_ZDEV - Change Kconfig option from tristate to bool - Remove VFIO_REGION_TYPE_IBM_ZDEV, move VFIO_REGION_SUBTYPE_ZDEV_CLP to a 1014 subtype - reject iswrite in .rw callback - Remove rw restriction on identical buffer sizes - Allow arbitrary sized read Pierre Morel (4): s390: pci: Exporting access to CLP PCI function and PCI group s390: pci: Define the maxstbl CLP response entry vfio: zpci: defining the VFIO headers vfio: pci: Using a device region to retrieve zPCI information arch/s390/include/asm/pci.h | 3 ++ arch/s390/include/asm/pci_clp.h | 2 +- arch/s390/pci/pci_clp.c | 71 ++++++++++++++++--------------- 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 +++++++++++++++++++++++++++++++++++++ include/uapi/linux/vfio.h | 1 + include/uapi/linux/vfio_zdev.h | 35 +++++++++++++++ 10 files changed, 189 insertions(+), 35 deletions(-) create mode 100644 drivers/vfio/pci/vfio_pci_zdev.c create mode 100644 include/uapi/linux/vfio_zdev.h -- 1.8.3.1 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
From: Pierre Morel <pmorel@linux.ibm.com> For the generic implementation of VFIO PCI we need to retrieve the hardware configuration for the PCI functions and the PCI function groups. We modify the internal function using CLP Query PCI function and CLP query PCI function group so that they can be called from outside the S390 architecture PCI code and prefix the two functions with "zdev" to make clear that they can be called knowing only the associated zdevice. Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> Reviewed-by: Sebastian Ott <sebott@linux.ibm.com> --- arch/s390/include/asm/pci.h | 3 ++ arch/s390/pci/pci_clp.c | 71 +++++++++++++++++++++++---------------------- 2 files changed, 40 insertions(+), 34 deletions(-) diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h index a2399ef..dd03212 100644 --- a/arch/s390/include/asm/pci.h +++ b/arch/s390/include/asm/pci.h @@ -266,4 +266,7 @@ static inline int __pcibus_to_node(const struct pci_bus *bus) #endif /* CONFIG_NUMA */ +int zdev_query_pci_fngrp(struct zpci_dev *zdev, + struct clp_req_rsp_query_pci_grp *rrb); +int zdev_query_pci_fn(struct zpci_dev *zdev, struct clp_req_rsp_query_pci *rrb); #endif diff --git a/arch/s390/pci/pci_clp.c b/arch/s390/pci/pci_clp.c index 9bdff4d..df06fdf 100644 --- a/arch/s390/pci/pci_clp.c +++ b/arch/s390/pci/pci_clp.c @@ -113,32 +113,18 @@ static void clp_store_query_pci_fngrp(struct zpci_dev *zdev, } } -static int clp_query_pci_fngrp(struct zpci_dev *zdev, u8 pfgid) +int zdev_query_pci_fngrp(struct zpci_dev *zdev, + struct clp_req_rsp_query_pci_grp *rrb) { - struct clp_req_rsp_query_pci_grp *rrb; - int rc; - - rrb = clp_alloc_block(GFP_KERNEL); - if (!rrb) - return -ENOMEM; - memset(rrb, 0, sizeof(*rrb)); rrb->request.hdr.len = sizeof(rrb->request); rrb->request.hdr.cmd = CLP_QUERY_PCI_FNGRP; rrb->response.hdr.len = sizeof(rrb->response); - rrb->request.pfgid = pfgid; + rrb->request.pfgid = zdev->pfgid; - rc = clp_req(rrb, CLP_LPS_PCI); - if (!rc && rrb->response.hdr.rsp == CLP_RC_OK) - clp_store_query_pci_fngrp(zdev, &rrb->response); - else { - zpci_err("Q PCI FGRP:\n"); - zpci_err_clp(rrb->response.hdr.rsp, rc); - rc = -EIO; - } - clp_free_block(rrb); - return rc; + return clp_req(rrb, CLP_LPS_PCI); } +EXPORT_SYMBOL(zdev_query_pci_fngrp); static int clp_store_query_pci_fn(struct zpci_dev *zdev, struct clp_rsp_query_pci *response) @@ -174,32 +160,49 @@ static int clp_store_query_pci_fn(struct zpci_dev *zdev, return 0; } -static int clp_query_pci_fn(struct zpci_dev *zdev, u32 fh) +int zdev_query_pci_fn(struct zpci_dev *zdev, struct clp_req_rsp_query_pci *rrb) +{ + + memset(rrb, 0, sizeof(*rrb)); + rrb->request.hdr.len = sizeof(rrb->request); + rrb->request.hdr.cmd = CLP_QUERY_PCI_FN; + rrb->response.hdr.len = sizeof(rrb->response); + rrb->request.fh = zdev->fh; + + return clp_req(rrb, CLP_LPS_PCI); +} + +static int clp_query_pci(struct zpci_dev *zdev) { struct clp_req_rsp_query_pci *rrb; + struct clp_req_rsp_query_pci_grp *grrb; int rc; rrb = clp_alloc_block(GFP_KERNEL); if (!rrb) return -ENOMEM; - memset(rrb, 0, sizeof(*rrb)); - rrb->request.hdr.len = sizeof(rrb->request); - rrb->request.hdr.cmd = CLP_QUERY_PCI_FN; - rrb->response.hdr.len = sizeof(rrb->response); - rrb->request.fh = fh; - - rc = clp_req(rrb, CLP_LPS_PCI); - if (!rc && rrb->response.hdr.rsp == CLP_RC_OK) { - rc = clp_store_query_pci_fn(zdev, &rrb->response); - if (rc) - goto out; - rc = clp_query_pci_fngrp(zdev, rrb->response.pfgid); - } else { + rc = zdev_query_pci_fn(zdev, rrb); + if (rc || rrb->response.hdr.rsp != CLP_RC_OK) { zpci_err("Q PCI FN:\n"); zpci_err_clp(rrb->response.hdr.rsp, rc); rc = -EIO; + goto out; } + rc = clp_store_query_pci_fn(zdev, &rrb->response); + if (rc) + goto out; + + grrb = (struct clp_req_rsp_query_pci_grp *)rrb; + rc = zdev_query_pci_fngrp(zdev, grrb); + if (rc || grrb->response.hdr.rsp != CLP_RC_OK) { + zpci_err("Q PCI FGRP:\n"); + zpci_err_clp(grrb->response.hdr.rsp, rc); + rc = -EIO; + goto out; + } + clp_store_query_pci_fngrp(zdev, &grrb->response); + out: clp_free_block(rrb); return rc; @@ -219,7 +222,7 @@ int clp_add_pci_device(u32 fid, u32 fh, int configured) zdev->fid = fid; /* Query function properties and update zdev */ - rc = clp_query_pci_fn(zdev, fh); + rc = clp_query_pci(zdev); if (rc) goto error; -- 1.8.3.1 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
From: Pierre Morel <pmorel@linux.ibm.com> This entry is already defined in QEMU but not in Linux. We need this to export this entry to QEMU through a VFIO device region. Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> --- arch/s390/include/asm/pci_clp.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/s390/include/asm/pci_clp.h b/arch/s390/include/asm/pci_clp.h index 5035917..03fc2f0 100644 --- a/arch/s390/include/asm/pci_clp.h +++ b/arch/s390/include/asm/pci_clp.h @@ -132,7 +132,7 @@ struct clp_rsp_query_pci_grp { u8 : 6; u8 frame : 1; u8 refresh : 1; /* TLB refresh mode */ - u16 reserved2; + u16 maxstbl; u16 mui; u16 : 16; u16 maxfaal; -- 1.8.3.1 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
From: Pierre Morel <pmorel@linux.ibm.com> We define a new device region in vfio.h to be able to get the ZPCI CLP information by reading this region from userland. We create a new file, vfio_zdev.h to define the structure of the new region we defined in vfio.h Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> --- include/uapi/linux/vfio.h | 1 + include/uapi/linux/vfio_zdev.h | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+) create mode 100644 include/uapi/linux/vfio_zdev.h diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h index 8f10748..8328c87 100644 --- a/include/uapi/linux/vfio.h +++ b/include/uapi/linux/vfio.h @@ -371,6 +371,7 @@ struct vfio_region_gfx_edid { * to do TLB invalidation on a GPU. */ #define VFIO_REGION_SUBTYPE_IBM_NVLINK2_ATSD (1) +#define VFIO_REGION_SUBTYPE_ZDEV_CLP (2) /* * The MSIX mappable capability informs that MSIX data of a BAR can be mmapped diff --git a/include/uapi/linux/vfio_zdev.h b/include/uapi/linux/vfio_zdev.h new file mode 100644 index 0000000..55e0d6d --- /dev/null +++ b/include/uapi/linux/vfio_zdev.h @@ -0,0 +1,35 @@ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ +/* + * Region definition for ZPCI devices + * + * Copyright IBM Corp. 2019 + * + * Author(s): Pierre Morel <pmorel@linux.ibm.com> + */ + +#ifndef _VFIO_ZDEV_H_ +#define _VFIO_ZDEV_H_ + +#include <linux/types.h> + +/** + * struct vfio_region_zpci_info - ZPCI information. + * + */ +struct vfio_region_zpci_info { + __u64 dasm; + __u64 start_dma; + __u64 end_dma; + __u64 msi_addr; + __u64 flags; + __u16 pchid; + __u16 mui; + __u16 noi; + __u16 maxstbl; + __u8 version; + __u8 gid; +#define VFIO_PCI_ZDEV_FLAGS_REFRESH 1 + __u8 util_str[]; +} __packed; + +#endif -- 1.8.3.1 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
From: Pierre Morel <pmorel@linux.ibm.com> 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 <pmorel@linux.ibm.com> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> --- 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 <pmorel@linux.ibm.com> + * + * 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 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
ping On 9/6/19 8:13 PM, Matthew Rosato wrote: > Note: These patches by Pierre got lost in the ether a few months back > as he has been unavailable to carry them forward. I've made changes > based upon comments received on his last version. > > We define a new configuration entry for VFIO/PCI, VFIO_PCI_ZDEV > to configure access to a zPCI region dedicated for retrieving > zPCI features. > > 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 userland needs to > give to a guest driving the zPCI function. > > > 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. > > -This means that the patch 1, "s390: pci: Exporting access to CLP PCI > function and PCI group" is not used and can be let out of this series > without denying the good working of the other patches. > - 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 > > To share the code with arch/s390/pci/pci_clp.c the original functions > in pci_clp.c to query PCI functions and PCI functions group are > modified so that they can be exported. > > A new function clp_query_pci() replaces clp_query_pci_fn() and > the previous calls to clp_query_pci_fn() and clp_query_pci_fngrp() > are replaced with calls to zdev_query_pci_fn() and zdev_query_pci_fngrp() > using a zdev pointer as argument. > > Changes since v3: > - New patch: define maxstbl > - Remove CLP_UTIL_STR_LEN references from uapi header > - Fix broken ifdef CONFIG_VFIO_PCI_ZDEV > - Change Kconfig option from tristate to bool > - Remove VFIO_REGION_TYPE_IBM_ZDEV, move VFIO_REGION_SUBTYPE_ZDEV_CLP to a 1014 subtype > - reject iswrite in .rw callback > - Remove rw restriction on identical buffer sizes > - Allow arbitrary sized read > > Pierre Morel (4): > s390: pci: Exporting access to CLP PCI function and PCI group > s390: pci: Define the maxstbl CLP response entry > vfio: zpci: defining the VFIO headers > vfio: pci: Using a device region to retrieve zPCI information > > arch/s390/include/asm/pci.h | 3 ++ > arch/s390/include/asm/pci_clp.h | 2 +- > arch/s390/pci/pci_clp.c | 71 ++++++++++++++++--------------- > 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 +++++++++++++++++++++++++++++++++++++ > include/uapi/linux/vfio.h | 1 + > include/uapi/linux/vfio_zdev.h | 35 +++++++++++++++ > 10 files changed, 189 insertions(+), 35 deletions(-) > create mode 100644 drivers/vfio/pci/vfio_pci_zdev.c > create mode 100644 include/uapi/linux/vfio_zdev.h > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
On Fri, 6 Sep 2019 20:13:50 -0400 Matthew Rosato <mjrosato@linux.ibm.com> wrote: > From: Pierre Morel <pmorel@linux.ibm.com> > > We define a new device region in vfio.h to be able to > get the ZPCI CLP information by reading this region from > userland. > > We create a new file, vfio_zdev.h to define the structure > of the new region we defined in vfio.h > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > --- > include/uapi/linux/vfio.h | 1 + > include/uapi/linux/vfio_zdev.h | 35 +++++++++++++++++++++++++++++++++++ > 2 files changed, 36 insertions(+) > create mode 100644 include/uapi/linux/vfio_zdev.h > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > index 8f10748..8328c87 100644 > --- a/include/uapi/linux/vfio.h > +++ b/include/uapi/linux/vfio.h > @@ -371,6 +371,7 @@ struct vfio_region_gfx_edid { > * to do TLB invalidation on a GPU. > */ > #define VFIO_REGION_SUBTYPE_IBM_NVLINK2_ATSD (1) > +#define VFIO_REGION_SUBTYPE_ZDEV_CLP (2) Using a subtype is fine, but maybe add a comment what this is for? > > /* > * The MSIX mappable capability informs that MSIX data of a BAR can be mmapped > diff --git a/include/uapi/linux/vfio_zdev.h b/include/uapi/linux/vfio_zdev.h > new file mode 100644 > index 0000000..55e0d6d > --- /dev/null > +++ b/include/uapi/linux/vfio_zdev.h > @@ -0,0 +1,35 @@ > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ > +/* > + * Region definition for ZPCI devices > + * > + * Copyright IBM Corp. 2019 > + * > + * Author(s): Pierre Morel <pmorel@linux.ibm.com> > + */ > + > +#ifndef _VFIO_ZDEV_H_ > +#define _VFIO_ZDEV_H_ > + > +#include <linux/types.h> > + > +/** > + * struct vfio_region_zpci_info - ZPCI information. Hm... probably should also get some more explanation. E.g. is that derived from a hardware structure? > + * > + */ > +struct vfio_region_zpci_info { > + __u64 dasm; > + __u64 start_dma; > + __u64 end_dma; > + __u64 msi_addr; > + __u64 flags; > + __u16 pchid; > + __u16 mui; > + __u16 noi; > + __u16 maxstbl; > + __u8 version; > + __u8 gid; > +#define VFIO_PCI_ZDEV_FLAGS_REFRESH 1 > + __u8 util_str[]; > +} __packed; > + > +#endif _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
On Fri, 6 Sep 2019 20:13:51 -0400 Matthew Rosato <mjrosato@linux.ibm.com> wrote: > From: Pierre Morel <pmorel@linux.ibm.com> > > 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 <pmorel@linux.ibm.com> > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > --- > 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? > 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? > +} > +#endif > + > #endif /* VFIO_PCI_PRIVATE_H */ (...) _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
On 9/19/19 11:20 AM, Cornelia Huck wrote: > On Fri, 6 Sep 2019 20:13:50 -0400 > Matthew Rosato <mjrosato@linux.ibm.com> wrote: > >> From: Pierre Morel <pmorel@linux.ibm.com> >> >> We define a new device region in vfio.h to be able to >> get the ZPCI CLP information by reading this region from >> userland. >> >> We create a new file, vfio_zdev.h to define the structure >> of the new region we defined in vfio.h >> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> >> --- >> include/uapi/linux/vfio.h | 1 + >> include/uapi/linux/vfio_zdev.h | 35 +++++++++++++++++++++++++++++++++++ >> 2 files changed, 36 insertions(+) >> create mode 100644 include/uapi/linux/vfio_zdev.h >> >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h >> index 8f10748..8328c87 100644 >> --- a/include/uapi/linux/vfio.h >> +++ b/include/uapi/linux/vfio.h >> @@ -371,6 +371,7 @@ struct vfio_region_gfx_edid { >> * to do TLB invalidation on a GPU. >> */ >> #define VFIO_REGION_SUBTYPE_IBM_NVLINK2_ATSD (1) >> +#define VFIO_REGION_SUBTYPE_ZDEV_CLP (2) > > Using a subtype is fine, but maybe add a comment what this is for? > Fair point. Maybe something like "IBM ZDEV CLP is used to pass zPCI device features to guest" >> >> /* >> * The MSIX mappable capability informs that MSIX data of a BAR can be mmapped >> diff --git a/include/uapi/linux/vfio_zdev.h b/include/uapi/linux/vfio_zdev.h >> new file mode 100644 >> index 0000000..55e0d6d >> --- /dev/null >> +++ b/include/uapi/linux/vfio_zdev.h >> @@ -0,0 +1,35 @@ >> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ >> +/* >> + * Region definition for ZPCI devices >> + * >> + * Copyright IBM Corp. 2019 >> + * >> + * Author(s): Pierre Morel <pmorel@linux.ibm.com> >> + */ >> + >> +#ifndef _VFIO_ZDEV_H_ >> +#define _VFIO_ZDEV_H_ >> + >> +#include <linux/types.h> >> + >> +/** >> + * struct vfio_region_zpci_info - ZPCI information. > > Hm... probably should also get some more explanation. E.g. is that > derived from a hardware structure? > The structure itself is not mapped 1:1 to a hardware structure, but it does serve as a collection of information that was derived from other hardware structures. "Used for passing hardware feature information about a zpci device between the host and guest" ? >> + * >> + */ >> +struct vfio_region_zpci_info { >> + __u64 dasm; >> + __u64 start_dma; >> + __u64 end_dma; >> + __u64 msi_addr; >> + __u64 flags; >> + __u16 pchid; >> + __u16 mui; >> + __u16 noi; >> + __u16 maxstbl; >> + __u8 version; >> + __u8 gid; >> +#define VFIO_PCI_ZDEV_FLAGS_REFRESH 1 >> + __u8 util_str[]; >> +} __packed; >> + >> +#endif > > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
On 9/19/19 11:25 AM, Cornelia Huck wrote: > On Fri, 6 Sep 2019 20:13:51 -0400 > Matthew Rosato <mjrosato@linux.ibm.com> wrote: > >> From: Pierre Morel <pmorel@linux.ibm.com> >> >> 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 <pmorel@linux.ibm.com> >> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> >> --- >> 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 */ > > (...) > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
On Thu, 19 Sep 2019 16:55:57 -0400 Matthew Rosato <mjrosato@linux.ibm.com> wrote: > On 9/19/19 11:20 AM, Cornelia Huck wrote: > > On Fri, 6 Sep 2019 20:13:50 -0400 > > Matthew Rosato <mjrosato@linux.ibm.com> wrote: > > > >> From: Pierre Morel <pmorel@linux.ibm.com> > >> > >> We define a new device region in vfio.h to be able to > >> get the ZPCI CLP information by reading this region from > >> userland. > >> > >> We create a new file, vfio_zdev.h to define the structure > >> of the new region we defined in vfio.h > >> > >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > >> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > >> --- > >> include/uapi/linux/vfio.h | 1 + > >> include/uapi/linux/vfio_zdev.h | 35 +++++++++++++++++++++++++++++++++++ > >> 2 files changed, 36 insertions(+) > >> create mode 100644 include/uapi/linux/vfio_zdev.h > >> > >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > >> index 8f10748..8328c87 100644 > >> --- a/include/uapi/linux/vfio.h > >> +++ b/include/uapi/linux/vfio.h > >> @@ -371,6 +371,7 @@ struct vfio_region_gfx_edid { > >> * to do TLB invalidation on a GPU. > >> */ > >> #define VFIO_REGION_SUBTYPE_IBM_NVLINK2_ATSD (1) > >> +#define VFIO_REGION_SUBTYPE_ZDEV_CLP (2) > > > > Using a subtype is fine, but maybe add a comment what this is for? > > > > Fair point. Maybe something like "IBM ZDEV CLP is used to pass zPCI > device features to guest" And if you're going to use a PCI vendor ID subtype, maintain consistent naming, VFIO_REGION_SUBTYPE_IBM_ZPCI_CLP or something. Ideally there'd also be a reference to the struct provided through this region otherwise it's rather obscure to find by looking for the call to vfio_pci_register_dev_region() and ops defined for the region. I wouldn't be opposed to defining the region structure here too rather than a separate file, but I guess you're following the example set by ccw. > >> > >> /* > >> * The MSIX mappable capability informs that MSIX data of a BAR can be mmapped > >> diff --git a/include/uapi/linux/vfio_zdev.h b/include/uapi/linux/vfio_zdev.h > >> new file mode 100644 > >> index 0000000..55e0d6d > >> --- /dev/null > >> +++ b/include/uapi/linux/vfio_zdev.h > >> @@ -0,0 +1,35 @@ > >> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ > >> +/* > >> + * Region definition for ZPCI devices > >> + * > >> + * Copyright IBM Corp. 2019 > >> + * > >> + * Author(s): Pierre Morel <pmorel@linux.ibm.com> > >> + */ > >> + > >> +#ifndef _VFIO_ZDEV_H_ > >> +#define _VFIO_ZDEV_H_ > >> + > >> +#include <linux/types.h> > >> + > >> +/** > >> + * struct vfio_region_zpci_info - ZPCI information. > > > > Hm... probably should also get some more explanation. E.g. is that > > derived from a hardware structure? > > > > The structure itself is not mapped 1:1 to a hardware structure, but it > does serve as a collection of information that was derived from other > hardware structures. > > "Used for passing hardware feature information about a zpci device > between the host and guest" ? > > >> + * > >> + */ > >> +struct vfio_region_zpci_info { > >> + __u64 dasm; > >> + __u64 start_dma; > >> + __u64 end_dma; > >> + __u64 msi_addr; > >> + __u64 flags; > >> + __u16 pchid; > >> + __u16 mui; > >> + __u16 noi; > >> + __u16 maxstbl; > >> + __u8 version; > >> + __u8 gid; > >> +#define VFIO_PCI_ZDEV_FLAGS_REFRESH 1 > >> + __u8 util_str[]; > >> +} __packed; > >> + > >> +#endif I'm half tempted to suggest that this struct could be exposed directly through an info capability, the trouble is where. It would be somewhat awkward to pick an arbitrary BAR or config space region to expose this info. The VFIO_DEVICE_GET_INFO ioctl could include it, but we don't support capabilities on that return structure and I'm not sure it's worth implementing versus the solution here. Just a thought. Thanks, Alex _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
On Thu, 19 Sep 2019 16:27:08 -0600 Alex Williamson <alex.williamson@redhat.com> wrote: > On Thu, 19 Sep 2019 16:55:57 -0400 > Matthew Rosato <mjrosato@linux.ibm.com> wrote: > > > On 9/19/19 11:20 AM, Cornelia Huck wrote: > > > On Fri, 6 Sep 2019 20:13:50 -0400 > > > Matthew Rosato <mjrosato@linux.ibm.com> wrote: > > > > > >> From: Pierre Morel <pmorel@linux.ibm.com> > > >> > > >> We define a new device region in vfio.h to be able to > > >> get the ZPCI CLP information by reading this region from > > >> userland. > > >> > > >> We create a new file, vfio_zdev.h to define the structure > > >> of the new region we defined in vfio.h > > >> > > >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > > >> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > > >> --- > > >> include/uapi/linux/vfio.h | 1 + > > >> include/uapi/linux/vfio_zdev.h | 35 +++++++++++++++++++++++++++++++++++ > > >> 2 files changed, 36 insertions(+) > > >> create mode 100644 include/uapi/linux/vfio_zdev.h > > >> > > >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > > >> index 8f10748..8328c87 100644 > > >> --- a/include/uapi/linux/vfio.h > > >> +++ b/include/uapi/linux/vfio.h > > >> @@ -371,6 +371,7 @@ struct vfio_region_gfx_edid { > > >> * to do TLB invalidation on a GPU. > > >> */ > > >> #define VFIO_REGION_SUBTYPE_IBM_NVLINK2_ATSD (1) > > >> +#define VFIO_REGION_SUBTYPE_ZDEV_CLP (2) > > > > > > Using a subtype is fine, but maybe add a comment what this is for? > > > > > > > Fair point. Maybe something like "IBM ZDEV CLP is used to pass zPCI > > device features to guest" > > And if you're going to use a PCI vendor ID subtype, maintain consistent > naming, VFIO_REGION_SUBTYPE_IBM_ZPCI_CLP or something. Ideally there'd > also be a reference to the struct provided through this region > otherwise it's rather obscure to find by looking for the call to > vfio_pci_register_dev_region() and ops defined for the region. I > wouldn't be opposed to defining the region structure here too rather > than a separate file, but I guess you're following the example set by > ccw. > > > >> > > >> /* > > >> * The MSIX mappable capability informs that MSIX data of a BAR can be mmapped > > >> diff --git a/include/uapi/linux/vfio_zdev.h b/include/uapi/linux/vfio_zdev.h > > >> new file mode 100644 > > >> index 0000000..55e0d6d > > >> --- /dev/null > > >> +++ b/include/uapi/linux/vfio_zdev.h > > >> @@ -0,0 +1,35 @@ > > >> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ > > >> +/* > > >> + * Region definition for ZPCI devices > > >> + * > > >> + * Copyright IBM Corp. 2019 > > >> + * > > >> + * Author(s): Pierre Morel <pmorel@linux.ibm.com> > > >> + */ > > >> + > > >> +#ifndef _VFIO_ZDEV_H_ > > >> +#define _VFIO_ZDEV_H_ > > >> + > > >> +#include <linux/types.h> > > >> + > > >> +/** > > >> + * struct vfio_region_zpci_info - ZPCI information. > > > > > > Hm... probably should also get some more explanation. E.g. is that > > > derived from a hardware structure? > > > > > > > The structure itself is not mapped 1:1 to a hardware structure, but it > > does serve as a collection of information that was derived from other > > hardware structures. > > > > "Used for passing hardware feature information about a zpci device > > between the host and guest" ? > > > > >> + * > > >> + */ > > >> +struct vfio_region_zpci_info { > > >> + __u64 dasm; > > >> + __u64 start_dma; > > >> + __u64 end_dma; > > >> + __u64 msi_addr; > > >> + __u64 flags; > > >> + __u16 pchid; > > >> + __u16 mui; > > >> + __u16 noi; > > >> + __u16 maxstbl; > > >> + __u8 version; > > >> + __u8 gid; > > >> +#define VFIO_PCI_ZDEV_FLAGS_REFRESH 1 Why is this defined so far away from the flags field? I thought it was lost at first. I also wonder what it means... brief descriptions? Thanks, Alex > > >> + __u8 util_str[]; > > >> +} __packed; > > >> + > > >> +#endif > > I'm half tempted to suggest that this struct could be exposed directly > through an info capability, the trouble is where. It would be somewhat > awkward to pick an arbitrary BAR or config space region to expose this > info. The VFIO_DEVICE_GET_INFO ioctl could include it, but we don't > support capabilities on that return structure and I'm not sure it's > worth implementing versus the solution here. Just a thought. Thanks, > > Alex _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
On Fri, 6 Sep 2019 20:13:51 -0400 Matthew Rosato <mjrosato@linux.ibm.com> wrote: > From: Pierre Morel <pmorel@linux.ibm.com> > > 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 <pmorel@linux.ibm.com> > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > --- > 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 <pmorel@linux.ibm.com> > + * > + * 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; > +} _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
On Thu, 19 Sep 2019 16:55:57 -0400 Matthew Rosato <mjrosato@linux.ibm.com> wrote: > On 9/19/19 11:20 AM, Cornelia Huck wrote: > > On Fri, 6 Sep 2019 20:13:50 -0400 > > Matthew Rosato <mjrosato@linux.ibm.com> wrote: > > > >> From: Pierre Morel <pmorel@linux.ibm.com> > >> > >> We define a new device region in vfio.h to be able to > >> get the ZPCI CLP information by reading this region from > >> userland. > >> > >> We create a new file, vfio_zdev.h to define the structure > >> of the new region we defined in vfio.h > >> > >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > >> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > >> --- > >> include/uapi/linux/vfio.h | 1 + > >> include/uapi/linux/vfio_zdev.h | 35 +++++++++++++++++++++++++++++++++++ > >> 2 files changed, 36 insertions(+) > >> create mode 100644 include/uapi/linux/vfio_zdev.h > >> diff --git a/include/uapi/linux/vfio_zdev.h b/include/uapi/linux/vfio_zdev.h > >> new file mode 100644 > >> index 0000000..55e0d6d > >> --- /dev/null > >> +++ b/include/uapi/linux/vfio_zdev.h > >> @@ -0,0 +1,35 @@ > >> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ > >> +/* > >> + * Region definition for ZPCI devices > >> + * > >> + * Copyright IBM Corp. 2019 > >> + * > >> + * Author(s): Pierre Morel <pmorel@linux.ibm.com> > >> + */ > >> + > >> +#ifndef _VFIO_ZDEV_H_ > >> +#define _VFIO_ZDEV_H_ > >> + > >> +#include <linux/types.h> > >> + > >> +/** > >> + * struct vfio_region_zpci_info - ZPCI information. > > > > Hm... probably should also get some more explanation. E.g. is that > > derived from a hardware structure? > > > > The structure itself is not mapped 1:1 to a hardware structure, but it > does serve as a collection of information that was derived from other > hardware structures. > > "Used for passing hardware feature information about a zpci device > between the host and guest" ? "zPCI specific hardware feature information for a device"? Are we reasonably sure that this is complete for now? I'm not sure if expanding this structure would work; adding another should always be possible, though (if a bit annoying). > > >> + * > >> + */ > >> +struct vfio_region_zpci_info { > >> + __u64 dasm; > >> + __u64 start_dma; > >> + __u64 end_dma; > >> + __u64 msi_addr; > >> + __u64 flags; > >> + __u16 pchid; > >> + __u16 mui; > >> + __u16 noi; > >> + __u16 maxstbl; > >> + __u8 version; > >> + __u8 gid; > >> +#define VFIO_PCI_ZDEV_FLAGS_REFRESH 1 > >> + __u8 util_str[]; > >> +} __packed; > >> + > >> +#endif > > > > > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
On Thu, 19 Sep 2019 16:57:10 -0400 Matthew Rosato <mjrosato@linux.ibm.com> wrote: > On 9/19/19 11:25 AM, Cornelia Huck wrote: > > On Fri, 6 Sep 2019 20:13:51 -0400 > > Matthew Rosato <mjrosato@linux.ibm.com> wrote: > > > >> From: Pierre Morel <pmorel@linux.ibm.com> > >> > >> 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 <pmorel@linux.ibm.com> > >> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > >> --- > >> 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." ? _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
On 9/19/19 6:49 PM, Alex Williamson wrote: > On Thu, 19 Sep 2019 16:27:08 -0600 > Alex Williamson <alex.williamson@redhat.com> wrote: > >> On Thu, 19 Sep 2019 16:55:57 -0400 >> Matthew Rosato <mjrosato@linux.ibm.com> wrote: >> >>> On 9/19/19 11:20 AM, Cornelia Huck wrote: >>>> On Fri, 6 Sep 2019 20:13:50 -0400 >>>> Matthew Rosato <mjrosato@linux.ibm.com> wrote: >>>> >>>>> From: Pierre Morel <pmorel@linux.ibm.com> >>>>> >>>>> We define a new device region in vfio.h to be able to >>>>> get the ZPCI CLP information by reading this region from >>>>> userland. >>>>> >>>>> We create a new file, vfio_zdev.h to define the structure >>>>> of the new region we defined in vfio.h >>>>> >>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>>>> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> >>>>> --- >>>>> include/uapi/linux/vfio.h | 1 + >>>>> include/uapi/linux/vfio_zdev.h | 35 +++++++++++++++++++++++++++++++++++ >>>>> 2 files changed, 36 insertions(+) >>>>> create mode 100644 include/uapi/linux/vfio_zdev.h >>>>> >>>>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h >>>>> index 8f10748..8328c87 100644 >>>>> --- a/include/uapi/linux/vfio.h >>>>> +++ b/include/uapi/linux/vfio.h >>>>> @@ -371,6 +371,7 @@ struct vfio_region_gfx_edid { >>>>> * to do TLB invalidation on a GPU. >>>>> */ >>>>> #define VFIO_REGION_SUBTYPE_IBM_NVLINK2_ATSD (1) >>>>> +#define VFIO_REGION_SUBTYPE_ZDEV_CLP (2) >>>> >>>> Using a subtype is fine, but maybe add a comment what this is for? >>>> >>> >>> Fair point. Maybe something like "IBM ZDEV CLP is used to pass zPCI >>> device features to guest" >> >> And if you're going to use a PCI vendor ID subtype, maintain consistent >> naming, VFIO_REGION_SUBTYPE_IBM_ZPCI_CLP or something. Ideally there'd >> also be a reference to the struct provided through this region >> otherwise it's rather obscure to find by looking for the call to >> vfio_pci_register_dev_region() and ops defined for the region. I Sure, will rename and add reference >> wouldn't be opposed to defining the region structure here too rather >> than a separate file, but I guess you're following the example set by >> ccw. >> Indeed. >>>>> >>>>> /* >>>>> * The MSIX mappable capability informs that MSIX data of a BAR can be mmapped >>>>> diff --git a/include/uapi/linux/vfio_zdev.h b/include/uapi/linux/vfio_zdev.h >>>>> new file mode 100644 >>>>> index 0000000..55e0d6d >>>>> --- /dev/null >>>>> +++ b/include/uapi/linux/vfio_zdev.h >>>>> @@ -0,0 +1,35 @@ >>>>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ >>>>> +/* >>>>> + * Region definition for ZPCI devices >>>>> + * >>>>> + * Copyright IBM Corp. 2019 >>>>> + * >>>>> + * Author(s): Pierre Morel <pmorel@linux.ibm.com> >>>>> + */ >>>>> + >>>>> +#ifndef _VFIO_ZDEV_H_ >>>>> +#define _VFIO_ZDEV_H_ >>>>> + >>>>> +#include <linux/types.h> >>>>> + >>>>> +/** >>>>> + * struct vfio_region_zpci_info - ZPCI information. >>>> >>>> Hm... probably should also get some more explanation. E.g. is that >>>> derived from a hardware structure? >>>> >>> >>> The structure itself is not mapped 1:1 to a hardware structure, but it >>> does serve as a collection of information that was derived from other >>> hardware structures. >>> >>> "Used for passing hardware feature information about a zpci device >>> between the host and guest" ? >>> >>>>> + * >>>>> + */ >>>>> +struct vfio_region_zpci_info { >>>>> + __u64 dasm; >>>>> + __u64 start_dma; >>>>> + __u64 end_dma; >>>>> + __u64 msi_addr; >>>>> + __u64 flags; >>>>> + __u16 pchid; >>>>> + __u16 mui; >>>>> + __u16 noi; >>>>> + __u16 maxstbl; >>>>> + __u8 version; >>>>> + __u8 gid; >>>>> +#define VFIO_PCI_ZDEV_FLAGS_REFRESH 1 > > Why is this defined so far away from the flags field? I thought it was > lost at first. I also wonder what it means... brief descriptions? > Thanks, > Not sure why Pierre chose to put it here, but I have no issues moving it up beneath flags. Otherwise, I am getting the general gist of the feedback: more comments to explain what this is doing. > Alex > >>>>> + __u8 util_str[]; >>>>> +} __packed; >>>>> + >>>>> +#endif >> >> I'm half tempted to suggest that this struct could be exposed directly >> through an info capability, the trouble is where. It would be somewhat >> awkward to pick an arbitrary BAR or config space region to expose this >> info. The VFIO_DEVICE_GET_INFO ioctl could include it, but we don't >> support capabilities on that return structure and I'm not sure it's >> worth implementing versus the solution here. Just a thought. Thanks, >> >> Alex > > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
On 9/19/19 6:57 PM, Alex Williamson wrote: > On Fri, 6 Sep 2019 20:13:51 -0400 > Matthew Rosato <mjrosato@linux.ibm.com> wrote: > >> From: Pierre Morel <pmorel@linux.ibm.com> >> >> 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 <pmorel@linux.ibm.com> >> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> >> --- >> 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 <pmorel@linux.ibm.com> >> + * >> + * 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; >> +} > > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
On 9/20/19 10:02 AM, Cornelia Huck wrote: > On Thu, 19 Sep 2019 16:55:57 -0400 > Matthew Rosato <mjrosato@linux.ibm.com> wrote: > >> On 9/19/19 11:20 AM, Cornelia Huck wrote: >>> On Fri, 6 Sep 2019 20:13:50 -0400 >>> Matthew Rosato <mjrosato@linux.ibm.com> wrote: >>> >>>> From: Pierre Morel <pmorel@linux.ibm.com> >>>> >>>> We define a new device region in vfio.h to be able to >>>> get the ZPCI CLP information by reading this region from >>>> userland. >>>> >>>> We create a new file, vfio_zdev.h to define the structure >>>> of the new region we defined in vfio.h >>>> >>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>>> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> >>>> --- >>>> include/uapi/linux/vfio.h | 1 + >>>> include/uapi/linux/vfio_zdev.h | 35 +++++++++++++++++++++++++++++++++++ >>>> 2 files changed, 36 insertions(+) >>>> create mode 100644 include/uapi/linux/vfio_zdev.h > >>>> diff --git a/include/uapi/linux/vfio_zdev.h b/include/uapi/linux/vfio_zdev.h >>>> new file mode 100644 >>>> index 0000000..55e0d6d >>>> --- /dev/null >>>> +++ b/include/uapi/linux/vfio_zdev.h >>>> @@ -0,0 +1,35 @@ >>>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ >>>> +/* >>>> + * Region definition for ZPCI devices >>>> + * >>>> + * Copyright IBM Corp. 2019 >>>> + * >>>> + * Author(s): Pierre Morel <pmorel@linux.ibm.com> >>>> + */ >>>> + >>>> +#ifndef _VFIO_ZDEV_H_ >>>> +#define _VFIO_ZDEV_H_ >>>> + >>>> +#include <linux/types.h> >>>> + >>>> +/** >>>> + * struct vfio_region_zpci_info - ZPCI information. >>> >>> Hm... probably should also get some more explanation. E.g. is that >>> derived from a hardware structure? >>> >> >> The structure itself is not mapped 1:1 to a hardware structure, but it >> does serve as a collection of information that was derived from other >> hardware structures. >> >> "Used for passing hardware feature information about a zpci device >> between the host and guest" ? > > "zPCI specific hardware feature information for a device"? > > Are we reasonably sure that this is complete for now? I'm not sure if > expanding this structure would work; adding another should always be > possible, though (if a bit annoying). > I think trying to make the structure expandable would be best... If we allow arbitrary-sized reads of the info, and only add new fields onto the end it should be OK, no? (older qemu doesn't get the info it doesn't ask for / understand).... But I guess that's not compatible with having util_str[] size being defined dynamically. Another caveat would be if CLP_UTIL_STR_LEN were to grow in size in the future, and assuming util_str[] was no longer at the end of the structure, I guess the additional data would have to end up in a util_str2[CLP_UTIL_STR_LEN_NEW-CLP_UTIL_STR_LEN_OLD]... To explain what I mean, something like: struct vfio_region_zpci_info { <..> __u8 util_str[CLP_UTIL_STR_LEN_OLD]; /* END OF V1 */ __u8 foo; /* END OF V2 */ __u8 util_str2[CLP_UTIL_STR_LEN_NEW-CLP_UTIL_STR_LEN_OLD]; /* END OF V3 */ } __packed; >> >>>> + * >>>> + */ >>>> +struct vfio_region_zpci_info { >>>> + __u64 dasm; >>>> + __u64 start_dma; >>>> + __u64 end_dma; >>>> + __u64 msi_addr; >>>> + __u64 flags; >>>> + __u16 pchid; >>>> + __u16 mui; >>>> + __u16 noi; >>>> + __u16 maxstbl; >>>> + __u8 version; >>>> + __u8 gid; >>>> +#define VFIO_PCI_ZDEV_FLAGS_REFRESH 1 >>>> + __u8 util_str[]; >>>> +} __packed; >>>> + >>>> +#endif >>> >>> >> > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
On 9/20/19 10:26 AM, Cornelia Huck wrote: > On Thu, 19 Sep 2019 16:57:10 -0400 > Matthew Rosato <mjrosato@linux.ibm.com> wrote: > >> On 9/19/19 11:25 AM, Cornelia Huck wrote: >>> On Fri, 6 Sep 2019 20:13:51 -0400 >>> Matthew Rosato <mjrosato@linux.ibm.com> wrote: >>> >>>> From: Pierre Morel <pmorel@linux.ibm.com> >>>> >>>> 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 <pmorel@linux.ibm.com> >>>> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> >>>> --- >>>> 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! _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
On Fri, 20 Sep 2019 11:14:28 -0400 Matthew Rosato <mjrosato@linux.ibm.com> wrote: > On 9/20/19 10:02 AM, Cornelia Huck wrote: > > On Thu, 19 Sep 2019 16:55:57 -0400 > > Matthew Rosato <mjrosato@linux.ibm.com> wrote: > > > >> On 9/19/19 11:20 AM, Cornelia Huck wrote: > >>> On Fri, 6 Sep 2019 20:13:50 -0400 > >>> Matthew Rosato <mjrosato@linux.ibm.com> wrote: > >>> > >>>> From: Pierre Morel <pmorel@linux.ibm.com> > >>>> > >>>> We define a new device region in vfio.h to be able to > >>>> get the ZPCI CLP information by reading this region from > >>>> userland. > >>>> > >>>> We create a new file, vfio_zdev.h to define the structure > >>>> of the new region we defined in vfio.h > >>>> > >>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > >>>> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > >>>> --- > >>>> include/uapi/linux/vfio.h | 1 + > >>>> include/uapi/linux/vfio_zdev.h | 35 +++++++++++++++++++++++++++++++++++ > >>>> 2 files changed, 36 insertions(+) > >>>> create mode 100644 include/uapi/linux/vfio_zdev.h > > > >>>> diff --git a/include/uapi/linux/vfio_zdev.h b/include/uapi/linux/vfio_zdev.h > >>>> new file mode 100644 > >>>> index 0000000..55e0d6d > >>>> --- /dev/null > >>>> +++ b/include/uapi/linux/vfio_zdev.h > >>>> @@ -0,0 +1,35 @@ > >>>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ > >>>> +/* > >>>> + * Region definition for ZPCI devices > >>>> + * > >>>> + * Copyright IBM Corp. 2019 > >>>> + * > >>>> + * Author(s): Pierre Morel <pmorel@linux.ibm.com> > >>>> + */ > >>>> + > >>>> +#ifndef _VFIO_ZDEV_H_ > >>>> +#define _VFIO_ZDEV_H_ > >>>> + > >>>> +#include <linux/types.h> > >>>> + > >>>> +/** > >>>> + * struct vfio_region_zpci_info - ZPCI information. > >>> > >>> Hm... probably should also get some more explanation. E.g. is that > >>> derived from a hardware structure? > >>> > >> > >> The structure itself is not mapped 1:1 to a hardware structure, but it > >> does serve as a collection of information that was derived from other > >> hardware structures. > >> > >> "Used for passing hardware feature information about a zpci device > >> between the host and guest" ? > > > > "zPCI specific hardware feature information for a device"? > > > > Are we reasonably sure that this is complete for now? I'm not sure if > > expanding this structure would work; adding another should always be > > possible, though (if a bit annoying). > > > > I think trying to make the structure expandable would be best... If we > allow arbitrary-sized reads of the info, and only add new fields onto > the end it should be OK, no? (older qemu doesn't get the info it doesn't > ask for / understand).... But I guess that's not compatible with having > util_str[] size being defined dynamically. Another caveat would be if > CLP_UTIL_STR_LEN were to grow in size in the future, and assuming > util_str[] was no longer at the end of the structure, I guess the > additional data would have to end up in a > util_str2[CLP_UTIL_STR_LEN_NEW-CLP_UTIL_STR_LEN_OLD]... To explain what > I mean, something like: > > struct vfio_region_zpci_info { > <..> > __u8 util_str[CLP_UTIL_STR_LEN_OLD]; > /* END OF V1 */ > __u8 foo; > /* END OF V2 */ > __u8 util_str2[CLP_UTIL_STR_LEN_NEW-CLP_UTIL_STR_LEN_OLD]; > /* END OF V3 */ > } __packed; [Sorry about the late response -- was on PTO] That sounds a bit too complicated to me, and I'd prefer the "add another region if we missed something" approach. If we put anything looking potentially useful in here now, that "add another region" event is hopefully far in the future. > > > >> > >>>> + * > >>>> + */ > >>>> +struct vfio_region_zpci_info { > >>>> + __u64 dasm; > >>>> + __u64 start_dma; > >>>> + __u64 end_dma; > >>>> + __u64 msi_addr; > >>>> + __u64 flags; > >>>> + __u16 pchid; > >>>> + __u16 mui; > >>>> + __u16 noi; > >>>> + __u16 maxstbl; > >>>> + __u8 version; > >>>> + __u8 gid; > >>>> +#define VFIO_PCI_ZDEV_FLAGS_REFRESH 1 > >>>> + __u8 util_str[]; > >>>> +} __packed; > >>>> + > >>>> +#endif > >>> > >>> > >> > > > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu