* [PATCH v4 1/4] s390: pci: Exporting access to CLP PCI function and PCI group
2019-09-07 0:13 [PATCH v4 0/4] Retrieving zPCI specific info with VFIO Matthew Rosato
@ 2019-09-07 0:13 ` Matthew Rosato
2019-09-07 0:13 ` [PATCH v4 2/4] s390: pci: Define the maxstbl CLP response entry Matthew Rosato
` (3 subsequent siblings)
4 siblings, 0 replies; 20+ messages in thread
From: Matthew Rosato @ 2019-09-07 0:13 UTC (permalink / raw)
To: sebott
Cc: linux-s390, walling, alex.williamson, gor, kvm, pmorel, cohuck,
heiko.carstens, linux-kernel, pasic, borntraeger, iommu,
robin.murphy, gerald.schaefer
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
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v4 2/4] s390: pci: Define the maxstbl CLP response entry
2019-09-07 0:13 [PATCH v4 0/4] Retrieving zPCI specific info with VFIO Matthew Rosato
2019-09-07 0:13 ` [PATCH v4 1/4] s390: pci: Exporting access to CLP PCI function and PCI group Matthew Rosato
@ 2019-09-07 0:13 ` Matthew Rosato
2019-09-07 0:13 ` [PATCH v4 3/4] vfio: zpci: defining the VFIO headers Matthew Rosato
` (2 subsequent siblings)
4 siblings, 0 replies; 20+ messages in thread
From: Matthew Rosato @ 2019-09-07 0:13 UTC (permalink / raw)
To: sebott
Cc: linux-s390, walling, alex.williamson, gor, kvm, pmorel, cohuck,
heiko.carstens, linux-kernel, pasic, borntraeger, iommu,
robin.murphy, gerald.schaefer
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
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v4 3/4] vfio: zpci: defining the VFIO headers
2019-09-07 0:13 [PATCH v4 0/4] Retrieving zPCI specific info with VFIO Matthew Rosato
2019-09-07 0:13 ` [PATCH v4 1/4] s390: pci: Exporting access to CLP PCI function and PCI group Matthew Rosato
2019-09-07 0:13 ` [PATCH v4 2/4] s390: pci: Define the maxstbl CLP response entry Matthew Rosato
@ 2019-09-07 0:13 ` Matthew Rosato
2019-09-19 15:20 ` Cornelia Huck
2019-09-07 0:13 ` [PATCH v4 4/4] vfio: pci: Using a device region to retrieve zPCI information Matthew Rosato
2019-09-19 1:36 ` [PATCH v4 0/4] Retrieving zPCI specific info with VFIO Matthew Rosato
4 siblings, 1 reply; 20+ messages in thread
From: Matthew Rosato @ 2019-09-07 0:13 UTC (permalink / raw)
To: sebott
Cc: linux-s390, walling, alex.williamson, gor, kvm, pmorel, cohuck,
heiko.carstens, linux-kernel, pasic, borntraeger, iommu,
robin.murphy, gerald.schaefer
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
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 3/4] vfio: zpci: defining the VFIO headers
2019-09-07 0:13 ` [PATCH v4 3/4] vfio: zpci: defining the VFIO headers Matthew Rosato
@ 2019-09-19 15:20 ` Cornelia Huck
2019-09-19 20:55 ` Matthew Rosato
0 siblings, 1 reply; 20+ messages in thread
From: Cornelia Huck @ 2019-09-19 15:20 UTC (permalink / raw)
To: Matthew Rosato
Cc: gor, linux-s390, walling, alex.williamson, kvm, sebott, pmorel,
heiko.carstens, linux-kernel, pasic, borntraeger, iommu,
robin.murphy, gerald.schaefer
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 3/4] vfio: zpci: defining the VFIO headers
2019-09-19 15:20 ` Cornelia Huck
@ 2019-09-19 20:55 ` Matthew Rosato
2019-09-19 22:27 ` Alex Williamson
2019-09-20 14:02 ` Cornelia Huck
0 siblings, 2 replies; 20+ messages in thread
From: Matthew Rosato @ 2019-09-19 20:55 UTC (permalink / raw)
To: Cornelia Huck
Cc: gor, linux-s390, walling, alex.williamson, kvm, sebott, pmorel,
heiko.carstens, linux-kernel, pasic, borntraeger, iommu,
robin.murphy, gerald.schaefer
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 3/4] vfio: zpci: defining the VFIO headers
2019-09-19 20:55 ` Matthew Rosato
@ 2019-09-19 22:27 ` Alex Williamson
2019-09-19 22:49 ` Alex Williamson
2019-09-20 14:02 ` Cornelia Huck
1 sibling, 1 reply; 20+ messages in thread
From: Alex Williamson @ 2019-09-19 22:27 UTC (permalink / raw)
To: Matthew Rosato
Cc: gor, linux-s390, walling, kvm, sebott, pmorel, Cornelia Huck,
heiko.carstens, linux-kernel, pasic, borntraeger, iommu,
robin.murphy, gerald.schaefer
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 3/4] vfio: zpci: defining the VFIO headers
2019-09-19 22:27 ` Alex Williamson
@ 2019-09-19 22:49 ` Alex Williamson
2019-09-20 14:46 ` Matthew Rosato
0 siblings, 1 reply; 20+ messages in thread
From: Alex Williamson @ 2019-09-19 22:49 UTC (permalink / raw)
To: Matthew Rosato
Cc: gor, linux-s390, walling, kvm, sebott, pmorel, Cornelia Huck,
heiko.carstens, linux-kernel, pasic, borntraeger, iommu,
robin.murphy, gerald.schaefer
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 3/4] vfio: zpci: defining the VFIO headers
2019-09-19 22:49 ` Alex Williamson
@ 2019-09-20 14:46 ` Matthew Rosato
0 siblings, 0 replies; 20+ messages in thread
From: Matthew Rosato @ 2019-09-20 14:46 UTC (permalink / raw)
To: Alex Williamson
Cc: gor, linux-s390, walling, kvm, sebott, pmorel, Cornelia Huck,
heiko.carstens, linux-kernel, pasic, borntraeger, iommu,
robin.murphy, gerald.schaefer
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 3/4] vfio: zpci: defining the VFIO headers
2019-09-19 20:55 ` Matthew Rosato
2019-09-19 22:27 ` Alex Williamson
@ 2019-09-20 14:02 ` Cornelia Huck
2019-09-20 15:14 ` Matthew Rosato
1 sibling, 1 reply; 20+ messages in thread
From: Cornelia Huck @ 2019-09-20 14:02 UTC (permalink / raw)
To: Matthew Rosato
Cc: gor, linux-s390, walling, alex.williamson, kvm, sebott, pmorel,
heiko.carstens, linux-kernel, pasic, borntraeger, iommu,
robin.murphy, gerald.schaefer
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 3/4] vfio: zpci: defining the VFIO headers
2019-09-20 14:02 ` Cornelia Huck
@ 2019-09-20 15:14 ` Matthew Rosato
2019-10-08 13:30 ` Cornelia Huck
0 siblings, 1 reply; 20+ messages in thread
From: Matthew Rosato @ 2019-09-20 15:14 UTC (permalink / raw)
To: Cornelia Huck
Cc: gor, linux-s390, walling, alex.williamson, kvm, sebott, pmorel,
heiko.carstens, linux-kernel, pasic, borntraeger, iommu,
robin.murphy, gerald.schaefer
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 3/4] vfio: zpci: defining the VFIO headers
2019-09-20 15:14 ` Matthew Rosato
@ 2019-10-08 13:30 ` Cornelia Huck
0 siblings, 0 replies; 20+ messages in thread
From: Cornelia Huck @ 2019-10-08 13:30 UTC (permalink / raw)
To: Matthew Rosato
Cc: gor, linux-s390, walling, alex.williamson, kvm, sebott, pmorel,
heiko.carstens, linux-kernel, pasic, borntraeger, iommu,
robin.murphy, gerald.schaefer
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 4/4] vfio: pci: Using a device region to retrieve zPCI information
2019-09-07 0:13 [PATCH v4 0/4] Retrieving zPCI specific info with VFIO Matthew Rosato
` (2 preceding siblings ...)
2019-09-07 0:13 ` [PATCH v4 3/4] vfio: zpci: defining the VFIO headers Matthew Rosato
@ 2019-09-07 0:13 ` Matthew Rosato
2019-09-19 15:25 ` Cornelia Huck
2019-09-19 22:57 ` Alex Williamson
2019-09-19 1:36 ` [PATCH v4 0/4] Retrieving zPCI specific info with VFIO Matthew Rosato
4 siblings, 2 replies; 20+ messages in thread
From: Matthew Rosato @ 2019-09-07 0:13 UTC (permalink / raw)
To: sebott
Cc: linux-s390, walling, alex.williamson, gor, kvm, pmorel, cohuck,
heiko.carstens, linux-kernel, pasic, borntraeger, iommu,
robin.murphy, gerald.schaefer
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
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 4/4] vfio: pci: Using a device region to retrieve zPCI information
2019-09-07 0:13 ` [PATCH v4 4/4] vfio: pci: Using a device region to retrieve zPCI information Matthew Rosato
@ 2019-09-19 15:25 ` Cornelia Huck
2019-09-19 20:57 ` Matthew Rosato
2019-09-19 22:57 ` Alex Williamson
1 sibling, 1 reply; 20+ messages in thread
From: Cornelia Huck @ 2019-09-19 15:25 UTC (permalink / raw)
To: Matthew Rosato
Cc: gor, linux-s390, walling, alex.williamson, kvm, sebott, pmorel,
heiko.carstens, linux-kernel, pasic, borntraeger, iommu,
robin.murphy, gerald.schaefer
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 4/4] vfio: pci: Using a device region to retrieve zPCI information
2019-09-19 15:25 ` Cornelia Huck
@ 2019-09-19 20:57 ` Matthew Rosato
2019-09-20 14:26 ` Cornelia Huck
0 siblings, 1 reply; 20+ messages in thread
From: Matthew Rosato @ 2019-09-19 20:57 UTC (permalink / raw)
To: Cornelia Huck
Cc: gor, linux-s390, walling, alex.williamson, kvm, sebott, pmorel,
heiko.carstens, linux-kernel, pasic, borntraeger, iommu,
robin.murphy, gerald.schaefer
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 4/4] vfio: pci: Using a device region to retrieve zPCI information
2019-09-19 20:57 ` Matthew Rosato
@ 2019-09-20 14:26 ` Cornelia Huck
2019-09-20 15:53 ` Matthew Rosato
0 siblings, 1 reply; 20+ messages in thread
From: Cornelia Huck @ 2019-09-20 14:26 UTC (permalink / raw)
To: Matthew Rosato
Cc: gor, linux-s390, walling, alex.williamson, kvm, sebott, pmorel,
heiko.carstens, linux-kernel, pasic, borntraeger, iommu,
robin.murphy, gerald.schaefer
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 4/4] vfio: pci: Using a device region to retrieve zPCI information
2019-09-20 14:26 ` Cornelia Huck
@ 2019-09-20 15:53 ` Matthew Rosato
0 siblings, 0 replies; 20+ messages in thread
From: Matthew Rosato @ 2019-09-20 15:53 UTC (permalink / raw)
To: Cornelia Huck
Cc: gor, linux-s390, walling, alex.williamson, kvm, sebott, pmorel,
heiko.carstens, linux-kernel, pasic, borntraeger, iommu,
robin.murphy, gerald.schaefer
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 4/4] vfio: pci: Using a device region to retrieve zPCI information
2019-09-07 0:13 ` [PATCH v4 4/4] vfio: pci: Using a device region to retrieve zPCI information Matthew Rosato
2019-09-19 15:25 ` Cornelia Huck
@ 2019-09-19 22:57 ` Alex Williamson
2019-09-20 14:57 ` Matthew Rosato
1 sibling, 1 reply; 20+ messages in thread
From: Alex Williamson @ 2019-09-19 22:57 UTC (permalink / raw)
To: Matthew Rosato
Cc: gor, linux-s390, walling, kvm, sebott, pmorel, cohuck,
heiko.carstens, linux-kernel, pasic, borntraeger, iommu,
robin.murphy, gerald.schaefer
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 4/4] vfio: pci: Using a device region to retrieve zPCI information
2019-09-19 22:57 ` Alex Williamson
@ 2019-09-20 14:57 ` Matthew Rosato
0 siblings, 0 replies; 20+ messages in thread
From: Matthew Rosato @ 2019-09-20 14:57 UTC (permalink / raw)
To: Alex Williamson
Cc: gor, linux-s390, walling, kvm, sebott, pmorel, cohuck,
heiko.carstens, linux-kernel, pasic, borntraeger, iommu,
robin.murphy, gerald.schaefer
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 0/4] Retrieving zPCI specific info with VFIO
2019-09-07 0:13 [PATCH v4 0/4] Retrieving zPCI specific info with VFIO Matthew Rosato
` (3 preceding siblings ...)
2019-09-07 0:13 ` [PATCH v4 4/4] vfio: pci: Using a device region to retrieve zPCI information Matthew Rosato
@ 2019-09-19 1:36 ` Matthew Rosato
4 siblings, 0 replies; 20+ messages in thread
From: Matthew Rosato @ 2019-09-19 1:36 UTC (permalink / raw)
To: sebott
Cc: linux-s390, walling, alex.williamson, gor, kvm, pmorel, cohuck,
heiko.carstens, linux-kernel, pasic, borntraeger, iommu,
robin.murphy, gerald.schaefer
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
^ permalink raw reply [flat|nested] 20+ messages in thread