diff mbox series

[v3,2/3] vfio: zpci: defining the VFIO headers

Message ID 1558614326-24711-3-git-send-email-pmorel@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series Retrieving zPCI specific info with VFIO | expand

Commit Message

Pierre Morel May 23, 2019, 12:25 p.m. UTC 
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>
---
 include/uapi/linux/vfio.h      |  4 ++++
 include/uapi/linux/vfio_zdev.h | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)
 create mode 100644 include/uapi/linux/vfio_zdev.h

Comments

Cornelia Huck May 23, 2019, 4:24 p.m. UTC | #1 
On Thu, 23 May 2019 14:25:25 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> 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>
> ---
>  include/uapi/linux/vfio.h      |  4 ++++
>  include/uapi/linux/vfio_zdev.h | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 38 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..56595b8 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -306,6 +306,10 @@ struct vfio_region_info_cap_type {
>  #define VFIO_REGION_TYPE_GFX                    (1)
>  #define VFIO_REGION_SUBTYPE_GFX_EDID            (1)
>  
> +/* IBM Subtypes */
> +#define VFIO_REGION_TYPE_IBM_ZDEV		(1)
> +#define VFIO_REGION_SUBTYPE_ZDEV_CLP		(1)

I'm afraid that confuses me a bit. You want to add the region to every
vfio-pci device when we're running under s390, right? So this does not
depend on the device type of the actual device (which may or may not be
from IBM), but only on the architecture?

(Generally speaking, I think using regions for this makes sense,
though.)

> +
>  /**
>   * struct vfio_region_gfx_edid - EDID region layout.
>   *
> diff --git a/include/uapi/linux/vfio_zdev.h b/include/uapi/linux/vfio_zdev.h
> new file mode 100644
> index 0000000..84b1a82
> --- /dev/null
> +++ b/include/uapi/linux/vfio_zdev.h
> @@ -0,0 +1,34 @@
> +/* 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;
> +	__u8 gid;
> +	__u8 version;
> +#define VFIO_PCI_ZDEV_FLAGS_REFRESH 1
> +	__u8 util_str[CLP_UTIL_STR_LEN];
> +} __packed;
> +
> +#endif
Alex Williamson May 28, 2019, 9:43 p.m. UTC | #2 
On Thu, 23 May 2019 14:25:25 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> 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>
> ---
>  include/uapi/linux/vfio.h      |  4 ++++
>  include/uapi/linux/vfio_zdev.h | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 38 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..56595b8 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -306,6 +306,10 @@ struct vfio_region_info_cap_type {
>  #define VFIO_REGION_TYPE_GFX                    (1)
>  #define VFIO_REGION_SUBTYPE_GFX_EDID            (1)
>  
> +/* IBM Subtypes */
> +#define VFIO_REGION_TYPE_IBM_ZDEV		(1)

This one defines (but never uses) a conflicting region type to GFX
above.

> +#define VFIO_REGION_SUBTYPE_ZDEV_CLP		(1)

If we're using a PCI vendor type, which the next patch indicates we
are, this is the one you need.  But please also specify it as a PCI
vendor sub-type with the hex PCI vendor ID, and perhaps group it with
the Intel vendor sub-types.

> +
>  /**
>   * struct vfio_region_gfx_edid - EDID region layout.
>   *
> diff --git a/include/uapi/linux/vfio_zdev.h b/include/uapi/linux/vfio_zdev.h
> new file mode 100644
> index 0000000..84b1a82
> --- /dev/null
> +++ b/include/uapi/linux/vfio_zdev.h
> @@ -0,0 +1,34 @@
> +/* 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;
> +	__u8 gid;
> +	__u8 version;
> +#define VFIO_PCI_ZDEV_FLAGS_REFRESH 1
> +	__u8 util_str[CLP_UTIL_STR_LEN];

I don't see where CLP_UTIL_STR_LEN is defined in a uapi consumable
header.  Should this simply be [] where the string length is implied by
the remaining region size?  QEMU hard codes it, that doesn't validate
the vfio uapi.

> +} __packed;
> +
> +#endif
Alex Williamson May 28, 2019, 9:46 p.m. UTC | #3 
On Tue, 28 May 2019 15:43:42 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Thu, 23 May 2019 14:25:25 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
> > 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>
> > ---
> >  include/uapi/linux/vfio.h      |  4 ++++
> >  include/uapi/linux/vfio_zdev.h | 34 ++++++++++++++++++++++++++++++++++
> >  2 files changed, 38 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..56595b8 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -306,6 +306,10 @@ struct vfio_region_info_cap_type {
> >  #define VFIO_REGION_TYPE_GFX                    (1)
> >  #define VFIO_REGION_SUBTYPE_GFX_EDID            (1)
> >  
> > +/* IBM Subtypes */
> > +#define VFIO_REGION_TYPE_IBM_ZDEV		(1)  
> 
> This one defines (but never uses) a conflicting region type to GFX
> above.
> 
> > +#define VFIO_REGION_SUBTYPE_ZDEV_CLP		(1)  
> 
> If we're using a PCI vendor type, which the next patch indicates we
> are, this is the one you need.  But please also specify it as a PCI
> vendor sub-type with the hex PCI vendor ID, and perhaps group it with
> the Intel vendor sub-types.

BTW, we've already started a set of IBM sub-types:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/uapi/linux/vfio.h?id=7f92891778dff62303c070ac81de7b7d80de331a

> > +
> >  /**
> >   * struct vfio_region_gfx_edid - EDID region layout.
> >   *
> > diff --git a/include/uapi/linux/vfio_zdev.h b/include/uapi/linux/vfio_zdev.h
> > new file mode 100644
> > index 0000000..84b1a82
> > --- /dev/null
> > +++ b/include/uapi/linux/vfio_zdev.h
> > @@ -0,0 +1,34 @@
> > +/* 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;
> > +	__u8 gid;
> > +	__u8 version;
> > +#define VFIO_PCI_ZDEV_FLAGS_REFRESH 1
> > +	__u8 util_str[CLP_UTIL_STR_LEN];  
> 
> I don't see where CLP_UTIL_STR_LEN is defined in a uapi consumable
> header.  Should this simply be [] where the string length is implied by
> the remaining region size?  QEMU hard codes it, that doesn't validate
> the vfio uapi.
> 
> > +} __packed;
> > +
> > +#endif  
>
Alex Williamson May 28, 2019, 9:54 p.m. UTC | #4 
On Thu, 23 May 2019 18:24:33 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Thu, 23 May 2019 14:25:25 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
> > 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>
> > ---
> >  include/uapi/linux/vfio.h      |  4 ++++
> >  include/uapi/linux/vfio_zdev.h | 34 ++++++++++++++++++++++++++++++++++
> >  2 files changed, 38 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..56595b8 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -306,6 +306,10 @@ struct vfio_region_info_cap_type {
> >  #define VFIO_REGION_TYPE_GFX                    (1)
> >  #define VFIO_REGION_SUBTYPE_GFX_EDID            (1)
> >  
> > +/* IBM Subtypes */
> > +#define VFIO_REGION_TYPE_IBM_ZDEV		(1)
> > +#define VFIO_REGION_SUBTYPE_ZDEV_CLP		(1)  
> 
> I'm afraid that confuses me a bit. You want to add the region to every
> vfio-pci device when we're running under s390, right? So this does not
> depend on the device type of the actual device (which may or may not be
> from IBM), but only on the architecture?

FWIW, I don't really have a strong opinion here but I welcome the
discussion.  It seems fair to me that a PCI vendor type could be used
for either a device with that vendor ID or by the vendor of the
platform.  We've got a lot of address space if want to use
VFIO_REGION_TYPE_IBM_ZDEV rather than a PCI vendor type (so long as
it's updated not to conflict with the GFX type).  Thanks,

Alex
 
> (Generally speaking, I think using regions for this makes sense,
> though.)
> 
> > +
> >  /**
> >   * struct vfio_region_gfx_edid - EDID region layout.
> >   *
> > diff --git a/include/uapi/linux/vfio_zdev.h
> > b/include/uapi/linux/vfio_zdev.h new file mode 100644
> > index 0000000..84b1a82
> > --- /dev/null
> > +++ b/include/uapi/linux/vfio_zdev.h
> > @@ -0,0 +1,34 @@
> > +/* 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;
> > +	__u8 gid;
> > +	__u8 version;
> > +#define VFIO_PCI_ZDEV_FLAGS_REFRESH 1
> > +	__u8 util_str[CLP_UTIL_STR_LEN];
> > +} __packed;
> > +
> > +#endif  
>
diff mbox series

Patch

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 8f10748..56595b8 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -306,6 +306,10 @@  struct vfio_region_info_cap_type {
 #define VFIO_REGION_TYPE_GFX                    (1)
 #define VFIO_REGION_SUBTYPE_GFX_EDID            (1)
 
+/* IBM Subtypes */
+#define VFIO_REGION_TYPE_IBM_ZDEV		(1)
+#define VFIO_REGION_SUBTYPE_ZDEV_CLP		(1)
+
 /**
  * struct vfio_region_gfx_edid - EDID region layout.
  *
diff --git a/include/uapi/linux/vfio_zdev.h b/include/uapi/linux/vfio_zdev.h
new file mode 100644
index 0000000..84b1a82
--- /dev/null
+++ b/include/uapi/linux/vfio_zdev.h
@@ -0,0 +1,34 @@ 
+/* 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;
+	__u8 gid;
+	__u8 version;
+#define VFIO_PCI_ZDEV_FLAGS_REFRESH 1
+	__u8 util_str[CLP_UTIL_STR_LEN];
+} __packed;
+
+#endif