Messages in this thread | | | Subject | Re: [PATCH v11 15/22] vfio: Introduce vfio_set_irqs_validate_and_prepare() | From | Kirti Wankhede <> | Date | Wed, 9 Nov 2016 01:52:49 +0530 |
| |
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); >> > >
| |