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

[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;