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

Matthew Rosato
2019-09-20T15:14:49Z

On 9/20/19 10:02 AM, Cornelia Huck wrote:
> On Thu, 19 Sep 2019 16:55:57 -0400
> Matthew Rosato <mjrosato@linux.ibm.com> wrote:
>
>> On 9/19/19 11:20 AM, Cornelia Huck wrote:
>>> On Fri, 6 Sep 2019 20:13:50 -0400
>>> Matthew Rosato <mjrosato@linux.ibm.com> wrote:
>>>
>>>> From: Pierre Morel <pmorel@linux.ibm.com>
>>>>
>>>> We define a new device region in vfio.h to be able to
>>>> get the ZPCI CLP information by reading this region from
>>>> userland.
>>>>
>>>> We create a new file, vfio_zdev.h to define the structure
>>>> of the new region we defined in vfio.h
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
>>>> ---
>>>> include/uapi/linux/vfio.h | 1 +
>>>> include/uapi/linux/vfio_zdev.h | 35 +++++++++++++++++++++++++++++++++++
>>>> 2 files changed, 36 insertions(+)
>>>> create mode 100644 include/uapi/linux/vfio_zdev.h
>
>>>> diff --git a/include/uapi/linux/vfio_zdev.h b/include/uapi/linux/vfio_zdev.h
>>>> new file mode 100644
>>>> index 0000000..55e0d6d
>>>> --- /dev/null
>>>> +++ b/include/uapi/linux/vfio_zdev.h
>>>> @@ -0,0 +1,35 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>>>> +/*
>>>> + * Region definition for ZPCI devices
>>>> + *
>>>> + * Copyright IBM Corp. 2019
>>>> + *
>>>> + * Author(s): Pierre Morel <pmorel@linux.ibm.com>
>>>> + */
>>>> +
>>>> +#ifndef _VFIO_ZDEV_H_
>>>> +#define _VFIO_ZDEV_H_
>>>> +
>>>> +#include <linux/types.h>
>>>> +
>>>> +/**
>>>> + * struct vfio_region_zpci_info - ZPCI information.
>>>
>>> Hm... probably should also get some more explanation. E.g. is that
>>> derived from a hardware structure?
>>>
>>
>> The structure itself is not mapped 1:1 to a hardware structure, but it
>> does serve as a collection of information that was derived from other
>> hardware structures.
>>
>> "Used for passing hardware feature information about a zpci device
>> between the host and guest" ?
>
> "zPCI specific hardware feature information for a device"?
>
> Are we reasonably sure that this is complete for now? I'm not sure if
> expanding this structure would work; adding another should always be
> possible, though (if a bit annoying).
>

I think trying to make the structure expandable would be best... If we
allow arbitrary-sized reads of the info, and only add new fields onto
the end it should be OK, no? (older qemu doesn't get the info it doesn't
ask for / understand).... But I guess that's not compatible with having
util_str[] size being defined dynamically. Another caveat would be if
CLP_UTIL_STR_LEN were to grow in size in the future, and assuming
util_str[] was no longer at the end of the structure, I guess the
additional data would have to end up in a
util_str2[CLP_UTIL_STR_LEN_NEW-CLP_UTIL_STR_LEN_OLD]... To explain what
I mean, something like:

struct vfio_region_zpci_info {
<..>
__u8 util_str[CLP_UTIL_STR_LEN_OLD];
/* END OF V1 */
__u8 foo;
/* END OF V2 */
__u8 util_str2[CLP_UTIL_STR_LEN_NEW-CLP_UTIL_STR_LEN_OLD];
/* END OF V3 */
} __packed;

>>
>>>> + *
>>>> + */
>>>> +struct vfio_region_zpci_info {
>>>> + __u64 dasm;
>>>> + __u64 start_dma;
>>>> + __u64 end_dma;
>>>> + __u64 msi_addr;
>>>> + __u64 flags;
>>>> + __u16 pchid;
>>>> + __u16 mui;
>>>> + __u16 noi;
>>>> + __u16 maxstbl;
>>>> + __u8 version;
>>>> + __u8 gid;
>>>> +#define VFIO_PCI_ZDEV_FLAGS_REFRESH 1
>>>> + __u8 util_str[];
>>>> +} __packed;
>>>> +
>>>> +#endif
>>>
>>>
>>
>