Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751090AbdLMHHh (ORCPT ); Wed, 13 Dec 2017 02:07:37 -0500 Received: from mga14.intel.com ([192.55.52.115]:43407 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750739AbdLMHHf (ORCPT ); Wed, 13 Dec 2017 02:07:35 -0500 X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,397,1508828400"; d="asc'?scan'208";a="2122101" Date: Wed, 13 Dec 2017 15:01:24 +0800 From: Zhenyu Wang To: Alexey Kardashevskiy Cc: Alex Williamson , linux-kernel@vger.kernel.org, kvm@vger.kernel.org, intel-gvt-dev@lists.freedesktop.org, kwankhede@nvidia.com, zhenyuw@linux.intel.com, zhi.a.wang@intel.com Subject: Re: [PATCH] vfio: Simplify capability helper Message-ID: <20171213070123.jw2isiu5xhjnnqie@zhen-hp.sh.intel.com> Reply-To: Zhenyu Wang References: <20171212195850.13691.40496.stgit@gimli.home> <47fab4f4-3f13-312c-aff8-5b3576b12578@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="5bewpx5m4omtz7r2" Content-Disposition: inline In-Reply-To: <47fab4f4-3f13-312c-aff8-5b3576b12578@ozlabs.ru> User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8969 Lines: 268 --5bewpx5m4omtz7r2 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On 2017.12.13 12:13:34 +1100, Alexey Kardashevskiy wrote: > On 13/12/17 06:59, Alex Williamson wrote: > > The vfio_info_add_capability() helper requires the caller to pass a > > capability ID, which it then uses to fill in header fields, assuming > > hard coded versions. This makes for an awkward and rigid interface. > > The only thing we want this helper to do is allocate sufficient > > space in the caps buffer and chain this capability into the list. > > Reduce it to that simple task. > >=20 > > Signed-off-by: Alex Williamson >=20 >=20 > Makes more sense now, thanks. I'll repost mine on top of this. >=20 >=20 > Reviewed-by: Alexey Kardashevskiy >=20 > Looks good for KVMGT part. Acked-by: Zhenyu Wang > Below one observation, unrelated to this patch. >=20 > > --- > > drivers/gpu/drm/i915/gvt/kvmgt.c | 15 +++++++---- > > drivers/vfio/pci/vfio_pci.c | 14 ++++++---- > > drivers/vfio/vfio.c | 52 +++---------------------------= -------- > > include/linux/vfio.h | 3 +- > > 4 files changed, 24 insertions(+), 60 deletions(-) > >=20 > > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gv= t/kvmgt.c > > index 96060920a6fe..0a7d084da1a2 100644 > > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c > > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c > > @@ -1012,6 +1012,8 @@ static long intel_vgpu_ioctl(struct mdev_device *= mdev, unsigned int cmd, > > if (!sparse) > > return -ENOMEM; > > =20 > > + sparse->header.id =3D VFIO_REGION_INFO_CAP_SPARSE_MMAP; > > + sparse->header.version =3D 1; > > sparse->nr_areas =3D nr_areas; > > cap_type_id =3D VFIO_REGION_INFO_CAP_SPARSE_MMAP; >=20 >=20 > @cap_type_id is initialized in just one of many cases of switch > (info.index) and after the entire switch, there is switch (cap_type_id). I > wonder why compiler missed "potentially uninitialized variable here, > although there is no bug - @cap_type_id is in sync with @spapse. It would > make it cleaner imho just to have vfio_info_add_capability() next to the > header initialization. >=20 yeah, we could clean that up, thanks for pointing out. >=20 >=20 > > sparse->areas[0].offset =3D > > @@ -1033,7 +1035,9 @@ static long intel_vgpu_ioctl(struct mdev_device *= mdev, unsigned int cmd, > > break; > > default: > > { > > - struct vfio_region_info_cap_type cap_type; > > + struct vfio_region_info_cap_type cap_type =3D { > > + .header.id =3D VFIO_REGION_INFO_CAP_TYPE, > > + .header.version =3D 1 }; > > =20 > > if (info.index >=3D VFIO_PCI_NUM_REGIONS + > > vgpu->vdev.num_regions) > > @@ -1050,8 +1054,8 @@ static long intel_vgpu_ioctl(struct mdev_device *= mdev, unsigned int cmd, > > cap_type.subtype =3D vgpu->vdev.region[i].subtype; > > =20 > > ret =3D vfio_info_add_capability(&caps, > > - VFIO_REGION_INFO_CAP_TYPE, > > - &cap_type); > > + &cap_type.header, > > + sizeof(cap_type)); > > if (ret) > > return ret; > > } > > @@ -1061,8 +1065,9 @@ static long intel_vgpu_ioctl(struct mdev_device *= mdev, unsigned int cmd, > > switch (cap_type_id) { > > case VFIO_REGION_INFO_CAP_SPARSE_MMAP: > > ret =3D vfio_info_add_capability(&caps, > > - VFIO_REGION_INFO_CAP_SPARSE_MMAP, > > - sparse); > > + &sparse->header, sizeof(*sparse) + > > + (sparse->nr_areas * > > + sizeof(*sparse->areas))); > > kfree(sparse); > > if (ret) > > return ret; > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c > > index f041b1a6cf66..a73e40983880 100644 > > --- a/drivers/vfio/pci/vfio_pci.c > > +++ b/drivers/vfio/pci/vfio_pci.c > > @@ -582,6 +582,8 @@ static int msix_sparse_mmap_cap(struct vfio_pci_dev= ice *vdev, > > if (!sparse) > > return -ENOMEM; > > =20 > > + sparse->header.id =3D VFIO_REGION_INFO_CAP_SPARSE_MMAP; > > + sparse->header.version =3D 1; > > sparse->nr_areas =3D nr_areas; > > =20 > > if (vdev->msix_offset & PAGE_MASK) { > > @@ -597,8 +599,7 @@ static int msix_sparse_mmap_cap(struct vfio_pci_dev= ice *vdev, > > i++; > > } > > =20 > > - ret =3D vfio_info_add_capability(caps, VFIO_REGION_INFO_CAP_SPARSE_MM= AP, > > - sparse); > > + ret =3D vfio_info_add_capability(caps, &sparse->header, size); > > kfree(sparse); > > =20 > > return ret; > > @@ -741,7 +742,9 @@ static long vfio_pci_ioctl(void *device_data, > > break; > > default: > > { > > - struct vfio_region_info_cap_type cap_type; > > + struct vfio_region_info_cap_type cap_type =3D { > > + .header.id =3D VFIO_REGION_INFO_CAP_TYPE, > > + .header.version =3D 1 }; > > =20 > > if (info.index >=3D > > VFIO_PCI_NUM_REGIONS + vdev->num_regions) > > @@ -756,9 +759,8 @@ static long vfio_pci_ioctl(void *device_data, > > cap_type.type =3D vdev->region[i].type; > > cap_type.subtype =3D vdev->region[i].subtype; > > =20 > > - ret =3D vfio_info_add_capability(&caps, > > - VFIO_REGION_INFO_CAP_TYPE, > > - &cap_type); > > + ret =3D vfio_info_add_capability(&caps, &cap_type.header, > > + sizeof(cap_type)); > > if (ret) > > return ret; > > =20 > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > > index 2bc3705a99bd..721f97f8dac1 100644 > > --- a/drivers/vfio/vfio.c > > +++ b/drivers/vfio/vfio.c > > @@ -1857,63 +1857,19 @@ void vfio_info_cap_shift(struct vfio_info_cap *= caps, size_t offset) > > } > > EXPORT_SYMBOL(vfio_info_cap_shift); > > =20 > > -static int sparse_mmap_cap(struct vfio_info_cap *caps, void *cap_type) > > +int vfio_info_add_capability(struct vfio_info_cap *caps, > > + struct vfio_info_cap_header *cap, size_t size) > > { > > struct vfio_info_cap_header *header; > > - struct vfio_region_info_cap_sparse_mmap *sparse_cap, *sparse =3D cap_= type; > > - size_t size; > > =20 > > - size =3D sizeof(*sparse) + sparse->nr_areas * sizeof(*sparse->areas); > > - header =3D vfio_info_cap_add(caps, size, > > - VFIO_REGION_INFO_CAP_SPARSE_MMAP, 1); > > + header =3D vfio_info_cap_add(caps, size, cap->id, cap->version); > > if (IS_ERR(header)) > > return PTR_ERR(header); > > =20 > > - sparse_cap =3D container_of(header, > > - struct vfio_region_info_cap_sparse_mmap, header); > > - sparse_cap->nr_areas =3D 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 =3D cap_type; > > + memcpy(header + 1, cap + 1, size - sizeof(*header)); > > =20 > > - header =3D vfio_info_cap_add(caps, sizeof(*cap), > > - VFIO_REGION_INFO_CAP_TYPE, 1); > > - if (IS_ERR(header)) > > - return PTR_ERR(header); > > - > > - type_cap =3D container_of(header, struct vfio_region_info_cap_type, > > - header); > > - type_cap->type =3D cap->type; > > - type_cap->subtype =3D cap->subtype; > > return 0; > > } > > - > > -int vfio_info_add_capability(struct vfio_info_cap *caps, int cap_type_= id, > > - void *cap_type) > > -{ > > - int ret =3D -EINVAL; > > - > > - if (!cap_type) > > - return 0; > > - > > - switch (cap_type_id) { > > - case VFIO_REGION_INFO_CAP_SPARSE_MMAP: > > - ret =3D sparse_mmap_cap(caps, cap_type); > > - break; > > - > > - case VFIO_REGION_INFO_CAP_TYPE: > > - ret =3D region_type_cap(caps, cap_type); > > - break; > > - } > > - > > - return ret; > > -} > > EXPORT_SYMBOL(vfio_info_add_capability); > > =20 > > int vfio_set_irqs_validate_and_prepare(struct vfio_irq_set *hdr, int n= um_irqs, > > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > > index a47b985341d1..66741ab087c1 100644 > > --- a/include/linux/vfio.h > > +++ b/include/linux/vfio.h > > @@ -145,7 +145,8 @@ extern struct vfio_info_cap_header *vfio_info_cap_a= dd( > > extern void vfio_info_cap_shift(struct vfio_info_cap *caps, size_t off= set); > > =20 > > extern int vfio_info_add_capability(struct vfio_info_cap *caps, > > - int cap_type_id, void *cap_type); > > + struct vfio_info_cap_header *cap, > > + size_t size); > > =20 > > extern int vfio_set_irqs_validate_and_prepare(struct vfio_irq_set *hdr, > > int num_irqs, int max_irq_type, > >=20 >=20 >=20 > --=20 > Alexey --=20 Open Source Technology Center, Intel ltd. $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827 --5bewpx5m4omtz7r2 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EARECAB0WIQTXuabgHDW6LPt9CICxBBozTXgYJwUCWjDQQwAKCRCxBBozTXgY J6WyAJ9hjVqP6nOl895u9cmupxRx1/At4QCeJzLLjWJeP4skyonTfL7wAed80Zo= =H6+I -----END PGP SIGNATURE----- --5bewpx5m4omtz7r2--