| Message ID | 1478293856-8191-16-git-send-email-kwankhede@nvidia.com |
|---|---|
| State | New |
| Headers | show |
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. > + *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. > + if (hdr->argsz - minsz < hdr->count * size) > + return -EINVAL; > + > + if (!data_size) > + return -EINVAL; Redundant check as well. > + > + *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() 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. > +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? > + > /* > * 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); >
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); >> > >
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); >>> >> >>
On Wed, 9 Nov 2016 14:07:58 +1100 Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > 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 :) Maybe you haven't noticed, but your patch series are often difficult to deal with, they almost always split across functional areas and maintainers. Splitting out code to common functions and _then_ updating the callers to make use of it is a common way to deal with that. We're in the same functional area here, but it's still good practice. Thanks, Alex
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) + *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) { + if (hdr->argsz - minsz < hdr->count * size) + return -EINVAL; + + if (!data_size) + return -EINVAL; + + *data_size = hdr->count * size; + } + + return 0; +} +EXPORT_SYMBOL(vfio_set_irqs_validate_and_prepare); + /* * 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);