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

[Zhang, Xiaolin]

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 at linux.intel.com>
>> Cc: Zhi Wang <zhi.a.wang at intel.com>
>> Cc: Min He<min.he at intel.com>
>> Cc: Fei Jiang <fei.jiang at intel.com>
>> Cc: Zhipeng Gong <zhipeng.gong at intel.com>
>> Cc: Hang Yuan <hang.yuan at intel.com>
>> Cc: Zhiyuan Lv <zhiyuan.lv at intel.com>
>> Signed-off-by: Xiaolin Zhang <xiaolin.zhang at 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.