RE: [PATCH v2 2/5] drm/i915/gvt: OpRegion support for GVT-g

[He, Min]

Xiaoguang,
I have some comments inline. Thanks.

> -----Original Message-----
> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@xxxxxxxxxxxxxxxxxxxxx] On
> Behalf Of Xiaoguang Chen
> Sent: Thursday, May 18, 2017 2:50 AM
> To: alex.williamson@xxxxxxxxxx; kraxel@xxxxxxxxxx; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> zhenyuw@xxxxxxxxxxxxxxx; Lv, Zhiyuan <zhiyuan.lv@xxxxxxxxx>; intel-gvt-
> dev@xxxxxxxxxxxxxxxxxxxxx; Wang, Zhi A <zhi.a.wang@xxxxxxxxx>; Tian, Kevin
> <kevin.tian@xxxxxxxxx>
> Cc: Niu, Bing <bing.niu@xxxxxxxxx>; Chen, Xiaoguang
> <xiaoguang.chen@xxxxxxxxx>
> Subject: [PATCH v2 2/5] drm/i915/gvt: OpRegion support for GVT-g
> 
> OpRegion is needed to support display related operation for
> intel vgpu.
> 
> A vfio device region is added to intel vgpu to deliver the
> host OpRegion information to user space so user space can
> construct the OpRegion for vgpu.
> 
> Signed-off-by: Bing Niu <bing.niu@xxxxxxxxx>
> Signed-off-by: Xiaoguang Chen <xiaoguang.chen@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/gvt/kvmgt.c    | 97
> +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/gvt/opregion.c | 12 ++++-
>  2 files changed, 107 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c
> b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index 3c6a02b..389f072 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -53,6 +53,8 @@
>  #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)
> 
> +#define OPREGION_SIGNATURE "IntelGraphicsMem"
> +
>  struct vfio_region;
>  struct intel_vgpu_regops {
>  	size_t (*rw)(struct intel_vgpu *vgpu, char *buf,
> @@ -436,6 +438,92 @@ static void kvmgt_protect_table_del(struct
> kvmgt_guest_info *info,
>  	}
>  }
> 
> +static size_t intel_vgpu_reg_rw_opregion(struct intel_vgpu *vgpu, char *buf,
> +		size_t count, loff_t *ppos, bool iswrite)
> +{
> +	unsigned int i = VFIO_PCI_OFFSET_TO_INDEX(*ppos) -
> +			VFIO_PCI_NUM_REGIONS;
> +	void *base = vgpu->vdev.region[i].data;
> +	loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
> +
> +	if (pos >= vgpu->vdev.region[i].size || iswrite) {
> +		gvt_vgpu_err("invalid op or offset for Intel vgpu OpRegion\n");
> +		return -EINVAL;
> +	}
> +	count = 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 = {
> +	.rw = intel_vgpu_reg_rw_opregion,
> +	.release = intel_vgpu_reg_release_opregion,
> +};
> +
> +static int intel_vgpu_register_reg(struct intel_vgpu *vgpu,
> +		unsigned int type, unsigned int subtype,
> +		const struct intel_vgpu_regops *ops,
> +		size_t size, u32 flags, void *data)
> +{
> +	struct vfio_region *region;
> +
> +	region = krealloc(vgpu->vdev.region,
> +			(vgpu->vdev.num_regions + 1) * sizeof(*region),
> +			GFP_KERNEL);
> +	if (!region)
> +		return -ENOMEM;
> +
> +	vgpu->vdev.region = region;
> +	vgpu->vdev.region[vgpu->vdev.num_regions].type = type;
> +	vgpu->vdev.region[vgpu->vdev.num_regions].subtype = subtype;
> +	vgpu->vdev.region[vgpu->vdev.num_regions].ops = ops;
> +	vgpu->vdev.region[vgpu->vdev.num_regions].size = size;
> +	vgpu->vdev.region[vgpu->vdev.num_regions].flags = flags;
> +	vgpu->vdev.region[vgpu->vdev.num_regions].data = data;
> +	vgpu->vdev.num_regions++;
> +
> +	return 0;
> +}
> +
> +static int intel_vgpu_reg_init_opregion(struct intel_vgpu *vgpu)
> +{
> +	unsigned int addr;
> +	void *base;
> +	int ret;
> +
> +	addr = vgpu->gvt->opregion.opregion_pa;
> +	if (!addr || !(~addr))
> +		return -ENODEV;
> +
> +	base = memremap(addr, OPREGION_SIZE, MEMREMAP_WB);
> +	if (!base)
> +		return -ENOMEM;
> +
> +	if (memcmp(base, OPREGION_SIGNATURE, 16)) {
> +		memunmap(base);
> +		return -EINVAL;
> +	}
> +
> +	ret = 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);
In current GVT code, we already have a function init_vgpu_opregion,
which will copy the physical opregion into a per vgpu structure: 
vgpu_opregion(vgpu)->va, why donât you reuse this function and set the
vgpu_opregion(vgpu)->va to the intel_vgpu_register_reg function?

> +	if (ret) {
> +		memunmap(base);
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
>  static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
>  {
>  	struct intel_vgpu *vgpu = NULL;
> @@ -467,6 +555,15 @@ static int intel_vgpu_create(struct kobject *kobj, struct
> mdev_device *mdev)
>  	vgpu->vdev.mdev = mdev;
>  	mdev_set_drvdata(mdev, vgpu);
> 
> +	ret = intel_vgpu_reg_init_opregion(vgpu);
> +	if (ret) {
> +		gvt_vgpu_err("create OpRegion failed\n");
> +		goto out;
> +	}
> +
> +	gvt_dbg_core("create OpRegion succeeded for mdev:%s\n",
> +			dev_name(mdev_dev(mdev)));
> +
>  	gvt_dbg_core("intel_vgpu_create succeeded for mdev: %s\n",
>  		     dev_name(mdev_dev(mdev)));
>  	ret = 0;
> diff --git a/drivers/gpu/drm/i915/gvt/opregion.c
> b/drivers/gpu/drm/i915/gvt/opregion.c
> index 3117991..99591bc 100644
> --- a/drivers/gpu/drm/i915/gvt/opregion.c
> +++ b/drivers/gpu/drm/i915/gvt/opregion.c
> @@ -283,14 +283,22 @@ int intel_vgpu_emulate_opregion_request(struct
> intel_vgpu *vgpu, u32 swsci)
>  {
>  	u32 *scic, *parm;
>  	u32 func, subfunc;
> +	u32 scic_val, parm_val;
> +	u32 gpa;
> 
> -	scic = vgpu_opregion(vgpu)->va + INTEL_GVT_OPREGION_SCIC;
> -	parm = vgpu_opregion(vgpu)->va + INTEL_GVT_OPREGION_PARM;
> +	memcpy(&gpa, vgpu_cfg_space(vgpu) + INTEL_GVT_PCI_OPREGION,
> +			sizeof(gpa));
> +	intel_gvt_hypervisor_read_gpa(vgpu, gpa +
> INTEL_GVT_OPREGION_SCIC,
> +			&scic_val, sizeof(scic_val));
> +	intel_gvt_hypervisor_read_gpa(vgpu, gpa +
> INTEL_GVT_OPREGION_PARM,
> +			&parm_val, sizeof(parm_val));
As I said, you can call init_vgpu_opregion(), then vgpu_opregion(vgpu)->va will
be valid and you donât need to change here. 

> 
>  	if (!(swsci & SWSCI_SCI_SELECT)) {
>  		gvt_vgpu_err("requesting SMI service\n");
>  		return 0;
>  	}
> +	scic = &scic_val;
> +	parm = &parm_val;
>  	/* ignore non 0->1 trasitions */
>  	if ((vgpu_cfg_space(vgpu)[INTEL_GVT_PCI_SWSCI]
>  				& SWSCI_SCI_TRIGGER) ||
> --
> 1.9.1
> 
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev