Re: [PATCH v3 2/3] vfio: zpci: defining the VFIO headers
On Thu, 23 May 2019 18:24:33 +0200 Cornelia Huck wrote: > On Thu, 23 May 2019 14:25:25 +0200 > Pierre Morel 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 > > --- > > 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 000..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 > > + */ > > + > > +#ifndef _VFIO_ZDEV_H_ > > +#define _VFIO_ZDEV_H_ > > + > > +#include > > + > > +/** > > + * 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 >
Re: [PATCH v3 2/3] vfio: zpci: defining the VFIO headers
On Tue, 28 May 2019 15:43:42 -0600 Alex Williamson wrote: > On Thu, 23 May 2019 14:25:25 +0200 > Pierre Morel 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 > > --- > > 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 000..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 > > + */ > > + > > +#ifndef _VFIO_ZDEV_H_ > > +#define _VFIO_ZDEV_H_ > > + > > +#include > > + > > +/** > > + * 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 >
Re: [PATCH v3 2/3] vfio: zpci: defining the VFIO headers
On Thu, 23 May 2019 14:25:25 +0200 Pierre Morel 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 > --- > 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 000..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 > + */ > + > +#ifndef _VFIO_ZDEV_H_ > +#define _VFIO_ZDEV_H_ > + > +#include > + > +/** > + * 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
Re: [PATCH v3 2/3] vfio: zpci: defining the VFIO headers
On Thu, 23 May 2019 14:25:25 +0200 Pierre Morel 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 > --- > 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 000..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 > + */ > + > +#ifndef _VFIO_ZDEV_H_ > +#define _VFIO_ZDEV_H_ > + > +#include > + > +/** > + * 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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu