Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753756AbcKHVmf (ORCPT ); Tue, 8 Nov 2016 16:42:35 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34418 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751211AbcKHVmc (ORCPT ); Tue, 8 Nov 2016 16:42:32 -0500 Date: Tue, 8 Nov 2016 14:42:29 -0700 From: Alex Williamson To: Kirti Wankhede Cc: Alexey Kardashevskiy , , , , , , , , , Subject: Re: [PATCH v11 13/22] vfio: Introduce common function to add capabilities Message-ID: <20161108144229.355533b5@t450s.home> In-Reply-To: <62fbdd2a-6e5b-6a2f-6312-6c2f35bb577c@nvidia.com> References: <1478293856-8191-1-git-send-email-kwankhede@nvidia.com> <1478293856-8191-14-git-send-email-kwankhede@nvidia.com> <10cec4f6-0c0a-dcad-cb2f-4eb6a94cdbde@ozlabs.ru> <62fbdd2a-6e5b-6a2f-6312-6c2f35bb577c@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Tue, 08 Nov 2016 21:42:32 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4908 Lines: 133 On Wed, 9 Nov 2016 02:16:17 +0530 Kirti Wankhede wrote: > On 11/8/2016 12:59 PM, Alexey Kardashevskiy wrote: > > On 05/11/16 08:10, Kirti Wankhede wrote: > >> Vendor driver using mediated device framework should use > >> vfio_info_add_capability() to add capabilities. > >> Introduced this function to reduce code duplication in vendor drivers. > >> > >> Signed-off-by: Kirti Wankhede > >> Signed-off-by: Neo Jia > >> Change-Id: I6fca329fa2291f37a2c859d0bc97574d9e2ce1a6 > >> --- > >> drivers/vfio/vfio.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++- > >> include/linux/vfio.h | 3 +++ > >> 2 files changed, 62 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > >> index 4ed1a6a247c6..9a03be0942a1 100644 > >> --- a/drivers/vfio/vfio.c > >> +++ b/drivers/vfio/vfio.c > >> @@ -1797,8 +1797,66 @@ void vfio_info_cap_shift(struct vfio_info_cap *caps, size_t offset) > >> for (tmp = caps->buf; tmp->next; tmp = (void *)tmp + tmp->next - offset) > >> tmp->next += offset; > >> } > >> -EXPORT_SYMBOL_GPL(vfio_info_cap_shift); > >> +EXPORT_SYMBOL(vfio_info_cap_shift); > > > > > > Why this change? > > > > > > We want this symbol to be available to all drivers. IOW, from proprietary drivers. It makes me uncomfortable how many non-GPL symbols we're adding (or converting) in this effort, but I'm trying to look objectively at every export as to whether a non-GPL caller of the function is legitimately separate from in-kernel code. For instance are they making use of data structures intrinsic to GPL'd code. In this case we're converting a symbol that's just manipulating a data buffer to add an offset to each element in a chain. The entries are documented in a uapi header. Kirti asked me about this one, and I couldn't find any basis to raise an objection. If you spot any reason that any of the export symbols in these series really should be GPL, please raise the issue. > >> > >> +static int sparse_mmap_cap(struct vfio_info_cap *caps, void *cap_type) > >> +{ > >> + struct vfio_info_cap_header *header; > >> + struct vfio_region_info_cap_sparse_mmap *sparse_cap, *sparse = cap_type; > >> + size_t size; > >> + > >> + size = sizeof(*sparse) + sparse->nr_areas * sizeof(*sparse->areas); > >> + header = vfio_info_cap_add(caps, size, > >> + VFIO_REGION_INFO_CAP_SPARSE_MMAP, 1); > >> + if (IS_ERR(header)) > >> + return PTR_ERR(header); > >> + > >> + sparse_cap = container_of(header, > >> + struct vfio_region_info_cap_sparse_mmap, header); > >> + sparse_cap->nr_areas = sparse->nr_areas; > >> + memcpy(sparse_cap->areas, sparse->areas, > >> + sparse->nr_areas * sizeof(*sparse->areas)); > >> + return 0; > >> +} > >> + > >> +static int region_type_cap(struct vfio_info_cap *caps, void *cap_type) > >> +{ > >> + struct vfio_info_cap_header *header; > >> + struct vfio_region_info_cap_type *type_cap, *cap = cap_type; > >> + > >> + header = vfio_info_cap_add(caps, sizeof(*cap), > >> + VFIO_REGION_INFO_CAP_TYPE, 1); > >> + if (IS_ERR(header)) > >> + return PTR_ERR(header); > >> + > >> + type_cap = container_of(header, struct vfio_region_info_cap_type, > >> + header); > >> + type_cap->type = cap->type; > >> + type_cap->subtype = cap->subtype; > >> + return 0; > >> +} > >> + > >> +int vfio_info_add_capability(struct vfio_info_cap *caps, int cap_type_id, > >> + void *cap_type) > >> +{ > >> + int ret = -EINVAL; > >> + > >> + if (!cap_type) > >> + return 0; > >> + > >> + switch (cap_type_id) { > >> + case VFIO_REGION_INFO_CAP_SPARSE_MMAP: > >> + ret = sparse_mmap_cap(caps, cap_type); > >> + break; > >> + > >> + case VFIO_REGION_INFO_CAP_TYPE: > >> + ret = region_type_cap(caps, cap_type); > >> + break; > >> + } > >> + > >> + return ret; > >> +} > >> +EXPORT_SYMBOL(vfio_info_add_capability); > >> > >> /* > >> * Pin a set of guest PFNs and return their associated host PFNs for local > >> diff --git a/include/linux/vfio.h b/include/linux/vfio.h > >> index dcda8fccefab..cf90393a11e2 100644 > >> --- a/include/linux/vfio.h > >> +++ b/include/linux/vfio.h > >> @@ -113,6 +113,9 @@ extern struct vfio_info_cap_header *vfio_info_cap_add( > >> struct vfio_info_cap *caps, size_t size, u16 id, u16 version); > >> 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); > >> + > > > > > > It would make it easier to review and bisect if 14/22 was squashed into > > this one. > > This was split based on Alex's suggestion on earlier version of this > patchset. Yeah, generally squashing patches together is the opposite of what we want for review and bisect. In this case the symbol exports should avoid any defined-but-unused warnings. Thanks, Alex