[v4,3/4] vfio: zpci: defining the VFIO headers

Commit Message

Matthew Rosato Sept. 7, 2019, 12:13 a.m. UTC

From: Pierre Morel <[e-mail]>

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 <[e-mail]>
Signed-off-by: Matthew Rosato <[e-mail]>
---
 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

Comments

Cornelia Huck Sept. 19, 2019, 3:20 p.m. UTC | #1

On Fri,  6 Sep 2019 20:13:50 -0400
Matthew Rosato <[e-mail]> wrote:

> From: Pierre Morel <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Matthew Rosato <[e-mail]>
> ---
>  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 <[e-mail]>
> + */
> +
> +#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

Matthew Rosato Sept. 19, 2019, 8:55 p.m. UTC | #2

On 9/19/19 11:20 AM, Cornelia Huck wrote:
> On Fri,  6 Sep 2019 20:13:50 -0400
> Matthew Rosato <[e-mail]> wrote:
> 
>> From: Pierre Morel <[e-mail]>
>>
>> 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 <[e-mail]>
>> Signed-off-by: Matthew Rosato <[e-mail]>
>> ---
>>  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 <[e-mail]>
>> + */
>> +
>> +#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
> 
>

Alex Williamson Sept. 19, 2019, 10:27 p.m. UTC | #3

On Thu, 19 Sep 2019 16:55:57 -0400
Matthew Rosato <[e-mail]> wrote:

> On 9/19/19 11:20 AM, Cornelia Huck wrote:
> > On Fri,  6 Sep 2019 20:13:50 -0400
> > Matthew Rosato <[e-mail]> wrote:
> >   
> >> From: Pierre Morel <[e-mail]>
> >>
> >> 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 <[e-mail]>
> >> Signed-off-by: Matthew Rosato <[e-mail]>
> >> ---
> >>  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 <[e-mail]>
> >> + */
> >> +
> >> +#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

Alex Williamson Sept. 19, 2019, 10:49 p.m. UTC | #4

On Thu, 19 Sep 2019 16:27:08 -0600
Alex Williamson <[e-mail]> wrote:

> On Thu, 19 Sep 2019 16:55:57 -0400
> Matthew Rosato <[e-mail]> wrote:
> 
> > On 9/19/19 11:20 AM, Cornelia Huck wrote:  
> > > On Fri,  6 Sep 2019 20:13:50 -0400
> > > Matthew Rosato <[e-mail]> wrote:
> > >     
> > >> From: Pierre Morel <[e-mail]>
> > >>
> > >> 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 <[e-mail]>
> > >> Signed-off-by: Matthew Rosato <[e-mail]>
> > >> ---
> > >>  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 <[e-mail]>
> > >> + */
> > >> +
> > >> +#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

Cornelia Huck Sept. 20, 2019, 2:02 p.m. UTC | #5

On Thu, 19 Sep 2019 16:55:57 -0400
Matthew Rosato <[e-mail]> wrote:

> On 9/19/19 11:20 AM, Cornelia Huck wrote:
> > On Fri,  6 Sep 2019 20:13:50 -0400
> > Matthew Rosato <[e-mail]> wrote:
> >   
> >> From: Pierre Morel <[e-mail]>
> >>
> >> 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 <[e-mail]>
> >> Signed-off-by: Matthew Rosato <[e-mail]>
> >> ---
> >>  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 <[e-mail]>
> >> + */
> >> +
> >> +#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  
> > 
> >   
>

Matthew Rosato Sept. 20, 2019, 2:46 p.m. UTC | #6

On 9/19/19 6:49 PM, Alex Williamson wrote:
> On Thu, 19 Sep 2019 16:27:08 -0600
> Alex Williamson <[e-mail]> wrote:
> 
>> On Thu, 19 Sep 2019 16:55:57 -0400
>> Matthew Rosato <[e-mail]> wrote:
>>
>>> On 9/19/19 11:20 AM, Cornelia Huck wrote:  
>>>> On Fri,  6 Sep 2019 20:13:50 -0400
>>>> Matthew Rosato <[e-mail]> wrote:
>>>>     
>>>>> From: Pierre Morel <[e-mail]>
>>>>>
>>>>> 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 <[e-mail]>
>>>>> Signed-off-by: Matthew Rosato <[e-mail]>
>>>>> ---
>>>>>  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 <[e-mail]>
>>>>> + */
>>>>> +
>>>>> +#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
> 
>

Matthew Rosato Sept. 20, 2019, 3:14 p.m. UTC | #7

On 9/20/19 10:02 AM, Cornelia Huck wrote:
> On Thu, 19 Sep 2019 16:55:57 -0400
> Matthew Rosato <[e-mail]> wrote:
> 
>> On 9/19/19 11:20 AM, Cornelia Huck wrote:
>>> On Fri,  6 Sep 2019 20:13:50 -0400
>>> Matthew Rosato <[e-mail]> wrote:
>>>   
>>>> From: Pierre Morel <[e-mail]>
>>>>
>>>> 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 <[e-mail]>
>>>> Signed-off-by: Matthew Rosato <[e-mail]>
>>>> ---
>>>>  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 <[e-mail]>
>>>> + */
>>>> +
>>>> +#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  
>>>
>>>   
>>
>

Cornelia Huck Oct. 8, 2019, 1:30 p.m. UTC | #8

On Fri, 20 Sep 2019 11:14:28 -0400
Matthew Rosato <[e-mail]> wrote:

> On 9/20/19 10:02 AM, Cornelia Huck wrote:
> > On Thu, 19 Sep 2019 16:55:57 -0400
> > Matthew Rosato <[e-mail]> wrote:
> >   
> >> On 9/19/19 11:20 AM, Cornelia Huck wrote:  
> >>> On Fri,  6 Sep 2019 20:13:50 -0400
> >>> Matthew Rosato <[e-mail]> wrote:
> >>>     
> >>>> From: Pierre Morel <[e-mail]>
> >>>>
> >>>> 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 <[e-mail]>
> >>>> Signed-off-by: Matthew Rosato <[e-mail]>
> >>>> ---
> >>>>  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 <[e-mail]>
> >>>> + */
> >>>> +
> >>>> +#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    
> >>>
> >>>     
> >>  
> >   
>

Patch

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 <[e-mail]>
+ */
+
+#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