Re: [PATCH v6 2/3] drivers/vfio: EEH support for VFIO PCI device

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

On Fri, May 23, 2014 at 08:52:23AM +0200, Alexander Graf wrote:
>
>
>> Am 23.05.2014 um 05:23 schrieb Alex Williamson <alex.williamson@redhat.com>:
>> 
>>> On Fri, 2014-05-23 at 10:37 +1000, Gavin Shan wrote:
>>>> On Fri, May 23, 2014 at 10:17:30AM +1000, Gavin Shan wrote:
>>>>> On Thu, May 22, 2014 at 11:55:29AM +0200, Alexander Graf wrote:
>>>>> On 22.05.14 10:23, Gavin Shan wrote:
>>> 
>>> .../...
>>> 
>>>>>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>>>>>> index cb9023d..ef55682 100644
>>>>>> --- a/include/uapi/linux/vfio.h
>>>>>> +++ b/include/uapi/linux/vfio.h
>>>>>> @@ -455,6 +455,59 @@ struct vfio_iommu_spapr_tce_info {
>>>>>> #define VFIO_IOMMU_SPAPR_TCE_GET_INFO    _IO(VFIO_TYPE, VFIO_BASE + 12)
>>>>>> +/*
>>>>>> + * EEH functionality can be enabled or disabled on one specific device.
>>>>>> + * Also, the DMA or IO frozen state can be removed from the frozen PE
>>>>>> + * if required.
>>>>>> + */
>>>>>> +struct vfio_eeh_pe_set_option {
>>>>>> +    __u32 argsz;
>>>>> 
>>>>> What is this argsz thing? Is this your way of maintaining backwards
>>>>> compatibility when we introduce new fields? A new field will change
>>>>> the ioctl number, so I don't think that makes a lot of sense :).
>>>>> 
>>>>> Just make the ioctl have a u32 as incoming argument. No fancy
>>>>> structs, no complicated code.
>>>>> 
>>>>> The same applies for a number of structs below.
>>>> 
>>>> ok. Will do in next revision.
>>> 
>>> Rechecked include/uapi/linux/vfio.h, the data struct for each ioctl command
>>> always has "argsz". I guess it was used as checker by Alex.W. Do you really
>>> want remove "argsz" ?
>> 
>> 
>> IIRC, this was actually a suggestion incorporated from David Gibson, but
>> using _IO with an argsz and flags field we can maintain compatibility
>> without bumping the ioctl number.  It really only makes sense if we have
>> a flags field so we can identify what additional information is being
>> provided.  Flags can be used as a bitmap of trailing structures or as
>> revision if we want a set of trailing structures that may change over
>> time.  Unless you can come up with a good argument against it that would
>> prevent us inventing a new ioctl as soon as we need a minor tweak, I'd
>> prefer to keep it.  As I noted in a previous comment, the one ioctl we
>> have for reset that doesn't take any options is likely going to be the
>> first ioctl that we need to entirely replace.  If we don't keep argsz,
>> it seems like we probably need a flags field and reserved structures.
>> 
>>>>>> +    __u32 option;
>>>>>> +};
>>>>>> +
>>>>>> +#define VFIO_EEH_PE_SET_OPTION        _IO(VFIO_TYPE, VFIO_BASE + 21)
>>>>>> +
>>>>>> +/*
>>>>>> + * Each EEH PE should have unique address to be identified. The command
>>>>>> + * helps to retrieve the address and the sharing mode of the PE.
>>>>>> + */
>>>>>> +struct vfio_eeh_pe_get_addr {
>>>>>> +    __u32 argsz;
>>>>>> +    __u32 option;
>>>>>> +    __u32 info;
>>>>> 
>>>>> Any particular reason you need the info field? Can't the return value
>>>>> of the ioctl hold this? Then you only have a single u32 argument left
>>>>> to the ioctl again.
>>>> 
>>>> ok. Will do in next revision.
>>> 
>>> If we eventually remove "argsz" and let ioctl() return value to hold
>>> information (or negative number for errors), we don't need any data
>>> struct because the 3rd parameter of ioctl() would be used as input
>>> and I only need one input parameter. Do you want see this ?
>>> 
>>> Hopefully, Alex.W saw this and hasn't objections :)
>> 
>> I'm not sure why we're pushing for the minimal data set to pass to an
>> ioctl.  Seems like a recipe for dead, useless ioctls.  Thanks,
>> 
>
>The ioctl number includes sizeof(payload). So if a new parameter gets added, that would be a different ioctl number.
>
>If you want to maintain backwards compatibility ioctl number wise in the kernel, you'll have to have a "flags" field to indicate whether new data is available and a "pad" field, prefarably in a union, that ensures the size of the struct doesn't change.
>
>I'm not sure it's really necessary here to have identical ioctl numbers if we add parameters, since we can always just define a new ioctl with a bigger payload that can then become the default handler and a shim backwards compatible handler with the old number.
>
>But if you think it is important, let's do it for real, not just halfway.
>

So I need add additional field "flags" ? Also, I need keep the return value from
ioctl() less or equal to 0 ? :-)

Thanks,
Gavin