[PATCH] drm/i915/gvt: introduced failsafe mode into vgpu

[He, Min]

Ok. will modify the patch and implement a new logic.

在 2/15/2017 4:31 PM, Tian, Kevin 写道:
>> From: He, Min
>> Sent: Wednesday, February 15, 2017 4:27 PM
>>
>> 在 2/15/2017 3:47 PM, Tian, Kevin 写道:
>>>> From: Min He
>>>> Sent: Wednesday, February 15, 2017 3:29 PM
>>>>
>>>> New failsafe mode is introduced, when we detect guest not supporting
>>>> GVTg.
>>>> In failsafe mode, we will ignore all the MMIO and cfg space read/write
>>>> from guest.
>>> instead of ignoring, is it better to make failsafe as a simple
>>> memory read/write emulation logic? Although we cannot
>>> guarantee guest behavior in such situation, returning a
>>> meaningful content (as previous programmed) looks more
>>> reasonable to cause less abnormal guest behaviors than
>>> returning all 1s.
>> Not tried other logic. Handling mmio registers is simple, but emulating
>> GTT entries may
>> need to add more logic. To simplify the design, I used current logic.
> I don't understand. We have vreg, vgtt, etc. You simply emulate
> read/write into virtual state w/o invoking any handler...
>
>>>> Signed-off-by: Min He <min.he at intel.com>
>>>> Signed-off-by: Pei Zhang <pei.zhang at intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/gvt/cfg_space.c |  2 ++
>>>>    drivers/gpu/drm/i915/gvt/gvt.h       |  2 ++
>>>>    drivers/gpu/drm/i915/gvt/handlers.c  | 26
>> ++++++++++++++++++++++++++
>>>>    drivers/gpu/drm/i915/gvt/mmio.c      |  8 ++++++++
>>>>    drivers/gpu/drm/i915/gvt/vgpu.c      |  8 +++++++-
>>>>    5 files changed, 45 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gvt/cfg_space.c
>>>> b/drivers/gpu/drm/i915/gvt/cfg_space.c
>>>> index 4a6a2ed..d74410b 100644
>>>> --- a/drivers/gpu/drm/i915/gvt/cfg_space.c
>>>> +++ b/drivers/gpu/drm/i915/gvt/cfg_space.c
>>>> @@ -236,6 +236,8 @@ int intel_vgpu_emulate_cfg_write(struct intel_vgpu *vgpu,
>>>> unsigned int offset,
>>>>    	void *p_data, unsigned int bytes)
>>>>    {
>>>>    	int ret;
>>>> +	if (vgpu->failsafe)
>>>> +		return 0;
>>>>
>>>>    	if (WARN_ON(bytes > 4))
>>>>    		return -EINVAL;
>>>> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
>>>> index e227caf..93bd157 100644
>>>> --- a/drivers/gpu/drm/i915/gvt/gvt.h
>>>> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
>>>> @@ -143,6 +143,8 @@ struct intel_vgpu {
>>>>    	int id;
>>>>    	unsigned long handle; /* vGPU handle used by hypervisor MPT modules */
>>>>    	bool active;
>>>> +	bool pv_notified;
>>>> +	bool failsafe;
>>>>    	bool resetting;
>>>>    	void *sched_data;
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c
>> b/drivers/gpu/drm/i915/gvt/handlers.c
>>>> index 1d45062..6089a1e 100644
>>>> --- a/drivers/gpu/drm/i915/gvt/handlers.c
>>>> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
>>>> @@ -154,6 +154,17 @@ static int sanitize_fence_mmio_access(struct intel_vgpu
>> *vgpu,
>>>>    		unsigned int fence_num, void *p_data, unsigned int bytes)
>>>>    {
>>>>    	if (fence_num >= vgpu_fence_sz(vgpu)) {
>>>> +		if (!vgpu->pv_notified) {
>>>> +			/* When guest access oob fence regs without access
>>>> +			 * pv_info first, we treat guest not supporting GVT,
>>>> +			 * and we will let vgpu enter failsafe mode.
>>>> +			 */
>>>> +			vgpu->failsafe = true;
>>>> +			pr_err("***Detected guest not supporting GVT-g***\n");
>>>> +			pr_err("***Please update your guest driver*******\n");
>>>> +			pr_err("***Entering failsafe mode****************\n");
>>>> +			return -EINVAL;
>>>> +		}
>>> move above logic into a function to avoid duplication in multiple places.
>> Same as the answer to Changbin. We may have different entries to
>> failsafe mode, so the error log is different.  It's hard to encapsule
>> these logs into one function.
>>
> isn't below generic?
>
>>>> +			vgpu->failsafe = true;
>>>> +			pr_err("***Detected guest not supporting GVT-g***\n");
>>>> +			pr_err("***Please update your guest driver*******\n");
>>>> +			pr_err("***Entering failsafe mode****************\n");
>>>> +			return -EINVAL;