Re: [PATCH v11 15/22] vfio: Introduce vfio_set_irqs_validate_and_prepare()

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 09/11/16 07:22, Kirti Wankhede wrote:
> 
> 
> On 11/8/2016 2:16 PM, Alexey Kardashevskiy wrote:
>> On 05/11/16 08:10, Kirti Wankhede wrote:
>>> Vendor driver using mediated device framework would use same mechnism to
>>> validate and prepare IRQs. Introducing this function to reduce code
>>> replication in multiple drivers.
>>>
>>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
>>> Signed-off-by: Neo Jia <cjia@nvidia.com>
>>> Change-Id: Ie201f269dda0713ca18a07dc4852500bd8b48309
>>> ---
>>>  drivers/vfio/vfio.c  | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>  include/linux/vfio.h |  4 ++++
>>>  2 files changed, 52 insertions(+)
>>>
>>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>>> index 9a03be0942a1..ed2361e4b904 100644
>>> --- a/drivers/vfio/vfio.c
>>> +++ b/drivers/vfio/vfio.c
>>> @@ -1858,6 +1858,54 @@ int vfio_info_add_capability(struct vfio_info_cap *caps, int cap_type_id,
>>>  }
>>>  EXPORT_SYMBOL(vfio_info_add_capability);
>>>  
>>> +int vfio_set_irqs_validate_and_prepare(struct vfio_irq_set *hdr, int num_irqs,
>>> +				       int max_irq_type, size_t *data_size)
>>> +{
>>> +	unsigned long minsz;
>>> +	size_t size;
>>> +
>>> +	minsz = offsetofend(struct vfio_irq_set, count);
>>> +
>>> +	if ((hdr->argsz < minsz) || (hdr->index >= max_irq_type) ||
>>> +	    (hdr->count >= (U32_MAX - hdr->start)) ||
>>> +	    (hdr->flags & ~(VFIO_IRQ_SET_DATA_TYPE_MASK |
>>> +				VFIO_IRQ_SET_ACTION_TYPE_MASK)))
>>> +		return -EINVAL;
>>> +
>>> +	if (data_size)
>>
>> Pointless check, the callers will pass non null pointer with value
>> initialized to 0 anyway.
>>
> 
> Not always, When VFIO_IRQ_SET_DATA_NONE flag is set, caller can pass
> data_size = NULL.


Today data_size is not NULL in all cases and the way it is used now (ioctl
VFIO_DEVICE_SET_IRQS) gives me an idea that this is not going to change.

> 
>>
>>> +		*data_size = 0;
>>> +
>>> +	if (hdr->start >= num_irqs || hdr->start + hdr->count > num_irqs)
>>> +		return -EINVAL;
>>> +
>>> +	switch (hdr->flags & VFIO_IRQ_SET_DATA_TYPE_MASK) {
>>> +	case VFIO_IRQ_SET_DATA_NONE:
>>> +		size = 0;
>>> +		break;
>>> +	case VFIO_IRQ_SET_DATA_BOOL:
>>> +		size = sizeof(uint8_t);
>>> +		break;
>>> +	case VFIO_IRQ_SET_DATA_EVENTFD:
>>> +		size = sizeof(int32_t);
>>> +		break;
>>> +	default:
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	if (size) {
>>
>> The whole branch would even work for size == 0.
>>
> 
> In that case below check (!data_size) might result in error if data_size
> == NULL, whereas its not error case when size == 0, i.e.
> VFIO_IRQ_SET_DATA_NONE flag set.
> 
>>> +		if (hdr->argsz - minsz < hdr->count * size)
>>> +			return -EINVAL;
>>> +
>>> +		if (!data_size)
>>> +			return -EINVAL;
>>
>> Redundant check as well.
>>
> 
> This is not redundant. If you see above check, it sets its init value to
> 0 but doesn't fail.
> 
>>> +
>>> +		*data_size = hdr->count * size;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>
>> It does not really prepare anything as the name suggests. It looks like
>> this is 2 different helpers actually:
>>
>> int vfio_set_irqs_validate()
>> and
>> size_t vfio_set_irqs_hdr_to_data_size()
>>
> 
> Later one is the prepare.


Does not like it prepares anything, just a simple converter.


>> And it would make it easier to review/bisect if 16/22 and 17/22 were merged
>> into this one as this patch alone adds new code which it does not use and
>> all 3 patches are fairly small.
>>
> 
> I do had all 3 patch merged in one in earlier version of patchset. This
> is split as per Alex's suggestion.

I got this from another mail from Alex. Which I find strange but whatever,
this is his realm anyway :)


> 
>>
>>> +EXPORT_SYMBOL(vfio_set_irqs_validate_and_prepare);
>>
>> Everything you export in this patchset is EXPORT_SYMBOL() while the
>> existing code uses EXPORT_SYMBOL_GPL(), is this for a reason?
>>
>>
> 
> We want these symbols to be available to all drivers.


Right, got it from another mail from Alex as well. Ok, seems all right so
far. A note in the commit log would be useful though.



> 
> Thanks,
> Kirti
> 
>>> +
>>>  /*
>>>   * Pin a set of guest PFNs and return their associated host PFNs for local
>>>   * domain only.
>>> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
>>> index cf90393a11e2..87c9afecd822 100644
>>> --- a/include/linux/vfio.h
>>> +++ b/include/linux/vfio.h
>>> @@ -116,6 +116,10 @@ extern void vfio_info_cap_shift(struct vfio_info_cap *caps, size_t offset);
>>>  extern int vfio_info_add_capability(struct vfio_info_cap *caps,
>>>  				    int cap_type_id, void *cap_type);
>>>  
>>> +extern int vfio_set_irqs_validate_and_prepare(struct vfio_irq_set *hdr,
>>> +					      int num_irqs, int max_irq_type,
>>> +					      size_t *data_size);
>>> +
>>>  struct pci_dev;
>>>  #ifdef CONFIG_EEH
>>>  extern void vfio_spapr_pci_eeh_open(struct pci_dev *pdev);
>>>
>>
>>


-- 
Alexey