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@xxxxxxxxxx>
>> Signed-off-by: Neo Jia <cjia@xxxxxxxxxx>
>> 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);
>>
>
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
