[v1,2/5] drm/i915/gvt: GVTg read_shared_page implementation

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

Details

Message ID 1541409649-21171-2-git-send-email-xiaolin.zhang@intel.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Intel GVT devel

Not browsing as part of any series.

Commit Message

Xiaolin Zhang Nov. 5, 2018, 9:20 a.m.
GVTg implemented the read_shared_page functionality based on
hypervisor_read_gpa().

the shared_page_gpa was passed from guest driver through PVINFO
shared_page_gpa register.

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/gvt.h      |  4 +++-
 drivers/gpu/drm/i915/gvt/handlers.c |  5 +++++
 drivers/gpu/drm/i915/gvt/vgpu.c     | 14 ++++++++++++++
 3 files changed, 22 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
index 31f6cdb..7562f75 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.h
+++ b/drivers/gpu/drm/i915/gvt/gvt.h
@@ -232,6 +232,7 @@  struct intel_vgpu {
 	struct completion vblank_done;
 
 	u32 scan_nonprivbb;
+	u64 shared_page_gpa;
 };
 
 /* validating GM healthy status*/
@@ -690,7 +691,8 @@  int intel_gvt_debugfs_add_vgpu(struct intel_vgpu *vgpu);
 void intel_gvt_debugfs_remove_vgpu(struct intel_vgpu *vgpu);
 int intel_gvt_debugfs_init(struct intel_gvt *gvt);
 void intel_gvt_debugfs_clean(struct intel_gvt *gvt);
-
+void intel_gvt_read_shared_page(struct intel_vgpu *vgpu,
+		unsigned int offset, void *buf, unsigned long len);
 
 #include "trace.h"
 #include "mpt.h"
diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c
index 32c6687..bf14c66 100644
--- a/drivers/gpu/drm/i915/gvt/handlers.c
+++ b/drivers/gpu/drm/i915/gvt/handlers.c
@@ -1252,6 +1252,11 @@  static int pvinfo_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
 			vgpu_vreg(vgpu, offset) = 0;
 		}
 		break;
+	case _vgtif_reg(shared_page_gpa.lo):
+	case _vgtif_reg(shared_page_gpa.hi):
+		vgpu->shared_page_gpa = vgpu_vreg64_t(vgpu,
+			vgtif_reg(shared_page_gpa));
+		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 d1674db..44507c9 100644
--- a/drivers/gpu/drm/i915/gvt/vgpu.c
+++ b/drivers/gpu/drm/i915/gvt/vgpu.c
@@ -591,3 +591,17 @@  void intel_gvt_reset_vgpu(struct intel_vgpu *vgpu)
 	intel_gvt_reset_vgpu_locked(vgpu, true, 0);
 	mutex_unlock(&vgpu->vgpu_lock);
 }
+
+/**
+ * intel_gvt_read_shared_page - read content from shared page
+ */
+void intel_gvt_read_shared_page(struct intel_vgpu *vgpu,
+		unsigned int offset, void *buf, unsigned long len)
+{
+	int ret = 0;
+	unsigned long gpa = vgpu->shared_page_gpa + offset;
+
+	ret = intel_gvt_hypervisor_read_gpa(vgpu, gpa, buf, len);
+	if (ret)
+		gvt_vgpu_err("read shared page (offset %x) failed", offset);
+}

Comments

On 2018.11.05 17:20:46 +0800, Xiaolin Zhang wrote:
> GVTg implemented the read_shared_page functionality based on
> hypervisor_read_gpa().
> 
> the shared_page_gpa was passed from guest driver through PVINFO
> shared_page_gpa register.
> 
> 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/gvt.h      |  4 +++-
>  drivers/gpu/drm/i915/gvt/handlers.c |  5 +++++
>  drivers/gpu/drm/i915/gvt/vgpu.c     | 14 ++++++++++++++
>  3 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
> index 31f6cdb..7562f75 100644
> --- a/drivers/gpu/drm/i915/gvt/gvt.h
> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> @@ -232,6 +232,7 @@ struct intel_vgpu {
>  	struct completion vblank_done;
>  
>  	u32 scan_nonprivbb;
> +	u64 shared_page_gpa;
>  };
>  
>  /* validating GM healthy status*/
> @@ -690,7 +691,8 @@ int intel_gvt_debugfs_add_vgpu(struct intel_vgpu *vgpu);
>  void intel_gvt_debugfs_remove_vgpu(struct intel_vgpu *vgpu);
>  int intel_gvt_debugfs_init(struct intel_gvt *gvt);
>  void intel_gvt_debugfs_clean(struct intel_gvt *gvt);
> -
> +void intel_gvt_read_shared_page(struct intel_vgpu *vgpu,
> +		unsigned int offset, void *buf, unsigned long len);
>  
>  #include "trace.h"
>  #include "mpt.h"
> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c
> index 32c6687..bf14c66 100644
> --- a/drivers/gpu/drm/i915/gvt/handlers.c
> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> @@ -1252,6 +1252,11 @@ static int pvinfo_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
>  			vgpu_vreg(vgpu, offset) = 0;
>  		}
>  		break;
> +	case _vgtif_reg(shared_page_gpa.lo):
> +	case _vgtif_reg(shared_page_gpa.hi):
> +		vgpu->shared_page_gpa = vgpu_vreg64_t(vgpu,
> +			vgtif_reg(shared_page_gpa));

Looks you don't track if both lo and hi address has been updated properly?

> +		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 d1674db..44507c9 100644
> --- a/drivers/gpu/drm/i915/gvt/vgpu.c
> +++ b/drivers/gpu/drm/i915/gvt/vgpu.c
> @@ -591,3 +591,17 @@ void intel_gvt_reset_vgpu(struct intel_vgpu *vgpu)
>  	intel_gvt_reset_vgpu_locked(vgpu, true, 0);
>  	mutex_unlock(&vgpu->vgpu_lock);
>  }
> +
> +/**
> + * intel_gvt_read_shared_page - read content from shared page
> + */
> +void intel_gvt_read_shared_page(struct intel_vgpu *vgpu,
> +		unsigned int offset, void *buf, unsigned long len)
> +{
> +	int ret = 0;
> +	unsigned long gpa = vgpu->shared_page_gpa + offset;
> +
> +	ret = intel_gvt_hypervisor_read_gpa(vgpu, gpa, buf, len);
> +	if (ret)
> +		gvt_vgpu_err("read shared page (offset %x) failed", offset);
> +}

No error handling to return? And if for invalid gpa, should we take guest
as invalid bad driver?
On 11/05/2018 06:01 PM, Zhenyu Wang wrote:
> On 2018.11.05 17:20:46 +0800, Xiaolin Zhang wrote:
>> GVTg implemented the read_shared_page functionality based on
>> hypervisor_read_gpa().
>>
>> the shared_page_gpa was passed from guest driver through PVINFO
>> shared_page_gpa register.
>>
>> 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/gvt.h      |  4 +++-
>>  drivers/gpu/drm/i915/gvt/handlers.c |  5 +++++
>>  drivers/gpu/drm/i915/gvt/vgpu.c     | 14 ++++++++++++++
>>  3 files changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
>> index 31f6cdb..7562f75 100644
>> --- a/drivers/gpu/drm/i915/gvt/gvt.h
>> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
>> @@ -232,6 +232,7 @@ struct intel_vgpu {
>>  	struct completion vblank_done;
>>  
>>  	u32 scan_nonprivbb;
>> +	u64 shared_page_gpa;
>>  };
>>  
>>  /* validating GM healthy status*/
>> @@ -690,7 +691,8 @@ int intel_gvt_debugfs_add_vgpu(struct intel_vgpu *vgpu);
>>  void intel_gvt_debugfs_remove_vgpu(struct intel_vgpu *vgpu);
>>  int intel_gvt_debugfs_init(struct intel_gvt *gvt);
>>  void intel_gvt_debugfs_clean(struct intel_gvt *gvt);
>> -
>> +void intel_gvt_read_shared_page(struct intel_vgpu *vgpu,
>> +		unsigned int offset, void *buf, unsigned long len);
>>  
>>  #include "trace.h"
>>  #include "mpt.h"
>> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c
>> index 32c6687..bf14c66 100644
>> --- a/drivers/gpu/drm/i915/gvt/handlers.c
>> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
>> @@ -1252,6 +1252,11 @@ static int pvinfo_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
>>  			vgpu_vreg(vgpu, offset) = 0;
>>  		}
>>  		break;
>> +	case _vgtif_reg(shared_page_gpa.lo):
>> +	case _vgtif_reg(shared_page_gpa.hi):
>> +		vgpu->shared_page_gpa = vgpu_vreg64_t(vgpu,
>> +			vgtif_reg(shared_page_gpa));
> Looks you don't track if both lo and hi address has been updated properly?
this register update is handled with the similar mechanism as pdp[4]
register within vgt_if. but in guest, there is a check to make sure
register updated correctly.  the code change as below for your reference.
+        gpa = __pa(dev_priv->vgpu.shared_page);
+        __raw_i915_write32(dev_priv, vgtif_reg(shared_page_gpa.lo),
+                lower_32_bits(gpa));
+        __raw_i915_write32(dev_priv, vgtif_reg(shared_page_gpa.hi),
+                upper_32_bits(gpa));
+        if (gpa != __raw_i915_read64(dev_priv,
+                vgtif_reg(shared_page_gpa))) {
+            DRM_ERROR("vgpu: passed shared_page_gpa failed\n");
+            free_page((unsigned long)dev_priv->vgpu.shared_page);
+            dev_priv->vgpu.pv_caps = 0;
+            return;
+        }

>
>> +		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 d1674db..44507c9 100644
>> --- a/drivers/gpu/drm/i915/gvt/vgpu.c
>> +++ b/drivers/gpu/drm/i915/gvt/vgpu.c
>> @@ -591,3 +591,17 @@ void intel_gvt_reset_vgpu(struct intel_vgpu *vgpu)
>>  	intel_gvt_reset_vgpu_locked(vgpu, true, 0);
>>  	mutex_unlock(&vgpu->vgpu_lock);
>>  }
>> +
>> +/**
>> + * intel_gvt_read_shared_page - read content from shared page
>> + */
>> +void intel_gvt_read_shared_page(struct intel_vgpu *vgpu,
>> +		unsigned int offset, void *buf, unsigned long len)
>> +{
>> +	int ret = 0;
>> +	unsigned long gpa = vgpu->shared_page_gpa + offset;
>> +
>> +	ret = intel_gvt_hypervisor_read_gpa(vgpu, gpa, buf, len);
>> +	if (ret)
>> +		gvt_vgpu_err("read shared page (offset %x) failed", offset);
>> +}
> No error handling to return? And if for invalid gpa, should we take guest
> as invalid bad driver?
>
yes, for invalid gpa, we should take guest as invalid bad driver. will
add offset check in next version to make sure gpa is illegal.
On 2018.11.06 05:33:06 +0000, Zhang, Xiaolin wrote:
> >> @@ -1252,6 +1252,11 @@ static int pvinfo_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
> >>  			vgpu_vreg(vgpu, offset) = 0;
> >>  		}
> >>  		break;
> >> +	case _vgtif_reg(shared_page_gpa.lo):
> >> +	case _vgtif_reg(shared_page_gpa.hi):
> >> +		vgpu->shared_page_gpa = vgpu_vreg64_t(vgpu,
> >> +			vgtif_reg(shared_page_gpa));
> > Looks you don't track if both lo and hi address has been updated properly?
> this register update is handled with the similar mechanism as pdp[4]
> register within vgt_if. but in guest, there is a check to make sure
> register updated correctly.  the code change as below for your reference.

We should always consider arbitrary guest behavior if you can't take
guest as secured, and do error check gracefully.

> +        gpa = __pa(dev_priv->vgpu.shared_page);
> +        __raw_i915_write32(dev_priv, vgtif_reg(shared_page_gpa.lo),
> +                lower_32_bits(gpa));
> +        __raw_i915_write32(dev_priv, vgtif_reg(shared_page_gpa.hi),
> +                upper_32_bits(gpa));
> +        if (gpa != __raw_i915_read64(dev_priv,
> +                vgtif_reg(shared_page_gpa))) {
> +            DRM_ERROR("vgpu: passed shared_page_gpa failed\n");
> +            free_page((unsigned long)dev_priv->vgpu.shared_page);
> +            dev_priv->vgpu.pv_caps = 0;
> +            return;
> +        }
> 
> >
> >> +		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 d1674db..44507c9 100644
> >> --- a/drivers/gpu/drm/i915/gvt/vgpu.c
> >> +++ b/drivers/gpu/drm/i915/gvt/vgpu.c
> >> @@ -591,3 +591,17 @@ void intel_gvt_reset_vgpu(struct intel_vgpu *vgpu)
> >>  	intel_gvt_reset_vgpu_locked(vgpu, true, 0);
> >>  	mutex_unlock(&vgpu->vgpu_lock);
> >>  }
> >> +
> >> +/**
> >> + * intel_gvt_read_shared_page - read content from shared page
> >> + */
> >> +void intel_gvt_read_shared_page(struct intel_vgpu *vgpu,
> >> +		unsigned int offset, void *buf, unsigned long len)
> >> +{
> >> +	int ret = 0;
> >> +	unsigned long gpa = vgpu->shared_page_gpa + offset;
> >> +
> >> +	ret = intel_gvt_hypervisor_read_gpa(vgpu, gpa, buf, len);
> >> +	if (ret)
> >> +		gvt_vgpu_err("read shared page (offset %x) failed", offset);
> >> +}
> > No error handling to return? And if for invalid gpa, should we take guest
> > as invalid bad driver?
> >
> yes, for invalid gpa, we should take guest as invalid bad driver. will
> add offset check in next version to make sure gpa is illegal.
>