[PATCH 0/4] Retrieving zPCI specific info with VFIO

[PATCH 0/4] Retrieving zPCI specific info with VFIO

[Pierre Morel]

Using the PCI VFIO interface allows userland, a.k.a. QEMU, to retrieve
ZPCI specific information without knowing Z specific identifiers
like the function ID or the function handle of the zPCI function
hidden behind the PCI interface. 
  
By using the VFIO_IOMMU_GET_INFO ioctl we enter the vfio_iommu_type1
ioctl callback and can insert there the treatment for a new Z specific
capability.
  
Once in vfio_iommu_type1 we can retrieve the real iommu device,
s390_iommu and call the get_attr iommu operation's callback
in which we can retrieve the zdev device and start clp operations
to retrieve Z specific values the guest driver is concerned with.
 
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.

These two functions are exported to be used from out of the s390_iommu
code.

Pierre Morel (4):
  s390: pci: Exporting access to CLP PCI function and PCI group
  vfio: vfio_iommu_type1: Define VFIO_IOMMU_INFO_CAPABILITIES
  s390: iommu: Adding get attributes for s390_iommu
  vfio: vfio_iommu_type1: implement VFIO_IOMMU_INFO_CAPABILITIES

 arch/s390/include/asm/pci.h     |  3 ++
 arch/s390/pci/pci_clp.c         | 72 ++++++++++++++++---------------
 drivers/iommu/s390-iommu.c      | 77 +++++++++++++++++++++++++++++++++
 drivers/vfio/vfio_iommu_type1.c | 95 ++++++++++++++++++++++++++++++++++++++++-
 include/linux/iommu.h           |  4 ++
 include/uapi/linux/vfio.h       | 10 +++++
 6 files changed, 226 insertions(+), 35 deletions(-)

-- 
2.7.4

[PATCH 1/4] s390: pci: Exporting access to CLP PCI function and PCI group

[Pierre Morel]

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     | 72 ++++++++++++++++++++++++---------------------
 2 files changed, 41 insertions(+), 34 deletions(-)

diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
index 305befd..e66b246 100644
--- a/arch/s390/include/asm/pci.h
+++ b/arch/s390/include/asm/pci.h
@@ -261,4 +261,7 @@ cpumask_of_pcibus(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 3a36b07..4ae5d77 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,50 @@ 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);
+}
+EXPORT_SYMBOL(zdev_query_pci_fn);
+
+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 +223,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;
 
-- 
2.7.4

[PATCH 2/4] vfio: vfio_iommu_type1: Define VFIO_IOMMU_INFO_CAPABILITIES

[Pierre Morel]

To use the VFIO_IOMMU_GET_INFO to retrieve IOMMU specific information,
we define a new flag VFIO_IOMMU_INFO_CAPABILITIES in the
vfio_iommu_type1_info structure and the associated capability
information block.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 include/uapi/linux/vfio.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 8f10748..8f68e0f 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -715,6 +715,16 @@ struct vfio_iommu_type1_info {
 	__u32	flags;
 #define VFIO_IOMMU_INFO_PGSIZES (1 << 0)	/* supported page sizes info */
 	__u64	iova_pgsizes;		/* Bitmap of supported page sizes */
+#define VFIO_IOMMU_INFO_CAPABILITIES (1 << 1)  /* support capabilities info */
+	__u64   cap_offset;     /* Offset within info struct of first cap */
+};
+
+#define VFIO_IOMMU_INFO_CAP_QFN		1
+#define VFIO_IOMMU_INFO_CAP_QGRP	2
+
+struct vfio_iommu_type1_info_block {
+	struct vfio_info_cap_header header;
+	__u32 data[];
 };
 
 #define VFIO_IOMMU_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
-- 
2.7.4

[PATCH 3/4] s390: iommu: Adding get attributes for s390_iommu

[Pierre Morel]

We add "get attributes" to the S390 iommu operations to retrieve the S390
specific attributes through the call of zPCI dedicated CLP functions.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 drivers/iommu/s390-iommu.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/iommu.h      |  4 +++
 2 files changed, 81 insertions(+)

diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index 22d4db3..98082f0 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -363,6 +363,82 @@ void zpci_destroy_iommu(struct zpci_dev *zdev)
 	iommu_device_sysfs_remove(&zdev->iommu_dev);
 }
 
+struct zpci_dev *get_zpci(struct s390_domain *s390_domain)
+{
+	struct s390_domain_device *domain_device;
+
+	domain_device = list_first_entry(&s390_domain->devices,
+					 struct s390_domain_device, list);
+	if (!domain_device)
+		return NULL;
+	return domain_device->zdev;
+}
+
+static int s390_domain_get_fn(struct iommu_domain *domain, void *data)
+{
+	struct zpci_dev *zdev;
+	struct clp_req_rsp_query_pci *rrb;
+	int rc;
+
+	zdev = get_zpci(to_s390_domain(domain));
+	if (!zdev)
+		return -ENODEV;
+	rrb = (struct clp_req_rsp_query_pci *)
+	      __get_free_pages(GFP_KERNEL, get_order(CLP_BLK_SIZE));
+	if (!rrb)
+		return -ENOMEM;
+	rc = zdev_query_pci_fn(zdev, rrb);
+
+	if (!rc && rrb->response.hdr.rsp == CLP_RC_OK)
+		memcpy(data, &rrb->response, sizeof(struct clp_rsp_query_pci));
+	else
+		rc = -EIO;
+	free_pages((unsigned long) rrb, get_order(CLP_BLK_SIZE));
+	return rc;
+}
+
+static int s390_domain_get_grp(struct iommu_domain *domain, void *data)
+{
+	struct zpci_dev *zdev;
+	struct clp_req_rsp_query_pci_grp *rrb;
+	int rc;
+
+	zdev = get_zpci(to_s390_domain(domain));
+	if (!zdev)
+		return -ENODEV;
+	rrb = (struct clp_req_rsp_query_pci_grp *)
+	      __get_free_pages(GFP_KERNEL, get_order(CLP_BLK_SIZE));
+	if (!rrb)
+		return -ENOMEM;
+
+	rc = zdev_query_pci_fngrp(zdev, rrb);
+	if (!rc && rrb->response.hdr.rsp == CLP_RC_OK)
+		memcpy(data, &rrb->response,
+		       sizeof(struct clp_rsp_query_pci_grp));
+	else
+		rc = -EIO;
+
+	free_pages((unsigned long) rrb, get_order(CLP_BLK_SIZE));
+	return rc;
+}
+
+static int s390_domain_get_attr(struct iommu_domain *domain,
+				enum iommu_attr attr, void *data)
+{
+	switch (attr) {
+	case DOMAIN_ATTR_ZPCI_FN_SIZE:
+		return sizeof(struct clp_rsp_query_pci);
+	case DOMAIN_ATTR_ZPCI_GRP_SIZE:
+		return sizeof(struct clp_rsp_query_pci_grp);
+	case DOMAIN_ATTR_ZPCI_FN:
+		return s390_domain_get_fn(domain, data);
+	case DOMAIN_ATTR_ZPCI_GRP:
+		return s390_domain_get_grp(domain, data);
+	default:
+		return -ENODEV;
+	}
+}
+
 static const struct iommu_ops s390_iommu_ops = {
 	.capable = s390_iommu_capable,
 	.domain_alloc = s390_domain_alloc,
@@ -376,6 +452,7 @@ static const struct iommu_ops s390_iommu_ops = {
 	.remove_device = s390_iommu_remove_device,
 	.device_group = generic_device_group,
 	.pgsize_bitmap = S390_IOMMU_PGSIZES,
+	.domain_get_attr = s390_domain_get_attr,
 };
 
 static int __init s390_iommu_init(void)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index ffbbc7e..ebdcac4 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -125,6 +125,10 @@ enum iommu_attr {
 	DOMAIN_ATTR_FSL_PAMUV1,
 	DOMAIN_ATTR_NESTING,	/* two stages of translation */
 	DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE,
+	DOMAIN_ATTR_ZPCI_FN_SIZE,
+	DOMAIN_ATTR_ZPCI_GRP_SIZE,
+	DOMAIN_ATTR_ZPCI_FN,
+	DOMAIN_ATTR_ZPCI_GRP,
 	DOMAIN_ATTR_MAX,
 };
 
-- 
2.7.4

[PATCH 4/4] vfio: vfio_iommu_type1: implement VFIO_IOMMU_INFO_CAPABILITIES

[Pierre Morel]

We implement a capability intercafe for VFIO_IOMMU_GET_INFO and add the
first capability: VFIO_IOMMU_INFO_CAPABILITIES.

When calling the ioctl, the user must specify
VFIO_IOMMU_INFO_CAPABILITIES to retrieve the capabilities and must check
in the answer if capabilities are supported.
Older kernel will not check nor set the VFIO_IOMMU_INFO_CAPABILITIES in
the flags of vfio_iommu_type1_info.

The iommu get_attr callback will be called to retrieve the specific
attributes and fill the capabilities, VFIO_IOMMU_INFO_CAP_QFN for the
PCI query function attributes and VFIO_IOMMU_INFO_CAP_QGRP for the
PCI query function group.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 drivers/vfio/vfio_iommu_type1.c | 95 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 94 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index d0f731c..f7f8120 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -1658,6 +1658,70 @@ static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
 	return ret;
 }
 
+int vfio_iommu_type1_caps(struct vfio_iommu *iommu, struct vfio_info_cap *caps,
+			  size_t size)
+{
+	struct vfio_domain *d;
+	struct vfio_iommu_type1_info_block *info_fn;
+	struct vfio_iommu_type1_info_block *info_grp;
+	unsigned long total_size, fn_size, grp_size;
+	int ret;
+
+	d = list_first_entry(&iommu->domain_list, struct vfio_domain, next);
+	if (!d)
+		return -ENODEV;
+	/* The size of these capabilities are device dependent */
+	fn_size = iommu_domain_get_attr(d->domain,
+					DOMAIN_ATTR_ZPCI_FN_SIZE, NULL);
+	if (fn_size < 0)
+		return fn_size;
+	fn_size +=  sizeof(struct vfio_info_cap_header);
+	total_size = fn_size;
+
+	grp_size = iommu_domain_get_attr(d->domain,
+					 DOMAIN_ATTR_ZPCI_GRP_SIZE, NULL);
+	if (grp_size < 0)
+		return grp_size;
+	grp_size +=  sizeof(struct vfio_info_cap_header);
+	total_size += grp_size;
+
+	/* Tell caller to call us with a greater buffer */
+	if (total_size > size) {
+		caps->size = total_size;
+		return 0;
+	}
+
+	info_fn = kzalloc(fn_size, GFP_KERNEL);
+	if (!info_fn)
+		return -ENOMEM;
+	ret = iommu_domain_get_attr(d->domain,
+				    DOMAIN_ATTR_ZPCI_FN, &info_fn->data);
+	if (ret < 0)
+		return ret;
+
+	info_fn->header.id = VFIO_IOMMU_INFO_CAP_QFN;
+
+	ret = vfio_info_add_capability(caps, &info_fn->header, fn_size);
+	if (ret)
+		goto err_fn;
+
+	info_grp = kzalloc(grp_size, GFP_KERNEL);
+	if (!info_grp)
+		goto err_fn;
+	ret = iommu_domain_get_attr(d->domain,
+				    DOMAIN_ATTR_ZPCI_GRP, &info_grp->data);
+	if (ret < 0)
+		goto err_grp;
+	info_grp->header.id = VFIO_IOMMU_INFO_CAP_QGRP;
+	ret = vfio_info_add_capability(caps, &info_grp->header, grp_size);
+
+err_grp:
+	kfree(info_grp);
+err_fn:
+	kfree(info_fn);
+	return ret;
+}
+
 static long vfio_iommu_type1_ioctl(void *iommu_data,
 				   unsigned int cmd, unsigned long arg)
 {
@@ -1679,6 +1743,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
 		}
 	} else if (cmd == VFIO_IOMMU_GET_INFO) {
 		struct vfio_iommu_type1_info info;
+		struct vfio_info_cap caps = { .buf = NULL, .size = 0 };
+		int ret;
 
 		minsz = offsetofend(struct vfio_iommu_type1_info, iova_pgsizes);
 
@@ -1688,7 +1754,34 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
 		if (info.argsz < minsz)
 			return -EINVAL;
 
-		info.flags = VFIO_IOMMU_INFO_PGSIZES;
+		if (info.flags & VFIO_IOMMU_INFO_CAPABILITIES) {
+			minsz = offsetofend(struct vfio_iommu_type1_info,
+					    cap_offset);
+			if (info.argsz < minsz)
+				return -EINVAL;
+			ret = vfio_iommu_type1_caps(iommu, &caps,
+						    info.argsz - sizeof(info));
+			if (ret)
+				return ret;
+		}
+		if (caps.size) {
+			if (info.argsz < sizeof(info) + caps.size) {
+				info.argsz = sizeof(info) + caps.size;
+				info.cap_offset = 0;
+			} else {
+				if (copy_to_user((void __user *)arg +
+						 sizeof(info), caps.buf,
+						 caps.size)) {
+					kfree(caps.buf);
+					return -EFAULT;
+				}
+
+				info.cap_offset = sizeof(info);
+			}
+			kfree(caps.buf);
+		}
+
+		info.flags |= VFIO_IOMMU_INFO_PGSIZES;
 
 		info.iova_pgsizes = vfio_pgsize_bitmap(iommu);
 
-- 
2.7.4

Re: [PATCH 1/4] s390: pci: Exporting access to CLP PCI function and PCI group

[Robin Murphy]

On 10/05/2019 09:22, Pierre Morel wrote:
> 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     | 72 ++++++++++++++++++++++++---------------------
>   2 files changed, 41 insertions(+), 34 deletions(-)
> 
> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
> index 305befd..e66b246 100644
> --- a/arch/s390/include/asm/pci.h
> +++ b/arch/s390/include/asm/pci.h
> @@ -261,4 +261,7 @@ cpumask_of_pcibus(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 3a36b07..4ae5d77 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);

AFAICS it's only the IOMMU driver itself which needs to call these. That 
can't be built as a module, so you shouldn't need explicit exports - the 
header declaration is enough.

Robin.

>   static int clp_store_query_pci_fn(struct zpci_dev *zdev,
>   				  struct clp_rsp_query_pci *response)
> @@ -174,32 +160,50 @@ 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);
> +}
> +EXPORT_SYMBOL(zdev_query_pci_fn);
> +
> +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 +223,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;
>   
> 

Re: [PATCH 1/4] s390: pci: Exporting access to CLP PCI function and PCI group

[Pierre Morel]

On 10/05/2019 12:21, Robin Murphy wrote:
> On 10/05/2019 09:22, Pierre Morel wrote:
>> 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     | 72 
>> ++++++++++++++++++++++++---------------------
>>   2 files changed, 41 insertions(+), 34 deletions(-)
>>
>> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
>> index 305befd..e66b246 100644
>> --- a/arch/s390/include/asm/pci.h
>> +++ b/arch/s390/include/asm/pci.h
>> @@ -261,4 +261,7 @@ cpumask_of_pcibus(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 3a36b07..4ae5d77 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);
> 
> AFAICS it's only the IOMMU driver itself which needs to call these. That 
> can't be built as a module, so you shouldn't need explicit exports - the 
> header declaration is enough.
> 
> Robin.

This is right and seeing the pointer type only zPCI and s390iommu can 
make use of it.
If nobody has another point of view I will remove the export on the
next iteration.

Thanks,
Pierre

> 
>>   static int clp_store_query_pci_fn(struct zpci_dev *zdev,
>>                     struct clp_rsp_query_pci *response)
>> @@ -174,32 +160,50 @@ 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);
>> +}
>> +EXPORT_SYMBOL(zdev_query_pci_fn);
>> +
>> +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 +223,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;
>>
> 


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH 2/4] vfio: vfio_iommu_type1: Define VFIO_IOMMU_INFO_CAPABILITIES

[Christian Borntraeger]

Alex, 

patch 1 and 3 are s390 specific, 2 and 4 are vfio common code.
Are you ok with the common code changes? If yes, would you prefer to have this
via the s390 tree (Martin) or your tree?

On 10.05.19 10:22, Pierre Morel wrote:
> To use the VFIO_IOMMU_GET_INFO to retrieve IOMMU specific information,
> we define a new flag VFIO_IOMMU_INFO_CAPABILITIES in the
> vfio_iommu_type1_info structure and the associated capability
> information block.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  include/uapi/linux/vfio.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 8f10748..8f68e0f 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -715,6 +715,16 @@ struct vfio_iommu_type1_info {
>  	__u32	flags;
>  #define VFIO_IOMMU_INFO_PGSIZES (1 << 0)	/* supported page sizes info */
>  	__u64	iova_pgsizes;		/* Bitmap of supported page sizes */
> +#define VFIO_IOMMU_INFO_CAPABILITIES (1 << 1)  /* support capabilities info */
> +	__u64   cap_offset;     /* Offset within info struct of first cap */
> +};
> +
> +#define VFIO_IOMMU_INFO_CAP_QFN		1
> +#define VFIO_IOMMU_INFO_CAP_QGRP	2
> +
> +struct vfio_iommu_type1_info_block {
> +	struct vfio_info_cap_header header;
> +	__u32 data[];
>  };
>  
>  #define VFIO_IOMMU_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
> 

Re: [PATCH 2/4] vfio: vfio_iommu_type1: Define VFIO_IOMMU_INFO_CAPABILITIES

[Alex Williamson]

On Fri, 10 May 2019 10:22:33 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> To use the VFIO_IOMMU_GET_INFO to retrieve IOMMU specific information,
> we define a new flag VFIO_IOMMU_INFO_CAPABILITIES in the
> vfio_iommu_type1_info structure and the associated capability
> information block.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  include/uapi/linux/vfio.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 8f10748..8f68e0f 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -715,6 +715,16 @@ struct vfio_iommu_type1_info {
>  	__u32	flags;
>  #define VFIO_IOMMU_INFO_PGSIZES (1 << 0)	/* supported page sizes info */
>  	__u64	iova_pgsizes;		/* Bitmap of supported page sizes */
> +#define VFIO_IOMMU_INFO_CAPABILITIES (1 << 1)  /* support capabilities info */
> +	__u64   cap_offset;     /* Offset within info struct of first cap */
> +};
> +
> +#define VFIO_IOMMU_INFO_CAP_QFN		1
> +#define VFIO_IOMMU_INFO_CAP_QGRP	2

Descriptions?

> +
> +struct vfio_iommu_type1_info_block {
> +	struct vfio_info_cap_header header;
> +	__u32 data[];
>  };
>  
>  #define VFIO_IOMMU_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)

This is just a blob of data, what's the API?  How do we revision it?
How does the user know how to interpret it?  Dumping kernel internal
structures out to userspace like this is not acceptable, define a user
API. Thanks,

Alex

Re: [PATCH 4/4] vfio: vfio_iommu_type1: implement VFIO_IOMMU_INFO_CAPABILITIES

[Alex Williamson]

On Fri, 10 May 2019 10:22:35 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> We implement a capability intercafe for VFIO_IOMMU_GET_INFO and add the
> first capability: VFIO_IOMMU_INFO_CAPABILITIES.
> 
> When calling the ioctl, the user must specify
> VFIO_IOMMU_INFO_CAPABILITIES to retrieve the capabilities and must check
> in the answer if capabilities are supported.
> Older kernel will not check nor set the VFIO_IOMMU_INFO_CAPABILITIES in
> the flags of vfio_iommu_type1_info.
> 
> The iommu get_attr callback will be called to retrieve the specific
> attributes and fill the capabilities, VFIO_IOMMU_INFO_CAP_QFN for the
> PCI query function attributes and VFIO_IOMMU_INFO_CAP_QGRP for the
> PCI query function group.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 95 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 94 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index d0f731c..f7f8120 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -1658,6 +1658,70 @@ static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
>  	return ret;
>  }
>  
> +int vfio_iommu_type1_caps(struct vfio_iommu *iommu, struct vfio_info_cap *caps,
> +			  size_t size)
> +{
> +	struct vfio_domain *d;
> +	struct vfio_iommu_type1_info_block *info_fn;
> +	struct vfio_iommu_type1_info_block *info_grp;
> +	unsigned long total_size, fn_size, grp_size;
> +	int ret;
> +
> +	d = list_first_entry(&iommu->domain_list, struct vfio_domain, next);
> +	if (!d)
> +		return -ENODEV;
> +	/* The size of these capabilities are device dependent */
> +	fn_size = iommu_domain_get_attr(d->domain,
> +					DOMAIN_ATTR_ZPCI_FN_SIZE, NULL);
> +	if (fn_size < 0)
> +		return fn_size;

What if non-Z archs want to use this?  The function is architected
specifically for this one use case, fail if any component is not there
which means it requires a re-write to add further support.  If
ZPCI_FN_SIZE isn't support, move on to the next thing.

> +	fn_size +=  sizeof(struct vfio_info_cap_header);
> +	total_size = fn_size;

Here too, total_size should be initialized to zero and each section +=
the size they'd like to add.

> +
> +	grp_size = iommu_domain_get_attr(d->domain,
> +					 DOMAIN_ATTR_ZPCI_GRP_SIZE, NULL);
> +	if (grp_size < 0)
> +		return grp_size;
> +	grp_size +=  sizeof(struct vfio_info_cap_header);
> +	total_size += grp_size;
> +
> +	/* Tell caller to call us with a greater buffer */
> +	if (total_size > size) {
> +		caps->size = total_size;
> +		return 0;
> +	}
> +
> +	info_fn = kzalloc(fn_size, GFP_KERNEL);
> +	if (!info_fn)
> +		return -ENOMEM;

Maybe fn_size was zero because we're not on Z.

> +	ret = iommu_domain_get_attr(d->domain,
> +				    DOMAIN_ATTR_ZPCI_FN, &info_fn->data);

Kernel internal structures != user api.  Thanks,

Alex

> +	if (ret < 0)
> +		return ret;
> +
> +	info_fn->header.id = VFIO_IOMMU_INFO_CAP_QFN;
> +
> +	ret = vfio_info_add_capability(caps, &info_fn->header, fn_size);
> +	if (ret)
> +		goto err_fn;
> +
> +	info_grp = kzalloc(grp_size, GFP_KERNEL);
> +	if (!info_grp)
> +		goto err_fn;
> +	ret = iommu_domain_get_attr(d->domain,
> +				    DOMAIN_ATTR_ZPCI_GRP, &info_grp->data);
> +	if (ret < 0)
> +		goto err_grp;
> +	info_grp->header.id = VFIO_IOMMU_INFO_CAP_QGRP;
> +	ret = vfio_info_add_capability(caps, &info_grp->header, grp_size);
> +
> +err_grp:
> +	kfree(info_grp);
> +err_fn:
> +	kfree(info_fn);
> +	return ret;
> +}
> +
>  static long vfio_iommu_type1_ioctl(void *iommu_data,
>  				   unsigned int cmd, unsigned long arg)
>  {
> @@ -1679,6 +1743,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>  		}
>  	} else if (cmd == VFIO_IOMMU_GET_INFO) {
>  		struct vfio_iommu_type1_info info;
> +		struct vfio_info_cap caps = { .buf = NULL, .size = 0 };
> +		int ret;
>  
>  		minsz = offsetofend(struct vfio_iommu_type1_info, iova_pgsizes);
>  
> @@ -1688,7 +1754,34 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>  		if (info.argsz < minsz)
>  			return -EINVAL;
>  
> -		info.flags = VFIO_IOMMU_INFO_PGSIZES;
> +		if (info.flags & VFIO_IOMMU_INFO_CAPABILITIES) {
> +			minsz = offsetofend(struct vfio_iommu_type1_info,
> +					    cap_offset);
> +			if (info.argsz < minsz)
> +				return -EINVAL;
> +			ret = vfio_iommu_type1_caps(iommu, &caps,
> +						    info.argsz - sizeof(info));
> +			if (ret)
> +				return ret;
> +		}
> +		if (caps.size) {
> +			if (info.argsz < sizeof(info) + caps.size) {
> +				info.argsz = sizeof(info) + caps.size;
> +				info.cap_offset = 0;
> +			} else {
> +				if (copy_to_user((void __user *)arg +
> +						 sizeof(info), caps.buf,
> +						 caps.size)) {
> +					kfree(caps.buf);
> +					return -EFAULT;
> +				}
> +
> +				info.cap_offset = sizeof(info);
> +			}
> +			kfree(caps.buf);
> +		}
> +
> +		info.flags |= VFIO_IOMMU_INFO_PGSIZES;
>  
>  		info.iova_pgsizes = vfio_pgsize_bitmap(iommu);
>  

Re: [PATCH 2/4] vfio: vfio_iommu_type1: Define VFIO_IOMMU_INFO_CAPABILITIES

[Alex Williamson]

On Thu, 16 May 2019 16:57:42 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> Alex, 
> 
> patch 1 and 3 are s390 specific, 2 and 4 are vfio common code.
> Are you ok with the common code changes? If yes, would you prefer to have this
> via the s390 tree (Martin) or your tree?

Hi Christian,

The vfio code still needs work imo, and I'm not sure it isn't somewhat
abusive of the iommu attribute interface as well.  I don't necessarily
have a problem with it ultimately going through the s390 tree, but
let's see what comes in the next revision.  Thanks,

Alex

> On 10.05.19 10:22, Pierre Morel wrote:
> > To use the VFIO_IOMMU_GET_INFO to retrieve IOMMU specific information,
> > we define a new flag VFIO_IOMMU_INFO_CAPABILITIES in the
> > vfio_iommu_type1_info structure and the associated capability
> > information block.
> > 
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > ---
> >  include/uapi/linux/vfio.h | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index 8f10748..8f68e0f 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -715,6 +715,16 @@ struct vfio_iommu_type1_info {
> >  	__u32	flags;
> >  #define VFIO_IOMMU_INFO_PGSIZES (1 << 0)	/* supported page sizes info */
> >  	__u64	iova_pgsizes;		/* Bitmap of supported page sizes */
> > +#define VFIO_IOMMU_INFO_CAPABILITIES (1 << 1)  /* support capabilities info */
> > +	__u64   cap_offset;     /* Offset within info struct of first cap */
> > +};
> > +
> > +#define VFIO_IOMMU_INFO_CAP_QFN		1
> > +#define VFIO_IOMMU_INFO_CAP_QGRP	2
> > +
> > +struct vfio_iommu_type1_info_block {
> > +	struct vfio_info_cap_header header;
> > +	__u32 data[];
> >  };
> >  
> >  #define VFIO_IOMMU_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
> >   
> 

Re: [PATCH 4/4] vfio: vfio_iommu_type1: implement VFIO_IOMMU_INFO_CAPABILITIES

[Pierre Morel]

On 16/05/2019 20:40, Alex Williamson wrote:
> On Fri, 10 May 2019 10:22:35 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> We implement a capability intercafe for VFIO_IOMMU_GET_INFO and add the
>> first capability: VFIO_IOMMU_INFO_CAPABILITIES.
>>
>> When calling the ioctl, the user must specify
>> VFIO_IOMMU_INFO_CAPABILITIES to retrieve the capabilities and must check
>> in the answer if capabilities are supported.
>> Older kernel will not check nor set the VFIO_IOMMU_INFO_CAPABILITIES in
>> the flags of vfio_iommu_type1_info.
>>
>> The iommu get_attr callback will be called to retrieve the specific
>> attributes and fill the capabilities, VFIO_IOMMU_INFO_CAP_QFN for the
>> PCI query function attributes and VFIO_IOMMU_INFO_CAP_QGRP for the
>> PCI query function group.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   drivers/vfio/vfio_iommu_type1.c | 95 ++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 94 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index d0f731c..f7f8120 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -1658,6 +1658,70 @@ static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
>>   	return ret;
>>   }
>>   
>> +int vfio_iommu_type1_caps(struct vfio_iommu *iommu, struct vfio_info_cap *caps,
>> +			  size_t size)
>> +{
>> +	struct vfio_domain *d;
>> +	struct vfio_iommu_type1_info_block *info_fn;
>> +	struct vfio_iommu_type1_info_block *info_grp;
>> +	unsigned long total_size, fn_size, grp_size;
>> +	int ret;
>> +
>> +	d = list_first_entry(&iommu->domain_list, struct vfio_domain, next);
>> +	if (!d)
>> +		return -ENODEV;
>> +	/* The size of these capabilities are device dependent */
>> +	fn_size = iommu_domain_get_attr(d->domain,
>> +					DOMAIN_ATTR_ZPCI_FN_SIZE, NULL);
>> +	if (fn_size < 0)
>> +		return fn_size;
> 
> What if non-Z archs want to use this?  The function is architected
> specifically for this one use case, fail if any component is not there
> which means it requires a re-write to add further support.  If
> ZPCI_FN_SIZE isn't support, move on to the next thing.

yes, clear.

> 
>> +	fn_size +=  sizeof(struct vfio_info_cap_header);
>> +	total_size = fn_size;
> 
> Here too, total_size should be initialized to zero and each section +=
> the size they'd like to add.

thanks, clear too.

> 
>> +
>> +	grp_size = iommu_domain_get_attr(d->domain,
>> +					 DOMAIN_ATTR_ZPCI_GRP_SIZE, NULL);
>> +	if (grp_size < 0)
>> +		return grp_size;
>> +	grp_size +=  sizeof(struct vfio_info_cap_header);
>> +	total_size += grp_size;
>> +
>> +	/* Tell caller to call us with a greater buffer */
>> +	if (total_size > size) {
>> +		caps->size = total_size;
>> +		return 0;
>> +	}
>> +
>> +	info_fn = kzalloc(fn_size, GFP_KERNEL);
>> +	if (!info_fn)
>> +		return -ENOMEM;
> 
> Maybe fn_size was zero because we're not on Z.
> 
>> +	ret = iommu_domain_get_attr(d->domain,
>> +				    DOMAIN_ATTR_ZPCI_FN, &info_fn->data);
> 
> Kernel internal structures != user api.  Thanks,
> 
> Alex

Thanks a lot Alex,
I understand the concerns, I was too focussed on Z, I will rework this 
as you said:
- definition of the user API and
- take care that another architecture may want to use the interface.

Regards,
Pierre



-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH 2/4] vfio: vfio_iommu_type1: Define VFIO_IOMMU_INFO_CAPABILITIES

[Pierre Morel]

On 16/05/2019 20:31, Alex Williamson wrote:
> On Fri, 10 May 2019 10:22:33 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> To use the VFIO_IOMMU_GET_INFO to retrieve IOMMU specific information,
>> we define a new flag VFIO_IOMMU_INFO_CAPABILITIES in the
>> vfio_iommu_type1_info structure and the associated capability
>> information block.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   include/uapi/linux/vfio.h | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index 8f10748..8f68e0f 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h
>> @@ -715,6 +715,16 @@ struct vfio_iommu_type1_info {
>>   	__u32	flags;
>>   #define VFIO_IOMMU_INFO_PGSIZES (1 << 0)	/* supported page sizes info */
>>   	__u64	iova_pgsizes;		/* Bitmap of supported page sizes */
>> +#define VFIO_IOMMU_INFO_CAPABILITIES (1 << 1)  /* support capabilities info */
>> +	__u64   cap_offset;     /* Offset within info struct of first cap */
>> +};
>> +
>> +#define VFIO_IOMMU_INFO_CAP_QFN		1
>> +#define VFIO_IOMMU_INFO_CAP_QGRP	2
> 
> Descriptions?
> 
>> +
>> +struct vfio_iommu_type1_info_block {
>> +	struct vfio_info_cap_header header;
>> +	__u32 data[];
>>   };
>>   
>>   #define VFIO_IOMMU_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
> 
> This is just a blob of data, what's the API?  How do we revision it?
> How does the user know how to interpret it?  Dumping kernel internal
> structures out to userspace like this is not acceptable, define a user
> API. Thanks,
> 
> Alex
> 

Thanks Alex for the comments.
I will add the decription and the user API for the next iteration.

Regards,
Pierre




-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany