From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753287AbcKIDIG (ORCPT ); Tue, 8 Nov 2016 22:08:06 -0500 Received: from mail-pf0-f196.google.com ([209.85.192.196]:36834 "EHLO mail-pf0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753035AbcKIDIF (ORCPT ); Tue, 8 Nov 2016 22:08:05 -0500 Subject: Re: [PATCH v11 15/22] vfio: Introduce vfio_set_irqs_validate_and_prepare() To: Kirti Wankhede , alex.williamson@redhat.com, pbonzini@redhat.com, kraxel@redhat.com, cjia@nvidia.com References: <1478293856-8191-1-git-send-email-kwankhede@nvidia.com> <1478293856-8191-16-git-send-email-kwankhede@nvidia.com> Cc: qemu-devel@nongnu.org, kvm@vger.kernel.org, kevin.tian@intel.com, jike.song@intel.com, bjsdjshi@linux.vnet.ibm.com, linux-kernel@vger.kernel.org From: Alexey Kardashevskiy Message-ID: <26a4171a-e68f-c8e5-af39-b5af2c1645fe@ozlabs.ru> Date: Wed, 9 Nov 2016 14:07:58 +1100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: 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 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 >>> 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. 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