[v1,1/5] drm/i915/gvt: GVTg handle enable_pvmmio PVINFO register

Submitted by Zhang, Xiaolin on Nov. 5, 2018, 9:20 a.m.

Details

Message ID 1541409649-21171-1-git-send-email-xiaolin.zhang@intel.com
State New
Series "Series without cover letter"
Headers show

Commit Message

Zhang, Xiaolin Nov. 5, 2018, 9:20 a.m.
implement enable_pvmmio PVINFO register handler in GVTg to
control different level pvmmio optimization within guest.

report VGT_CAPS_PVMMIO capability in pvinfo page for guest.

v1: rebase
v0: RFC

Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
Cc: Zhi Wang <zhi.a.wang@intel.com>
Cc: Min He<min.he@intel.com>
Cc: Fei Jiang <fei.jiang@intel.com>
Cc: Zhipeng Gong <zhipeng.gong@intel.com>
Cc: Hang Yuan <hang.yuan@intel.com>
Cc: Zhiyuan Lv <zhiyuan.lv@intel.com>
Signed-off-by: Xiaolin Zhang <xiaolin.zhang@intel.com>
---
 drivers/gpu/drm/i915/gvt/handlers.c | 10 ++++++++++
 drivers/gpu/drm/i915/gvt/vgpu.c     |  7 +++++++
 2 files changed, 17 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c
index b5475c9..32c6687 100644
--- a/drivers/gpu/drm/i915/gvt/handlers.c
+++ b/drivers/gpu/drm/i915/gvt/handlers.c
@@ -1242,6 +1242,16 @@  static int pvinfo_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
 	case _vgtif_reg(g2v_notify):
 		ret = handle_g2v_notification(vgpu, data);
 		break;
+	case _vgtif_reg(enable_pvmmio):
+		if (vgpu->gvt->dev_priv->vgpu.pv_caps) {
+			vgpu_vreg(vgpu, offset) = data &
+				vgpu->gvt->dev_priv->vgpu.pv_caps;
+			DRM_INFO("vgpu id=%d pvmmio=0x%x\n",
+				vgpu->id, VGPU_PVMMIO(vgpu));
+		} else {
+			vgpu_vreg(vgpu, offset) = 0;
+		}
+		break;
 	/* add xhot and yhot to handled list to avoid error log */
 	case _vgtif_reg(cursor_x_hot):
 	case _vgtif_reg(cursor_y_hot):
diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
index c628be0..d1674db 100644
--- a/drivers/gpu/drm/i915/gvt/vgpu.c
+++ b/drivers/gpu/drm/i915/gvt/vgpu.c
@@ -47,6 +47,7 @@  void populate_pvinfo_page(struct intel_vgpu *vgpu)
 	vgpu_vreg_t(vgpu, vgtif_reg(vgt_caps)) = VGT_CAPS_FULL_48BIT_PPGTT;
 	vgpu_vreg_t(vgpu, vgtif_reg(vgt_caps)) |= VGT_CAPS_HWSP_EMULATION;
 	vgpu_vreg_t(vgpu, vgtif_reg(vgt_caps)) |= VGT_CAPS_HUGE_GTT;
+	vgpu_vreg_t(vgpu, vgtif_reg(vgt_caps)) |= VGT_CAPS_PVMMIO;
 
 	vgpu_vreg_t(vgpu, vgtif_reg(avail_rs.mappable_gmadr.base)) =
 		vgpu_aperture_gmadr_base(vgpu);
@@ -62,6 +63,8 @@  void populate_pvinfo_page(struct intel_vgpu *vgpu)
 	vgpu_vreg_t(vgpu, vgtif_reg(cursor_x_hot)) = UINT_MAX;
 	vgpu_vreg_t(vgpu, vgtif_reg(cursor_y_hot)) = UINT_MAX;
 
+	vgpu_vreg_t(vgpu, vgtif_reg(enable_pvmmio)) = 0;
+
 	gvt_dbg_core("Populate PVINFO PAGE for vGPU %d\n", vgpu->id);
 	gvt_dbg_core("aperture base [GMADR] 0x%llx size 0x%llx\n",
 		vgpu_aperture_gmadr_base(vgpu), vgpu_aperture_sz(vgpu));
@@ -491,6 +494,8 @@  struct intel_vgpu *intel_gvt_create_vgpu(struct intel_gvt *gvt,
 	return vgpu;
 }
 
+#define _vgtif_reg(x) \
+	(VGT_PVINFO_PAGE + offsetof(struct vgt_if, x))
 /**
  * intel_gvt_reset_vgpu_locked - reset a virtual GPU by DMLR or GT reset
  * @vgpu: virtual GPU
@@ -525,6 +530,7 @@  void intel_gvt_reset_vgpu_locked(struct intel_vgpu *vgpu, bool dmlr,
 	struct intel_gvt *gvt = vgpu->gvt;
 	struct intel_gvt_workload_scheduler *scheduler = &gvt->scheduler;
 	unsigned int resetting_eng = dmlr ? ALL_ENGINES : engine_mask;
+	int enable_pvmmio = vgpu_vreg(vgpu, _vgtif_reg(enable_pvmmio));
 
 	gvt_dbg_core("------------------------------------------\n");
 	gvt_dbg_core("resseting vgpu%d, dmlr %d, engine_mask %08x\n",
@@ -556,6 +562,7 @@  void intel_gvt_reset_vgpu_locked(struct intel_vgpu *vgpu, bool dmlr,
 
 		intel_vgpu_reset_mmio(vgpu, dmlr);
 		populate_pvinfo_page(vgpu);
+		vgpu_vreg(vgpu, _vgtif_reg(enable_pvmmio)) = enable_pvmmio;
 		intel_vgpu_reset_display(vgpu);
 
 		if (dmlr) {

Comments

Zhenyu Wang Nov. 5, 2018, 9:50 a.m.
On 2018.11.05 17:20:45 +0800, Xiaolin Zhang wrote:
> implement enable_pvmmio PVINFO register handler in GVTg to
> control different level pvmmio optimization within guest.
> 
> report VGT_CAPS_PVMMIO capability in pvinfo page for guest.
> 
> v1: rebase
> v0: RFC
> 
> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> Cc: Zhi Wang <zhi.a.wang@intel.com>
> Cc: Min He<min.he@intel.com>
> Cc: Fei Jiang <fei.jiang@intel.com>
> Cc: Zhipeng Gong <zhipeng.gong@intel.com>
> Cc: Hang Yuan <hang.yuan@intel.com>
> Cc: Zhiyuan Lv <zhiyuan.lv@intel.com>
> Signed-off-by: Xiaolin Zhang <xiaolin.zhang@intel.com>
> ---
>  drivers/gpu/drm/i915/gvt/handlers.c | 10 ++++++++++
>  drivers/gpu/drm/i915/gvt/vgpu.c     |  7 +++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c
> index b5475c9..32c6687 100644
> --- a/drivers/gpu/drm/i915/gvt/handlers.c
> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> @@ -1242,6 +1242,16 @@ static int pvinfo_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
>  	case _vgtif_reg(g2v_notify):
>  		ret = handle_g2v_notification(vgpu, data);
>  		break;
> +	case _vgtif_reg(enable_pvmmio):
> +		if (vgpu->gvt->dev_priv->vgpu.pv_caps) {

I can't find where this is defined..And why this would reference host i915's definition?
Shouldn't this be just vgpu pv caps?

> +			vgpu_vreg(vgpu, offset) = data &
> +				vgpu->gvt->dev_priv->vgpu.pv_caps;
> +			DRM_INFO("vgpu id=%d pvmmio=0x%x\n",
> +				vgpu->id, VGPU_PVMMIO(vgpu));
> +		} else {
> +			vgpu_vreg(vgpu, offset) = 0;
> +		}
> +		break;
>  	/* add xhot and yhot to handled list to avoid error log */
>  	case _vgtif_reg(cursor_x_hot):
>  	case _vgtif_reg(cursor_y_hot):
> diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
> index c628be0..d1674db 100644
> --- a/drivers/gpu/drm/i915/gvt/vgpu.c
> +++ b/drivers/gpu/drm/i915/gvt/vgpu.c
> @@ -47,6 +47,7 @@ void populate_pvinfo_page(struct intel_vgpu *vgpu)
>  	vgpu_vreg_t(vgpu, vgtif_reg(vgt_caps)) = VGT_CAPS_FULL_48BIT_PPGTT;
>  	vgpu_vreg_t(vgpu, vgtif_reg(vgt_caps)) |= VGT_CAPS_HWSP_EMULATION;
>  	vgpu_vreg_t(vgpu, vgtif_reg(vgt_caps)) |= VGT_CAPS_HUGE_GTT;
> +	vgpu_vreg_t(vgpu, vgtif_reg(vgt_caps)) |= VGT_CAPS_PVMMIO;

This should be the last one when you enabled all caps.

>  
>  	vgpu_vreg_t(vgpu, vgtif_reg(avail_rs.mappable_gmadr.base)) =
>  		vgpu_aperture_gmadr_base(vgpu);
> @@ -62,6 +63,8 @@ void populate_pvinfo_page(struct intel_vgpu *vgpu)
>  	vgpu_vreg_t(vgpu, vgtif_reg(cursor_x_hot)) = UINT_MAX;
>  	vgpu_vreg_t(vgpu, vgtif_reg(cursor_y_hot)) = UINT_MAX;
>  
> +	vgpu_vreg_t(vgpu, vgtif_reg(enable_pvmmio)) = 0;

Could we rename this as 'pvmmio_caps' instead of enable_xxx which seems
like an interface to turn it on/off?

> +
>  	gvt_dbg_core("Populate PVINFO PAGE for vGPU %d\n", vgpu->id);
>  	gvt_dbg_core("aperture base [GMADR] 0x%llx size 0x%llx\n",
>  		vgpu_aperture_gmadr_base(vgpu), vgpu_aperture_sz(vgpu));
> @@ -491,6 +494,8 @@ struct intel_vgpu *intel_gvt_create_vgpu(struct intel_gvt *gvt,
>  	return vgpu;
>  }
>  
> +#define _vgtif_reg(x) \
> +	(VGT_PVINFO_PAGE + offsetof(struct vgt_if, x))

If you need this, put it in some header.

>  /**
>   * intel_gvt_reset_vgpu_locked - reset a virtual GPU by DMLR or GT reset
>   * @vgpu: virtual GPU
> @@ -525,6 +530,7 @@ void intel_gvt_reset_vgpu_locked(struct intel_vgpu *vgpu, bool dmlr,
>  	struct intel_gvt *gvt = vgpu->gvt;
>  	struct intel_gvt_workload_scheduler *scheduler = &gvt->scheduler;
>  	unsigned int resetting_eng = dmlr ? ALL_ENGINES : engine_mask;
> +	int enable_pvmmio = vgpu_vreg(vgpu, _vgtif_reg(enable_pvmmio));
>  
>  	gvt_dbg_core("------------------------------------------\n");
>  	gvt_dbg_core("resseting vgpu%d, dmlr %d, engine_mask %08x\n",
> @@ -556,6 +562,7 @@ void intel_gvt_reset_vgpu_locked(struct intel_vgpu *vgpu, bool dmlr,
>  
>  		intel_vgpu_reset_mmio(vgpu, dmlr);
>  		populate_pvinfo_page(vgpu);
> +		vgpu_vreg(vgpu, _vgtif_reg(enable_pvmmio)) = enable_pvmmio;
>  		intel_vgpu_reset_display(vgpu);
>  
>  		if (dmlr) {
> -- 
> 2.7.4
>
Zhang, Xiaolin Nov. 6, 2018, 5:20 a.m.
On 11/05/2018 05:59 PM, Zhenyu Wang wrote:
> On 2018.11.05 17:20:45 +0800, Xiaolin Zhang wrote:
>> implement enable_pvmmio PVINFO register handler in GVTg to
>> control different level pvmmio optimization within guest.
>>
>> report VGT_CAPS_PVMMIO capability in pvinfo page for guest.
>>
>> v1: rebase
>> v0: RFC
>>
>> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
>> Cc: Zhi Wang <zhi.a.wang@intel.com>
>> Cc: Min He<min.he@intel.com>
>> Cc: Fei Jiang <fei.jiang@intel.com>
>> Cc: Zhipeng Gong <zhipeng.gong@intel.com>
>> Cc: Hang Yuan <hang.yuan@intel.com>
>> Cc: Zhiyuan Lv <zhiyuan.lv@intel.com>
>> Signed-off-by: Xiaolin Zhang <xiaolin.zhang@intel.com>
>> ---
>>  drivers/gpu/drm/i915/gvt/handlers.c | 10 ++++++++++
>>  drivers/gpu/drm/i915/gvt/vgpu.c     |  7 +++++++
>>  2 files changed, 17 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c
>> index b5475c9..32c6687 100644
>> --- a/drivers/gpu/drm/i915/gvt/handlers.c
>> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
>> @@ -1242,6 +1242,16 @@ static int pvinfo_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
>>  	case _vgtif_reg(g2v_notify):
>>  		ret = handle_g2v_notification(vgpu, data);
>>  		break;
>> +	case _vgtif_reg(enable_pvmmio):
>> +		if (vgpu->gvt->dev_priv->vgpu.pv_caps) {
> I can't find where this is defined..And why this would reference host i915's definition?
> Shouldn't this be just vgpu pv caps?
below is the code code in i915_drv.h.  pv caps is a part of vgpu
structure as below.
@@ -1348,6 +1349,7 @@ struct i915_workarounds {
 struct i915_virtual_gpu {
     bool active;
     u32 caps;
+    u32 pv_caps;
 };
 
>
>> +			vgpu_vreg(vgpu, offset) = data &
>> +				vgpu->gvt->dev_priv->vgpu.pv_caps;
>> +			DRM_INFO("vgpu id=%d pvmmio=0x%x\n",
>> +				vgpu->id, VGPU_PVMMIO(vgpu));
>> +		} else {
>> +			vgpu_vreg(vgpu, offset) = 0;
>> +		}
>> +		break;
>>  	/* add xhot and yhot to handled list to avoid error log */
>>  	case _vgtif_reg(cursor_x_hot):
>>  	case _vgtif_reg(cursor_y_hot):
>> diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
>> index c628be0..d1674db 100644
>> --- a/drivers/gpu/drm/i915/gvt/vgpu.c
>> +++ b/drivers/gpu/drm/i915/gvt/vgpu.c
>> @@ -47,6 +47,7 @@ void populate_pvinfo_page(struct intel_vgpu *vgpu)
>>  	vgpu_vreg_t(vgpu, vgtif_reg(vgt_caps)) = VGT_CAPS_FULL_48BIT_PPGTT;
>>  	vgpu_vreg_t(vgpu, vgtif_reg(vgt_caps)) |= VGT_CAPS_HWSP_EMULATION;
>>  	vgpu_vreg_t(vgpu, vgtif_reg(vgt_caps)) |= VGT_CAPS_HUGE_GTT;
>> +	vgpu_vreg_t(vgpu, vgtif_reg(vgt_caps)) |= VGT_CAPS_PVMMIO;
> This should be the last one when you enabled all caps.
do you mean this line code change should be splitted out for a single patch?
>
>>  
>>  	vgpu_vreg_t(vgpu, vgtif_reg(avail_rs.mappable_gmadr.base)) =
>>  		vgpu_aperture_gmadr_base(vgpu);
>> @@ -62,6 +63,8 @@ void populate_pvinfo_page(struct intel_vgpu *vgpu)
>>  	vgpu_vreg_t(vgpu, vgtif_reg(cursor_x_hot)) = UINT_MAX;
>>  	vgpu_vreg_t(vgpu, vgtif_reg(cursor_y_hot)) = UINT_MAX;
>>  
>> +	vgpu_vreg_t(vgpu, vgtif_reg(enable_pvmmio)) = 0;
> Could we rename this as 'pvmmio_caps' instead of enable_xxx which seems
> like an interface to turn it on/off?
no problem, rename is OK.
>
>> +
>>  	gvt_dbg_core("Populate PVINFO PAGE for vGPU %d\n", vgpu->id);
>>  	gvt_dbg_core("aperture base [GMADR] 0x%llx size 0x%llx\n",
>>  		vgpu_aperture_gmadr_base(vgpu), vgpu_aperture_sz(vgpu));
>> @@ -491,6 +494,8 @@ struct intel_vgpu *intel_gvt_create_vgpu(struct intel_gvt *gvt,
>>  	return vgpu;
>>  }
>>  
>> +#define _vgtif_reg(x) \
>> +	(VGT_PVINFO_PAGE + offsetof(struct vgt_if, x))
> If you need this, put it in some header.
no problem.
>
>>  /**
>>   * intel_gvt_reset_vgpu_locked - reset a virtual GPU by DMLR or GT reset
>>   * @vgpu: virtual GPU
>> @@ -525,6 +530,7 @@ void intel_gvt_reset_vgpu_locked(struct intel_vgpu *vgpu, bool dmlr,
>>  	struct intel_gvt *gvt = vgpu->gvt;
>>  	struct intel_gvt_workload_scheduler *scheduler = &gvt->scheduler;
>>  	unsigned int resetting_eng = dmlr ? ALL_ENGINES : engine_mask;
>> +	int enable_pvmmio = vgpu_vreg(vgpu, _vgtif_reg(enable_pvmmio));
>>  
>>  	gvt_dbg_core("------------------------------------------\n");
>>  	gvt_dbg_core("resseting vgpu%d, dmlr %d, engine_mask %08x\n",
>> @@ -556,6 +562,7 @@ void intel_gvt_reset_vgpu_locked(struct intel_vgpu *vgpu, bool dmlr,
>>  
>>  		intel_vgpu_reset_mmio(vgpu, dmlr);
>>  		populate_pvinfo_page(vgpu);
>> +		vgpu_vreg(vgpu, _vgtif_reg(enable_pvmmio)) = enable_pvmmio;
>>  		intel_vgpu_reset_display(vgpu);
>>  
>>  		if (dmlr) {
>> -- 
>> 2.7.4
>>
Zhenyu Wang Nov. 6, 2018, 6:06 a.m.
On 2018.11.06 05:20:19 +0000, Zhang, Xiaolin wrote:
> On 11/05/2018 05:59 PM, Zhenyu Wang wrote:
> > On 2018.11.05 17:20:45 +0800, Xiaolin Zhang wrote:
> >> implement enable_pvmmio PVINFO register handler in GVTg to
> >> control different level pvmmio optimization within guest.
> >>
> >> report VGT_CAPS_PVMMIO capability in pvinfo page for guest.
> >>
> >> v1: rebase
> >> v0: RFC
> >>
> >> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> >> Cc: Zhi Wang <zhi.a.wang@intel.com>
> >> Cc: Min He<min.he@intel.com>
> >> Cc: Fei Jiang <fei.jiang@intel.com>
> >> Cc: Zhipeng Gong <zhipeng.gong@intel.com>
> >> Cc: Hang Yuan <hang.yuan@intel.com>
> >> Cc: Zhiyuan Lv <zhiyuan.lv@intel.com>
> >> Signed-off-by: Xiaolin Zhang <xiaolin.zhang@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/gvt/handlers.c | 10 ++++++++++
> >>  drivers/gpu/drm/i915/gvt/vgpu.c     |  7 +++++++
> >>  2 files changed, 17 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c
> >> index b5475c9..32c6687 100644
> >> --- a/drivers/gpu/drm/i915/gvt/handlers.c
> >> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> >> @@ -1242,6 +1242,16 @@ static int pvinfo_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
> >>  	case _vgtif_reg(g2v_notify):
> >>  		ret = handle_g2v_notification(vgpu, data);
> >>  		break;
> >> +	case _vgtif_reg(enable_pvmmio):
> >> +		if (vgpu->gvt->dev_priv->vgpu.pv_caps) {
> > I can't find where this is defined..And why this would reference host i915's definition?
> > Shouldn't this be just vgpu pv caps?
> below is the code code in i915_drv.h.  pv caps is a part of vgpu
> structure as below.
> @@ -1348,6 +1349,7 @@ struct i915_workarounds {
>  struct i915_virtual_gpu {
>      bool active;
>      u32 caps;
> +    u32 pv_caps;
>  };

struct i915_virtual_gpu is for guest state, but here you want
to check gvt host supported pvmmio caps, which seems weird. Don't
mix host state vs. guest state.

>  
> >
> >> +			vgpu_vreg(vgpu, offset) = data &
> >> +				vgpu->gvt->dev_priv->vgpu.pv_caps;
> >> +			DRM_INFO("vgpu id=%d pvmmio=0x%x\n",
> >> +				vgpu->id, VGPU_PVMMIO(vgpu));
> >> +		} else {
> >> +			vgpu_vreg(vgpu, offset) = 0;
> >> +		}
> >> +		break;
> >>  	/* add xhot and yhot to handled list to avoid error log */
> >>  	case _vgtif_reg(cursor_x_hot):
> >>  	case _vgtif_reg(cursor_y_hot):
> >> diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
> >> index c628be0..d1674db 100644
> >> --- a/drivers/gpu/drm/i915/gvt/vgpu.c
> >> +++ b/drivers/gpu/drm/i915/gvt/vgpu.c
> >> @@ -47,6 +47,7 @@ void populate_pvinfo_page(struct intel_vgpu *vgpu)
> >>  	vgpu_vreg_t(vgpu, vgtif_reg(vgt_caps)) = VGT_CAPS_FULL_48BIT_PPGTT;
> >>  	vgpu_vreg_t(vgpu, vgtif_reg(vgt_caps)) |= VGT_CAPS_HWSP_EMULATION;
> >>  	vgpu_vreg_t(vgpu, vgtif_reg(vgt_caps)) |= VGT_CAPS_HUGE_GTT;
> >> +	vgpu_vreg_t(vgpu, vgtif_reg(vgt_caps)) |= VGT_CAPS_PVMMIO;
> > This should be the last one when you enabled all caps.
> do you mean this line code change should be splitted out for a single patch?
> >
> >>  
> >>  	vgpu_vreg_t(vgpu, vgtif_reg(avail_rs.mappable_gmadr.base)) =
> >>  		vgpu_aperture_gmadr_base(vgpu);
> >> @@ -62,6 +63,8 @@ void populate_pvinfo_page(struct intel_vgpu *vgpu)
> >>  	vgpu_vreg_t(vgpu, vgtif_reg(cursor_x_hot)) = UINT_MAX;
> >>  	vgpu_vreg_t(vgpu, vgtif_reg(cursor_y_hot)) = UINT_MAX;
> >>  
> >> +	vgpu_vreg_t(vgpu, vgtif_reg(enable_pvmmio)) = 0;
> > Could we rename this as 'pvmmio_caps' instead of enable_xxx which seems
> > like an interface to turn it on/off?
> no problem, rename is OK.
> >
> >> +
> >>  	gvt_dbg_core("Populate PVINFO PAGE for vGPU %d\n", vgpu->id);
> >>  	gvt_dbg_core("aperture base [GMADR] 0x%llx size 0x%llx\n",
> >>  		vgpu_aperture_gmadr_base(vgpu), vgpu_aperture_sz(vgpu));
> >> @@ -491,6 +494,8 @@ struct intel_vgpu *intel_gvt_create_vgpu(struct intel_gvt *gvt,
> >>  	return vgpu;
> >>  }
> >>  
> >> +#define _vgtif_reg(x) \
> >> +	(VGT_PVINFO_PAGE + offsetof(struct vgt_if, x))
> > If you need this, put it in some header.
> no problem.
> >
> >>  /**
> >>   * intel_gvt_reset_vgpu_locked - reset a virtual GPU by DMLR or GT reset
> >>   * @vgpu: virtual GPU
> >> @@ -525,6 +530,7 @@ void intel_gvt_reset_vgpu_locked(struct intel_vgpu *vgpu, bool dmlr,
> >>  	struct intel_gvt *gvt = vgpu->gvt;
> >>  	struct intel_gvt_workload_scheduler *scheduler = &gvt->scheduler;
> >>  	unsigned int resetting_eng = dmlr ? ALL_ENGINES : engine_mask;
> >> +	int enable_pvmmio = vgpu_vreg(vgpu, _vgtif_reg(enable_pvmmio));
> >>  
> >>  	gvt_dbg_core("------------------------------------------\n");
> >>  	gvt_dbg_core("resseting vgpu%d, dmlr %d, engine_mask %08x\n",
> >> @@ -556,6 +562,7 @@ void intel_gvt_reset_vgpu_locked(struct intel_vgpu *vgpu, bool dmlr,
> >>  
> >>  		intel_vgpu_reset_mmio(vgpu, dmlr);
> >>  		populate_pvinfo_page(vgpu);
> >> +		vgpu_vreg(vgpu, _vgtif_reg(enable_pvmmio)) = enable_pvmmio;
> >>  		intel_vgpu_reset_display(vgpu);
> >>  
> >>  		if (dmlr) {
> >> -- 
> >> 2.7.4
> >>
>
gvt@intel.com Nov. 8, 2018, 3:10 a.m.

Zhenyu Wang Nov. 9, 2018, 2:39 a.m.
On 2018.11.05 17:20:45 +0800, Xiaolin Zhang wrote:
> implement enable_pvmmio PVINFO register handler in GVTg to
> control different level pvmmio optimization within guest.
> 
> report VGT_CAPS_PVMMIO capability in pvinfo page for guest.
> 

Another thing that I think is not consistent for these pv interface
which is the guest to host notification is not aligned, some are none
e.g share page setup, master irq, some are implicitly through specific
MMIO write, e.g execlist port. Which seems not good to me. We should
have a formal notification definition for all of them e.g through
current 'g2v' notification interface, looks ppgtt update uses that. So
host would handle pv request when receiving notification consistently,
instead of having different ways which is not good to track.

Thanks
Zhang, Xiaolin Nov. 12, 2018, 1 a.m.
On 11/09/2018 10:48 AM, Zhenyu Wang wrote:
> On 2018.11.05 17:20:45 +0800, Xiaolin Zhang wrote:
>> implement enable_pvmmio PVINFO register handler in GVTg to
>> control different level pvmmio optimization within guest.
>>
>> report VGT_CAPS_PVMMIO capability in pvinfo page for guest.
>>
> Another thing that I think is not consistent for these pv interface
> which is the guest to host notification is not aligned, some are none
> e.g share page setup, master irq, some are implicitly through specific
> MMIO write, e.g execlist port. Which seems not good to me. We should
> have a formal notification definition for all of them e.g through
> current 'g2v' notification interface, looks ppgtt update uses that. So
> host would handle pv request when receiving notification consistently,
> instead of having different ways which is not good to track.
>
> Thanks

Thanks your point. the host to handle pv request is the same way just
with different names. Shared page setup is the foundation of PV features
and only setup once. so shared page setup must be setup first and for
this trap notification, we have created new shared_page_gpa register for
this purpose which should be notified  from guest. for master irq
feature, we crated one more register "check_pending_irq" for guest
notification to trap from guest to host; for workload submission
(execlist), we reuse the submit register to trap from guest to host
instead of creating new one to reduce trap cost (otherwise, we have 2
traps here , one for submit register and one for notification). for
ppgtt, since the existing g2v notification can't cover our requirement,
so we extended 3 more in G2V PPTT notification definition.  Maybe these
names (shared_page_gpa, check_pedning_irq, submit_reg) are not uniform,
but they do the same thing for g2v notification.

BRs. Xiaolin
Zhenyu Wang Nov. 12, 2018, 2:23 a.m.
On 2018.11.12 01:00:04 +0000, Zhang, Xiaolin wrote:
> On 11/09/2018 10:48 AM, Zhenyu Wang wrote:
> > On 2018.11.05 17:20:45 +0800, Xiaolin Zhang wrote:
> >> implement enable_pvmmio PVINFO register handler in GVTg to
> >> control different level pvmmio optimization within guest.
> >>
> >> report VGT_CAPS_PVMMIO capability in pvinfo page for guest.
> >>
> > Another thing that I think is not consistent for these pv interface
> > which is the guest to host notification is not aligned, some are none
> > e.g share page setup, master irq, some are implicitly through specific
> > MMIO write, e.g execlist port. Which seems not good to me. We should
> > have a formal notification definition for all of them e.g through
> > current 'g2v' notification interface, looks ppgtt update uses that. So
> > host would handle pv request when receiving notification consistently,
> > instead of having different ways which is not good to track.
> >
> > Thanks
> 
> Thanks your point. the host to handle pv request is the same way just
> with different names. Shared page setup is the foundation of PV features
> and only setup once. so shared page setup must be setup first and for
> this trap notification, we have created new shared_page_gpa register for
> this purpose which should be notified  from guest. for master irq
> feature, we crated one more register "check_pending_irq" for guest
> notification to trap from guest to host; for workload submission
> (execlist), we reuse the submit register to trap from guest to host
> instead of creating new one to reduce trap cost (otherwise, we have 2
> traps here , one for submit register and one for notification). for
> ppgtt, since the existing g2v notification can't cover our requirement,
> so we extended 3 more in G2V PPTT notification definition.  Maybe these
> names (shared_page_gpa, check_pedning_irq, submit_reg) are not uniform,
> but they do the same thing for g2v notification.
> 

That's exactly my point that they are not uniformed. Guest driver should
write any update in shared page, then notify gvt host to handle, e.g check_pending_irq,
submit_execlist should all be gvt notification. Then you would have consistent
state, e.g if guest and host has reached agreement on PV action, they would
follow that for sure, any other action would be taken as illegal, e.g write
some MMIO instead, etc.