> -----Original Message----- > From: Chen, Xiaoguang > Sent: Thursday, May 18, 2017 7:20 PM > To: He, Min <[email protected]>; [email protected]; > [email protected]; [email protected]; linux- > [email protected]; [email protected]; Lv, Zhiyuan > <[email protected]>; [email protected]; Wang, Zhi A > <[email protected]>; Tian, Kevin <[email protected]> > Cc: Niu, Bing <[email protected]> > Subject: RE: [PATCH v2 2/5] drm/i915/gvt: OpRegion support for GVT-g > > Hi min, > > >-----Original Message----- > >From: He, Min > >Sent: Thursday, May 18, 2017 11:44 PM > >To: Chen, Xiaoguang <[email protected]>; > >[email protected]; [email protected]; intel- > >[email protected]; [email protected]; > >[email protected]; Lv, Zhiyuan <[email protected]>; intel-gvt- > >[email protected]; Wang, Zhi A <[email protected]>; Tian, Kevin > ><[email protected]> > >Cc: Niu, Bing <[email protected]>; Chen, Xiaoguang > ><[email protected]> > >Subject: RE: [PATCH v2 2/5] drm/i915/gvt: OpRegion support for GVT-g > > > >Xiaoguang, > >I have some comments inline. Thanks. > > > >> -----Original Message----- > >> From: intel-gvt-dev > >> [mailto:[email protected]] On Behalf Of > >> Xiaoguang Chen > >> Sent: Thursday, May 18, 2017 2:50 AM > >> To: [email protected]; [email protected]; intel- > >> [email protected]; [email protected]; > >> [email protected]; Lv, Zhiyuan <[email protected]>; > >> intel-gvt- [email protected]; Wang, Zhi A > >> <[email protected]>; Tian, Kevin <[email protected]> > >> Cc: Niu, Bing <[email protected]>; Chen, Xiaoguang > >> <[email protected]> > >> 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 <[email protected]> > >> Signed-off-by: Xiaoguang Chen <[email protected]> > >> --- > >> 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? > The reason is we do not the know the gpa of the OpRegion. > For KVMGT the gpa of the OpRegion is allocated by seabios.
Maybe you can consolidate them into one function to remove duplicate code.
