[PATCH] drm/i915/gvt: Use KVM r/w to access guest opregion
Zhang, Tina
tina.zhang at intel.com
Fri Jan 26 06:53:44 UTC 2018
> -----Original Message-----
> From: Wang, Zhenyu Z
> Sent: Friday, January 26, 2018 2:29 PM
> To: Zhang, Tina <tina.zhang at intel.com>
> Cc: intel-gvt-dev at lists.freedesktop.org; Zhao, Yan Y <yan.y.zhao at intel.com>;
> Zhang, Xiong Y <xiong.y.zhang at intel.com>
> Subject: Re: [PATCH] drm/i915/gvt: Use KVM r/w to access guest opregion
>
> On 2018.01.26 00:03:03 +0000, Zhang, Tina wrote:
> > > On 2018.01.25 16:40:21 +0800, Tina Zhang wrote:
> > > > For KVMGT, the guest opregion, which is handled by VFIO, is
> > > > actually a piece of guest memory which won't be accessed by
> > > > devices. So, its mfn shouldn't be obtained through VFIO interface.
> > > > This patch uses KVM r/w interface to access the data in guest opregion.
> > > >
> > >
> > > gvt uses virtualized opregion for vgpu, why not just r/w vgpu opregion?
> > As the comments said in "kvmgt.c":
> >
> > static int kvmgt_set_opregion(void *p_vgpu) {
> > struct intel_vgpu *vgpu = (struct intel_vgpu *)p_vgpu;
> > void *base;
> > int ret;
> >
> > /* Each vgpu has its own opregion, although VFIO would create another
> > * one later. This one is used to expose opregion to VFIO. And the
> > * other one created by VFIO later, is used by guest actually.
> > */
> > base = vgpu_opregion(vgpu)->va;
> > if (!base)
> > return -ENOMEM;
> >
> > Here "This one" means the vgpu opregion created and populated by device
> model.
> > "the other one" is actually used by guest VM.
> >
>
> yeah, forgot that one has already been setup, comment below.
>
>
> > > > Fix the guest opregion accessing issue when host "intel_iommu=on".
> > > >
> > > > Signed-off-by: Tina Zhang <tina.zhang at intel.com>
> > > > Cc: Yan Zhao <yan.y.zhao at intel.com>
> > > > Cc: Xiong Zhang <xiong.y.zhang at intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/gvt/gvt.h | 1 -
> > > > drivers/gpu/drm/i915/gvt/opregion.c | 95
> > > > ++++++++++++++++++++++++++-----------
> > > > 2 files changed, 68 insertions(+), 28 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gvt/gvt.h
> > > > b/drivers/gpu/drm/i915/gvt/gvt.h index 7dc7a80..205d88d 100644
> > > > --- a/drivers/gpu/drm/i915/gvt/gvt.h
> > > > +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> > > > @@ -127,7 +127,6 @@ struct intel_vgpu_irq { struct
> > > > intel_vgpu_opregion {
> > > > bool mapped;
> > > > void *va;
> > > > - void *va_gopregion;
> > > > u32 gfn[INTEL_GVT_OPREGION_PAGES]; };
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gvt/opregion.c
> > > > b/drivers/gpu/drm/i915/gvt/opregion.c
> > > > index 8420d1f..4030233 100644
> > > > --- a/drivers/gpu/drm/i915/gvt/opregion.c
> > > > +++ b/drivers/gpu/drm/i915/gvt/opregion.c
> > > > @@ -299,20 +299,13 @@ int
> > > > intel_vgpu_opregion_base_write_handler(struct intel_vgpu *vgpu,
> > > > u32
> > > > gpa) {
> > > >
> > > > int i, ret = 0;
> > > > - unsigned long pfn;
> > > >
> > > > gvt_dbg_core("emulate opregion from kernel\n");
> > > >
> > > > switch (intel_gvt_host.hypervisor_type) {
> > > > case INTEL_GVT_HYPERVISOR_KVM:
> > > > - pfn = intel_gvt_hypervisor_gfn_to_mfn(vgpu, gpa >>
> > > PAGE_SHIFT);
> > > > - vgpu_opregion(vgpu)->va_gopregion = memremap(pfn <<
> > > PAGE_SHIFT,
> > > > - INTEL_GVT_OPREGION_SIZE,
> > > > - MEMREMAP_WB);
> > > > - if (!vgpu_opregion(vgpu)->va_gopregion) {
> > > > - gvt_vgpu_err("failed to map guest opregion\n");
> > > > - ret = -EFAULT;
> > > > - }
> > > > + for (i = 0; i < INTEL_GVT_OPREGION_PAGES; i++)
> > > > + vgpu_opregion(vgpu)->gfn[i] = (gpa >> PAGE_SHIFT) +
> > > i;
> > > > vgpu_opregion(vgpu)->mapped = true;
> > > > break;
> > > > case INTEL_GVT_HYPERVISOR_XEN:
> > > > @@ -352,10 +345,7 @@ void intel_vgpu_clean_opregion(struct
> > > > intel_vgpu
> > > *vgpu)
> > > > if (vgpu_opregion(vgpu)->mapped)
> > > > map_vgpu_opregion(vgpu, false);
> > > > } else if (intel_gvt_host.hypervisor_type ==
> > > INTEL_GVT_HYPERVISOR_KVM) {
> > > > - if (vgpu_opregion(vgpu)->mapped) {
> > > > - memunmap(vgpu_opregion(vgpu)->va_gopregion);
> > > > - vgpu_opregion(vgpu)->va_gopregion = NULL;
> > > > - }
> > > > + /* Guest opregion is released by VFIO */
> > > > }
> > > > free_pages((unsigned long)vgpu_opregion(vgpu)->va,
> > > > get_order(INTEL_GVT_OPREGION_SIZE));
> > > > @@ -480,19 +470,38 @@ static bool querying_capabilities(u32 scic)
> > > > */
> > > > int intel_vgpu_emulate_opregion_request(struct intel_vgpu *vgpu,
> > > > u32
> > > > swsci) {
> > > > - u32 *scic, *parm;
> > > > + u32 scic, parm;
> > > > u32 func, subfunc;
> > > > + u64 scic_pa = 0, parm_pa = 0;
> > > > + int ret;
> > > >
> > > > switch (intel_gvt_host.hypervisor_type) {
> > > > case INTEL_GVT_HYPERVISOR_XEN:
> > > > - scic = vgpu_opregion(vgpu)->va +
> > > INTEL_GVT_OPREGION_SCIC;
> > > > - parm = vgpu_opregion(vgpu)->va +
> > > INTEL_GVT_OPREGION_PARM;
> > > > + scic = *((u32 *)vgpu_opregion(vgpu)->va +
> > > > + INTEL_GVT_OPREGION_SCIC);
> > > > + parm = *((u32 *)vgpu_opregion(vgpu)->va +
> > > > + INTEL_GVT_OPREGION_PARM);
> > > > break;
> > > > case INTEL_GVT_HYPERVISOR_KVM:
> > > > - scic = vgpu_opregion(vgpu)->va_gopregion +
> > > > - INTEL_GVT_OPREGION_SCIC;
> > > > - parm = vgpu_opregion(vgpu)->va_gopregion +
> > > > -
> > > INTEL_GVT_OPREGION_PARM;
> > > > + scic_pa = vgpu_opregion(vgpu)->gfn[0] +
> > > INTEL_GVT_OPREGION_SCIC;
> > > > + parm_pa = vgpu_opregion(vgpu)->gfn[0] +
> > > INTEL_GVT_OPREGION_PARM;
>
> Looks mis use of gfn for gpa here?
Indeed.
Tina
>
> > > > +
> > > > + ret = intel_gvt_hypervisor_read_gpa(vgpu, scic_pa,
> > > > + &scic, sizeof(scic));
> > > > + if (ret) {
> > > > + gvt_vgpu_err("guet opregion read error %d, gpa
> > > 0x%llx, len %lu\n",
> > > > + ret, scic_pa, sizeof(scic));
> > > > + return ret;
> > > > + }
> > > > +
> > > > + ret = intel_gvt_hypervisor_read_gpa(vgpu, parm_pa,
> > > > + &parm, sizeof(parm));
> > > > + if (ret) {
> > > > + gvt_vgpu_err("guet opregion read error %d, gpa
> > > 0x%llx, len %lu\n",
> > > > + ret, scic_pa, sizeof(scic));
> > > > + return ret;
> > > > + }
> > > > +
> > > > break;
> > > > default:
> > > > gvt_vgpu_err("not supported hypervisor\n"); @@ -510,9
> > > +519,9 @@ int
> > > > intel_vgpu_emulate_opregion_request(struct intel_vgpu *vgpu, u32 swsci)
> > > > return 0;
> > > > }
> > > >
> > > > - func = GVT_OPREGION_FUNC(*scic);
> > > > - subfunc = GVT_OPREGION_SUBFUNC(*scic);
> > > > - if (!querying_capabilities(*scic)) {
> > > > + func = GVT_OPREGION_FUNC(scic);
> > > > + subfunc = GVT_OPREGION_SUBFUNC(scic);
> > > > + if (!querying_capabilities(scic)) {
> > > > gvt_vgpu_err("requesting runtime service: func \"%s\","
> > > > " subfunc \"%s\"\n",
> > > > opregion_func_name(func),
> > > > @@ -521,11 +530,43 @@ int
> > > > intel_vgpu_emulate_opregion_request(struct
> > > intel_vgpu *vgpu, u32 swsci)
> > > > * emulate exit status of function call, '0' means
> > > > * "failure, generic, unsupported or unknown cause"
> > > > */
> > > > - *scic &= ~OPREGION_SCIC_EXIT_MASK;
> > > > - return 0;
> > > > + scic &= ~OPREGION_SCIC_EXIT_MASK;
> > > > + goto out;
> > > > + }
> > > > +
> > > > + scic = 0;
> > > > + parm = 0;
> > > > +
> > > > +out:
> > > > + switch (intel_gvt_host.hypervisor_type) {
> > > > + case INTEL_GVT_HYPERVISOR_XEN:
> > > > + *((u32 *)vgpu_opregion(vgpu)->va +
> > > > + INTEL_GVT_OPREGION_SCIC) = scic;
> > > > + *((u32 *)vgpu_opregion(vgpu)->va +
> > > > + INTEL_GVT_OPREGION_PARM) =
> > > parm;
> > > > + break;
> > > > + case INTEL_GVT_HYPERVISOR_KVM:
> > > > + ret = intel_gvt_hypervisor_write_gpa(vgpu, scic_pa,
> > > > + &scic, sizeof(scic));
> > > > + if (ret) {
> > > > + gvt_vgpu_err("guet opregion write error %d, gpa
> > > 0x%llx, len %lu\n",
> > > > + ret, scic_pa, sizeof(scic));
> > > > + return ret;
> > > > + }
> > > > +
> > > > + ret = intel_gvt_hypervisor_write_gpa(vgpu, parm_pa,
> > > > + &parm, sizeof(parm));
> > > > + if (ret) {
> > > > + gvt_vgpu_err("guet opregion write error %d, gpa
> > > 0x%llx, len %lu\n",
> > > > + ret, scic_pa, sizeof(scic));
> > > > + return ret;
> > > > + }
> > > > +
> > > > + break;
> > > > + default:
> > > > + gvt_vgpu_err("not supported hypervisor\n");
> > > > + return -EINVAL;
> > > > }
> > > >
> > > > - *scic = 0;
> > > > - *parm = 0;
> > > > return 0;
> > > > }
> > > > --
> > > > 2.7.4
> > > >
> > >
> > > --
> > > Open Source Technology Center, Intel ltd.
> > >
> > > $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
>
> --
> Open Source Technology Center, Intel ltd.
>
> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
More information about the intel-gvt-dev
mailing list