From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42317) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bymxp-00042T-24 for qemu-devel@nongnu.org; Mon, 24 Oct 2016 17:39:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bymxl-0008Mp-SO for qemu-devel@nongnu.org; Mon, 24 Oct 2016 17:39:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33040) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bymxl-0008MV-Kg for qemu-devel@nongnu.org; Mon, 24 Oct 2016 17:39:21 -0400 Date: Mon, 24 Oct 2016 15:39:19 -0600 From: Alex Williamson Message-ID: <20161024153919.2e501eba@t450s.home> In-Reply-To: <4772b79e-c221-7880-02c2-6ff836108f24@nvidia.com> References: <1476739332-4911-1-git-send-email-kwankhede@nvidia.com> <1476739332-4911-6-git-send-email-kwankhede@nvidia.com> <20161020132452.091e525e@t450s.home> <4772b79e-c221-7880-02c2-6ff836108f24@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v9 05/12] vfio: Introduce common function to add capabilities List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kirti Wankhede Cc: pbonzini@redhat.com, kraxel@redhat.com, cjia@nvidia.com, 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 On Tue, 25 Oct 2016 02:57:58 +0530 Kirti Wankhede wrote: > On 10/21/2016 12:54 AM, Alex Williamson wrote: > > On Tue, 18 Oct 2016 02:52:05 +0530 > > 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 | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> include/linux/vfio.h | 4 +++ > >> 2 files changed, 82 insertions(+) > >> > >> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > >> index a5a210005b65..e96cb3f7a23c 100644 > >> --- a/drivers/vfio/vfio.c > >> +++ b/drivers/vfio/vfio.c > >> @@ -1799,6 +1799,84 @@ void vfio_info_cap_shift(struct vfio_info_cap *caps, size_t offset) > >> } > >> EXPORT_SYMBOL_GPL(vfio_info_cap_shift); > >> > >> +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_region_info *info, > >> + struct vfio_info_cap *caps, > >> + int cap_type_id, > >> + void *cap_type) > >> +{ > >> + int ret; > >> + > >> + if (!cap_type) > >> + return 0; > >> + > >> + switch (cap_type_id) { > >> + case VFIO_REGION_INFO_CAP_SPARSE_MMAP: > >> + ret = sparse_mmap_cap(caps, cap_type); > >> + if (ret) > >> + return ret; > >> + break; > >> + > >> + case VFIO_REGION_INFO_CAP_TYPE: > >> + ret = region_type_cap(caps, cap_type); > >> + if (ret) > >> + return ret; > >> + break; > >> + default: > >> + return -EINVAL; > >> + } > >> + > >> + info->flags |= VFIO_REGION_INFO_FLAG_CAPS; > >> + > >> + if (caps->size) { > >> + if (info->argsz < sizeof(*info) + caps->size) { > >> + info->argsz = sizeof(*info) + caps->size; > >> + info->cap_offset = 0; > >> + } else { > >> + vfio_info_cap_shift(caps, sizeof(*info)); > >> + info->cap_offset = sizeof(*info); > > > > This doesn't work. We build the capability chain in a buffer and > > vfio_info_cap_add() expects the chain to be zero-based as each > > capability is added. vfio_info_cap_shift() is meant to be called once > > on that buffer immediately before copying it back to the user buffer to > > adjust the chain offsets to account for the offset within the buffer. > > vfio_info_cap_shift() cannot be called repeatedly on the buffer as we > > do support multiple capabilities in a chain. > > > > From the code I see, we add one type of capability at a time, either > VFIO_REGION_INFO_CAP_SPARSE_MMAP or VFIO_REGION_INFO_CAP_TYPE. Both are > not the part of same case in the switch, right? > I do tested VFIO_REGION_INFO_CAP_SPARSE_MMAP by mapping some part of > BAR0 and that works. That simply means that we don't _currently_ have a user that implements multiple chain entries. The interface is however designed to support multiple entries and this breaks that goal. Thanks, Alex