From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756689AbdJKEgT (ORCPT ); Wed, 11 Oct 2017 00:36:19 -0400 Received: from mga06.intel.com ([134.134.136.31]:61025 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750718AbdJKEgS (ORCPT ); Wed, 11 Oct 2017 00:36:18 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.43,359,1503385200"; d="asc'?scan'208";a="1023911595" Date: Wed, 11 Oct 2017 12:29:29 +0800 From: "Du, Changbin" To: Tina Zhang Cc: alex.williamson@redhat.com, kraxel@redhat.com, chris@chris-wilson.co.uk, zhenyuw@linux.intel.com, zhiyuan.lv@intel.com, zhi.a.wang@intel.com, kevin.tian@intel.com, daniel@ffwll.ch, kwankhede@nvidia.com, intel-gfx@lists.freedesktop.org, Bing Niu , intel-gvt-dev@lists.freedesktop.org, linux-kernel@vger.kernel.org, changbin.du@intel.com Subject: Re: [PATCH v15 4/7] drm/i915/gvt: Add opregion support Message-ID: <20171011042929.GA29216@intel.com> References: <1507629007-3183-1-git-send-email-tina.zhang@intel.com> <1507629007-3183-5-git-send-email-tina.zhang@intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="nFreZHaLTZJo0R7j" Content-Disposition: inline In-Reply-To: <1507629007-3183-5-git-send-email-tina.zhang@intel.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --nFreZHaLTZJo0R7j Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Oct 10, 2017 at 05:50:04PM +0800, Tina Zhang wrote: > Windows guest driver needs vbt in opregion, to configure the setting > for display. Without opregion support, the display registers won't > be set and this blocks display model to get the correct information > of the guest display plane. >=20 > This patch is to provide a virtual opregion for guest. Current > implementation is to fill the virtual opregion with the content in > host's opregion. The original author of this patch is Xiaoguang Chen. >=20 > Signed-off-by: Bing Niu > Signed-off-by: Tina Zhang > --- > drivers/gpu/drm/i915/gvt/hypercall.h | 1 + > drivers/gpu/drm/i915/gvt/kvmgt.c | 109 +++++++++++++++++++++++++++++= +++++- > drivers/gpu/drm/i915/gvt/mpt.h | 15 +++++ > drivers/gpu/drm/i915/gvt/opregion.c | 26 +++++++-- > drivers/gpu/drm/i915/gvt/vgpu.c | 4 ++ > 5 files changed, 146 insertions(+), 9 deletions(-) >=20 > diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h b/drivers/gpu/drm/i915/= gvt/hypercall.h > index df7f33a..32c345c 100644 > --- a/drivers/gpu/drm/i915/gvt/hypercall.h > +++ b/drivers/gpu/drm/i915/gvt/hypercall.h > @@ -55,6 +55,7 @@ struct intel_gvt_mpt { > unsigned long mfn, unsigned int nr, bool map); > int (*set_trap_area)(unsigned long handle, u64 start, u64 end, > bool map); > + int (*set_opregion)(void *vgpu); Seems we try to hide struct intel_vgpu for kvmgt, but acctually kvmgt alrea= dy use it. So set type as struct intel_vgpu directly? I am not sure about xeng= t, but the code shows that 'handle' is correct thing? > }; > =20 > extern struct intel_gvt_mpt xengt_mpt; > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/= kvmgt.c > index fd0c85f..6b0a330 100644 > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c > @@ -53,11 +53,23 @@ static const struct intel_gvt_ops *intel_gvt_ops; > #define VFIO_PCI_INDEX_TO_OFFSET(index) ((u64)(index) << VFIO_PCI_OFFSET= _SHIFT) > #define VFIO_PCI_OFFSET_MASK (((u64)(1) << VFIO_PCI_OFFSET_SHIFT) - 1) > =20 > +#define OPREGION_SIGNATURE "IntelGraphicsMem" > + > +struct vfio_region; > +struct intel_vgpu_regops { > + size_t (*rw)(struct intel_vgpu *vgpu, char *buf, > + size_t count, loff_t *ppos, bool iswrite); > + void (*release)(struct intel_vgpu *vgpu, > + struct vfio_region *region); > +}; > + > struct vfio_region { > u32 type; > u32 subtype; > size_t size; > u32 flags; > + const struct intel_vgpu_regops *ops; > + void *data; > }; > =20 > struct kvmgt_pgfn { > @@ -430,6 +442,91 @@ static void kvmgt_protect_table_del(struct kvmgt_gue= st_info *info, > } > } > =20 > +static size_t intel_vgpu_reg_rw_opregion(struct intel_vgpu *vgpu, char *= buf, > + size_t count, loff_t *ppos, bool iswrite) Personally I think intel_vgpu_rw_opregion() is better. :) > +{ > + unsigned int i =3D VFIO_PCI_OFFSET_TO_INDEX(*ppos) - > + VFIO_PCI_NUM_REGIONS; > + void *base =3D vgpu->vdev.region[i].data; > + loff_t pos =3D *ppos & VFIO_PCI_OFFSET_MASK; > + > + if (pos >=3D vgpu->vdev.region[i].size || iswrite) { > + gvt_vgpu_err("invalid op or offset for Intel vgpu OpRegion\n"); > + return -EINVAL; > + } > + count =3D min(count, (size_t)(vgpu->vdev.region[i].size - pos)); > + memcpy(buf, base + pos, count); > + > + return count; > +} > + > +static void intel_vgpu_reg_release_opregion(struct intel_vgpu *vgpu, > + struct vfio_region *region) > +{ > + memunmap(region->data); > +} > + > +static const struct intel_vgpu_regops intel_vgpu_regops_opregion =3D { > + .rw =3D intel_vgpu_reg_rw_opregion, > + .release =3D intel_vgpu_reg_release_opregion, > +}; > + > +static int intel_vgpu_register_reg(struct intel_vgpu *vgpu, Maybe full name xx_register_region? coufusing between a register and region. > + unsigned int type, unsigned int subtype, > + const struct intel_vgpu_regops *ops, > + size_t size, u32 flags, void *data) > +{ > + struct vfio_region *region; > + > + region =3D krealloc(vgpu->vdev.region, > + (vgpu->vdev.num_regions + 1) * sizeof(*region), > + GFP_KERNEL); > + if (!region) > + return -ENOMEM; > + > + vgpu->vdev.region =3D region; > + vgpu->vdev.region[vgpu->vdev.num_regions].type =3D type; > + vgpu->vdev.region[vgpu->vdev.num_regions].subtype =3D subtype; > + vgpu->vdev.region[vgpu->vdev.num_regions].ops =3D ops; > + vgpu->vdev.region[vgpu->vdev.num_regions].size =3D size; > + vgpu->vdev.region[vgpu->vdev.num_regions].flags =3D flags; > + vgpu->vdev.region[vgpu->vdev.num_regions].data =3D data; > + vgpu->vdev.num_regions++; > + > + return 0; > +} > + > +static int kvmgt_set_opregion(void *p_vgpu) > +{ > + struct intel_vgpu *vgpu =3D (struct intel_vgpu *)p_vgpu; > + unsigned int addr; > + void *base; > + int ret; > + > + addr =3D vgpu->gvt->opregion.opregion_pa; > + if (!addr || !(~addr)) > + return -ENODEV; > + > + base =3D memremap(addr, OPREGION_SIZE, MEMREMAP_WB); > + if (!base) > + return -ENOMEM; No need map again, i915 has mapped it (dev_priv->opregion->header). > + > + if (memcmp(base, OPREGION_SIGNATURE, 16)) { > + memunmap(base); > + return -EINVAL; > + } > + Just check "opregion->header !=3D NULL", since i915 already validates it. > + ret =3D intel_vgpu_register_reg(vgpu, > + PCI_VENDOR_ID_INTEL | VFIO_REGION_TYPE_PCI_VENDOR_TYPE, > + VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION, > + &intel_vgpu_regops_opregion, OPREGION_SIZE, > + VFIO_REGION_INFO_FLAG_READ, base); > + if (ret) > + memunmap(base); > + > + return ret; > +} > + > static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *m= dev) > { > struct intel_vgpu *vgpu =3D NULL; > @@ -646,7 +743,7 @@ static ssize_t intel_vgpu_rw(struct mdev_device *mdev= , char *buf, > int ret =3D -EINVAL; > =20 > =20 > - if (index >=3D VFIO_PCI_NUM_REGIONS) { > + if (index >=3D VFIO_PCI_NUM_REGIONS + vgpu->vdev.num_regions) { > gvt_vgpu_err("invalid index: %u\n", index); > return -EINVAL; > } > @@ -680,8 +777,11 @@ static ssize_t intel_vgpu_rw(struct mdev_device *mde= v, char *buf, > case VFIO_PCI_BAR5_REGION_INDEX: > case VFIO_PCI_VGA_REGION_INDEX: > case VFIO_PCI_ROM_REGION_INDEX: > + break; > default: > - gvt_vgpu_err("unsupported region: %u\n", index); > + index -=3D VFIO_PCI_NUM_REGIONS; > + return vgpu->vdev.region[index].ops->rw(vgpu, buf, count, > + ppos, is_write); Make region ops gerneric is great. You can factor out MMIO and CFG region o= ps as well. It is better not to mix generic and special handling. > } > =20 > return ret =3D=3D 0 ? count : ret; > @@ -944,7 +1044,8 @@ static long intel_vgpu_ioctl(struct mdev_device *mde= v, unsigned int cmd, > =20 > info.flags =3D VFIO_DEVICE_FLAGS_PCI; > info.flags |=3D VFIO_DEVICE_FLAGS_RESET; > - info.num_regions =3D VFIO_PCI_NUM_REGIONS; > + info.num_regions =3D VFIO_PCI_NUM_REGIONS + > + vgpu->vdev.num_regions; > info.num_irqs =3D VFIO_PCI_NUM_IRQS; > =20 > return copy_to_user((void __user *)arg, &info, minsz) ? > @@ -1065,6 +1166,7 @@ static long intel_vgpu_ioctl(struct mdev_device *md= ev, unsigned int cmd, > } > =20 > if (caps.size) { > + info.flags |=3D VFIO_REGION_INFO_FLAG_CAPS; > if (info.argsz < sizeof(info) + caps.size) { > info.argsz =3D sizeof(info) + caps.size; > info.cap_offset =3D 0; > @@ -1513,6 +1615,7 @@ struct intel_gvt_mpt kvmgt_mpt =3D { > .read_gpa =3D kvmgt_read_gpa, > .write_gpa =3D kvmgt_write_gpa, > .gfn_to_mfn =3D kvmgt_gfn_to_pfn, > + .set_opregion =3D kvmgt_set_opregion, > }; > EXPORT_SYMBOL_GPL(kvmgt_mpt); > =20 > diff --git a/drivers/gpu/drm/i915/gvt/mpt.h b/drivers/gpu/drm/i915/gvt/mp= t.h > index f0e5487..9e73f2e 100644 > --- a/drivers/gpu/drm/i915/gvt/mpt.h > +++ b/drivers/gpu/drm/i915/gvt/mpt.h > @@ -292,4 +292,19 @@ static inline int intel_gvt_hypervisor_set_trap_area( > return intel_gvt_host.mpt->set_trap_area(vgpu->handle, start, end, map); > } > =20 > +/** > + * intel_gvt_hypervisor_set_opregion - Set opregion for guest > + * @vgpu: a vGPU > + * > + * Returns: > + * Zero on success, negative error code if failed. > + */ > +static inline int intel_gvt_hypervisor_set_opregion(struct intel_vgpu *v= gpu) > +{ > + if (!intel_gvt_host.mpt->set_opregion) > + return 0; > + > + return intel_gvt_host.mpt->set_opregion(vgpu); > +} > + > #endif /* _GVT_MPT_H_ */ > diff --git a/drivers/gpu/drm/i915/gvt/opregion.c b/drivers/gpu/drm/i915/g= vt/opregion.c > index 3117991..04c0452 100644 > --- a/drivers/gpu/drm/i915/gvt/opregion.c > +++ b/drivers/gpu/drm/i915/gvt/opregion.c > @@ -113,23 +113,37 @@ void intel_vgpu_clean_opregion(struct intel_vgpu *v= gpu) > */ > int intel_vgpu_init_opregion(struct intel_vgpu *vgpu, u32 gpa) > { > - int ret; > + int ret =3D 0; > + unsigned long pfn; > =20 > gvt_dbg_core("vgpu%d: init vgpu opregion\n", vgpu->id); > =20 > - if (intel_gvt_host.hypervisor_type =3D=3D INTEL_GVT_HYPERVISOR_XEN) { > + switch (intel_gvt_host.hypervisor_type) { > + case INTEL_GVT_HYPERVISOR_KVM: > + pfn =3D intel_gvt_hypervisor_gfn_to_mfn(vgpu, gpa >> PAGE_SHIFT); > + vgpu_opregion(vgpu)->va =3D memremap(pfn << PAGE_SHIFT, > + INTEL_GVT_OPREGION_SIZE, > + MEMREMAP_WB); > + if (!vgpu_opregion(vgpu)->va) { > + gvt_vgpu_err("failed to map guest opregion\n"); > + ret =3D -EFAULT; > + } ditto. > + break; > + case INTEL_GVT_HYPERVISOR_XEN: > gvt_dbg_core("emulate opregion from kernel\n"); > =20 > ret =3D init_vgpu_opregion(vgpu, gpa); > if (ret) > - return ret; > + break; > =20 > ret =3D map_vgpu_opregion(vgpu, true); > - if (ret) > - return ret; > + break; > + default: > + ret =3D -EINVAL; > + gvt_vgpu_err("not supported hypervisor\n"); > } > =20 > - return 0; > + return ret; > } > =20 > /** > diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/v= gpu.c > index 3deadcb..bcbf535 100644 > --- a/drivers/gpu/drm/i915/gvt/vgpu.c > +++ b/drivers/gpu/drm/i915/gvt/vgpu.c > @@ -383,6 +383,10 @@ static struct intel_vgpu *__intel_gvt_create_vgpu(st= ruct intel_gvt *gvt, > if (ret) > goto out_clean_shadow_ctx; > =20 > + ret =3D intel_gvt_hypervisor_set_opregion(vgpu); > + if (ret) > + goto out_clean_shadow_ctx; > + > mutex_unlock(&gvt->lock); > =20 > return vgpu; > --=20 > 2.7.4 >=20 > _______________________________________________ > intel-gvt-dev mailing list > intel-gvt-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev --=20 Thanks, Changbin Du --nFreZHaLTZJo0R7j Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJZ3Z4pAAoJEAanuZwLnPNURzQIAIwhomAWkga+lUeGd8aoruox EiZ+eh4rK466TUswaurUxZkSiDRrZt1zhyoK50by8jxws7oFtLecYUnx54/iJZcC nZsvZEW6Xjd/I9fAiJUqB5PmwDq0Neep4/QjFcNMirWd9jBre4zaHQJTdMETuSrK oDi4xlkRvf1Cjg4cGAsCbqT+bgq6d0DCwD9BCBbvBS15WSqC1GrIrEqEYnf0ZT3W 9N+ctV6bH5zvo8RljIVwbrexFRG7phjmWiuqwQhyQmQJhUn+Q+ot66iwer0SyV+Q IgH829Mx7CcTtzozFgSqI3RWlqn571wGS5lXtrwn9AlSYHQJhFh2VqJPiVdqpA4= =85xF -----END PGP SIGNATURE----- --nFreZHaLTZJo0R7j--