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

[Kirti Wankhede <kwankhede@nvidia.com>]

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.

> 
>> +		*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.

> 
> 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.

> 
>> +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.

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);
>>
> 
>