Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933518AbcKHU0E (ORCPT ); Tue, 8 Nov 2016 15:26:04 -0500 Received: from hqemgate16.nvidia.com ([216.228.121.65]:18644 "EHLO hqemgate16.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932468AbcKHU0D (ORCPT ); Tue, 8 Nov 2016 15:26:03 -0500 X-PGP-Universal: processed; by hqpgpgate101.nvidia.com on Tue, 08 Nov 2016 12:26:00 -0800 Subject: Re: [PATCH v11 15/22] vfio: Introduce vfio_set_irqs_validate_and_prepare() To: Alexey Kardashevskiy , , , , References: <1478293856-8191-1-git-send-email-kwankhede@nvidia.com> <1478293856-8191-16-git-send-email-kwankhede@nvidia.com> CC: , , , , , X-Nvconfidentiality: public From: Kirti Wankhede Message-ID: Date: Wed, 9 Nov 2016 01:52:49 +0530 MIME-Version: 1.0 In-Reply-To: X-Originating-IP: [10.24.70.42] X-ClientProxiedBy: DRHKMAIL103.nvidia.com (10.25.59.17) To bgmail102.nvidia.com (10.25.59.11) Content-Type: text/plain; charset="koi8-r" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4205 Lines: 149 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 >> Signed-off-by: Neo Jia >> 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); >> > >