[Intel-gfx] [PATCH 6/7] drm/i915: guest i915 notification for Intel-GVTg

[Zhiyuan Lv]

On Thu, Aug 20, 2015 at 04:11:57PM +0300, Joonas Lahtinen wrote:
> Hi,
> 
> Notes below.
> 
> On to, 2015-08-20 at 15:45 +0800, Zhiyuan Lv wrote:
> > When i915 drivers run inside a VM with Intel-GVTg, some explicit
> > notifications are needed from guest to host device model through PV
> > INFO page write. The notifications include:
> > 
> > 	PPGTT create/destroy
> > 	EXECLIST create/destroy
> > 
> > They are used for the shadow implementation of PPGTT and EXECLIST
> > context. Intel GVT-g needs to write-protect the guest pages of PPGTT
> > and contexts, and clear the write protection when they end their life
> > cycle.
> > 
> > Signed-off-by: Zhiyuan Lv <zhiyuan.lv at intel.com>
> > Signed-off-by: Zhi Wang <zhi.a.wang at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 41 
> > +++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_lrc.c    | 25 ++++++++++++++++++++++
> >  2 files changed, 66 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
> > b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 823005c..00dafb0 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -896,6 +896,41 @@ static int gen8_init_scratch(struct 
> > i915_address_space *vm)
> >  	return 0;
> >  }
> >  
> > +static int gen8_ppgtt_notify_vgt(struct i915_hw_ppgtt *ppgtt, bool 
> > create)
> > +{
> > +	enum vgt_g2v_type msg;
> > +	struct drm_device *dev = ppgtt->base.dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	unsigned int offset = vgtif_reg(pdp0_lo);
> > +	int i;
> > +
> > +	if (USES_FULL_48BIT_PPGTT(dev)) {
> 
> With regards the patch "preallocate pdps for 32 bit vgpu", is this code
> path ever taken?

48-bit PPGTT will create root pml4 table in gen8_ppgtt_init(), and
after that, the PPGTT structure is complete, so we still need the
notification.

I did not tested the path though, because I found currently
i915.enable_ppgtt will not equal to 3. The whole 48-bit PPGTT support
in GVT-g is verified with windows VM. Thanks!

> 
> > +		u64 daddr = px_dma(&ppgtt->pml4);
> > +
> > +		I915_WRITE(offset, daddr & 0xffffffff);
> > +		I915_WRITE(offset + 4, daddr >> 32);
> > +
> > +		msg = (create ? VGT_G2V_PPGTT_L4_PAGE_TABLE_CREATE :
> > +				VGT_G2V_PPGTT_L4_PAGE_TABLE_DESTROY)
> > ;
> > +	} else {
> > +		for (i = 0; i < GEN8_LEGACY_PDPES; i++) {
> > +			u64 daddr = i915_page_dir_dma_addr(ppgtt, 
> > i);
> > +
> > +			I915_WRITE(offset, daddr & 0xffffffff);
> > +			I915_WRITE(offset + 4, daddr >> 32);
> > +
> > +			offset += 8;
> > +		}
> > +
> > +		msg = (create ? VGT_G2V_PPGTT_L3_PAGE_TABLE_CREATE :
> > +				VGT_G2V_PPGTT_L3_PAGE_TABLE_DESTROY)
> > ;
> > +	}
> > +
> > +	I915_WRITE(vgtif_reg(g2v_notify), msg);
> > +
> > +	return 0;
> > +}
> > +
> >  static void gen8_free_scratch(struct i915_address_space *vm)
> >  {
> >  	struct drm_device *dev = vm->dev;
> > @@ -942,6 +977,9 @@ static void gen8_ppgtt_cleanup(struct 
> > i915_address_space *vm)
> >  	struct i915_hw_ppgtt *ppgtt =
> >  		container_of(vm, struct i915_hw_ppgtt, base);
> >  
> > +	if (intel_vgpu_active(vm->dev))
> > +		gen8_ppgtt_notify_vgt(ppgtt, false);
> > +
> >  	if (!USES_FULL_48BIT_PPGTT(ppgtt->base.dev))
> >  		gen8_ppgtt_cleanup_3lvl(ppgtt->base.dev, &ppgtt
> > ->pdp);
> >  	else
> > @@ -1516,6 +1554,9 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt 
> > *ppgtt)
> >  		}
> >  	}
> >  
> > +	if (intel_vgpu_active(ppgtt->base.dev))
> > +		gen8_ppgtt_notify_vgt(ppgtt, true);
> > +
> >  	return 0;
> >  
> >  free_scratch:
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
> > b/drivers/gpu/drm/i915/intel_lrc.c
> > index 4b2ac37..80d424b 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -136,6 +136,7 @@
> >  #include <drm/i915_drm.h>
> >  #include "i915_drv.h"
> >  #include "intel_mocs.h"
> > +#include "i915_vgpu.h"
> >  
> >  #define GEN9_LR_CONTEXT_RENDER_SIZE (22 * PAGE_SIZE)
> >  #define GEN8_LR_CONTEXT_RENDER_SIZE (20 * PAGE_SIZE)
> > @@ -2122,6 +2123,22 @@ make_rpcs(struct drm_device *dev)
> >  	return rpcs;
> >  }
> >  
> > +static void intel_lr_context_notify_vgt(struct intel_context *ctx,
> > +					struct intel_engine_cs 
> > *ring,
> > +					int msg)
> > +{
> > +	struct drm_device *dev = ring->dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	u64 tmp = intel_lr_context_descriptor(ctx, ring);
> > +
> > +	I915_WRITE(vgtif_reg(execlist_context_descriptor_lo),
> > +			tmp & 0xffffffff);
> > +	I915_WRITE(vgtif_reg(execlist_context_descriptor_hi),
> > +			tmp >> 32);
> > +
> > +	I915_WRITE(vgtif_reg(g2v_notify), msg);
> > +}
> > +
> 
> Why the other interface has bool for action and the other msg?

It would be better to change the "int" to "bool", so that the
notification type can be hidden inside this function. I will make the
change. Thanks!

> 
> Regards, Joonas
> 
> >  static int
> >  populate_lr_context(struct intel_context *ctx, struct 
> > drm_i915_gem_object *ctx_obj,
> >  		    struct intel_engine_cs *ring, struct 
> > intel_ringbuffer *ringbuf)
> > @@ -2282,6 +2299,10 @@ void intel_lr_context_free(struct 
> > intel_context *ctx)
> >  					ctx->engine[i].ringbuf;
> >  			struct intel_engine_cs *ring = ringbuf
> > ->ring;
> >  
> > +			if (intel_vgpu_active(ringbuf->ring->dev))
> > +				intel_lr_context_notify_vgt(ctx, 
> > ring,
> > +					VGT_G2V_EXECLIST_CONTEXT_DES
> > TROY);
> > +
> >  			if ((ctx == ring->default_context) ||
> >  			    (intel_vgpu_active(ring->dev))) {
> >  				intel_unpin_ringbuffer_obj(ringbuf);
> > @@ -2439,6 +2460,10 @@ int intel_lr_context_deferred_create(struct 
> > intel_context *ctx,
> >  	ctx->engine[ring->id].ringbuf = ringbuf;
> >  	ctx->engine[ring->id].state = ctx_obj;
> >  
> > +	if (intel_vgpu_active(dev))
> > +		intel_lr_context_notify_vgt(ctx, ring,
> > +				VGT_G2V_EXECLIST_CONTEXT_CREATE);
> > +
> >  	if (ctx == ring->default_context)
> >  		lrc_setup_hardware_status_page(ring, ctx_obj);
> >  	else if (ring->id == RCS && !ctx->rcs_initialized) {