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