[PATCH] drm/i915/gvt: Fix aperture read/write emulation when enable x-no-mmap=on

[Du, Changbin]

On Tue, Jan 30, 2018 at 12:35:58PM +0800, Zhenyu Wang wrote:
> On 2018.01.30 12:04:18 +0800, Du, Changbin wrote:
> > On Tue, Jan 30, 2018 at 10:56:30AM +0800, Zhenyu Wang wrote:
> > > On 2018.01.26 14:58:48 +0800, changbin.du at intel.com wrote:
> > > > From: Changbin Du <changbin.du at intel.com>
> > > > 
> > > > When add 'x-no-mmap=on' for vfio-pci option, aperture access in guest
> > > > is emulated. But the vgpu_aperture_rw() function take wrong offset when
> > > > do memcpy, since vgpu->gm.aperture_va is not the base of entire aperture.
> > > > This mistake cause GPU command in guest get lost and so the seqno is not
> > > > updated in engine HWSP.
> > > > 
> > > > This patch fix this, and it also move the emulation code to kvmgt.
> > > > Because only vfio need to emulate it. Put aperture rw to MMIO emulation
> > > > path breaks assumptions in xengt.
> > > > 
> > > > Fixes: f090a00df9ec ("drm/i915/gvt: Add emulation for BAR2 (aperture) with normal file RW approach")
> > > > Signed-off-by: Changbin Du <changbin.du at intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/gvt/cfg_space.c | 15 +------------
> > > >  drivers/gpu/drm/i915/gvt/gvt.h       |  1 -
> > > >  drivers/gpu/drm/i915/gvt/kvmgt.c     | 36 +++++++++++++++++++++++++++++--
> > > >  drivers/gpu/drm/i915/gvt/mmio.c      | 42 ------------------------------------
> > > >  4 files changed, 35 insertions(+), 59 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/gvt/cfg_space.c b/drivers/gpu/drm/i915/gvt/cfg_space.c
> > > > index 97bfc00..c62346f 100644
> > > > --- a/drivers/gpu/drm/i915/gvt/cfg_space.c
> > > > +++ b/drivers/gpu/drm/i915/gvt/cfg_space.c
> > > > @@ -119,16 +119,6 @@ static int map_aperture(struct intel_vgpu *vgpu, bool map)
> > > >  	if (map == vgpu->cfg_space.bar[INTEL_GVT_PCI_BAR_APERTURE].tracked)
> > > >  		return 0;
> > > >  
> > > > -	if (map) {
> > > > -		vgpu->gm.aperture_va = memremap(aperture_pa, aperture_sz,
> > > > -						MEMREMAP_WC);
> > > > -		if (!vgpu->gm.aperture_va)
> > > > -			return -ENOMEM;
> > > > -	} else {
> > > > -		memunmap(vgpu->gm.aperture_va);
> > > > -		vgpu->gm.aperture_va = NULL;
> > > > -	}
> > > > -
> > > >  	val = vgpu_cfg_space(vgpu)[PCI_BASE_ADDRESS_2];
> > > >  	if (val & PCI_BASE_ADDRESS_MEM_TYPE_64)
> > > >  		val = *(u64 *)(vgpu_cfg_space(vgpu) + PCI_BASE_ADDRESS_2);
> > > > @@ -141,11 +131,8 @@ static int map_aperture(struct intel_vgpu *vgpu, bool map)
> > > >  						  aperture_pa >> PAGE_SHIFT,
> > > >  						  aperture_sz >> PAGE_SHIFT,
> > > >  						  map);
> > > > -	if (ret) {
> > > > -		memunmap(vgpu->gm.aperture_va);
> > > > -		vgpu->gm.aperture_va = NULL;
> > > > +	if (ret)
> > > >  		return ret;
> > > > -	}
> > > >  
> > > >  	vgpu->cfg_space.bar[INTEL_GVT_PCI_BAR_APERTURE].tracked = map;
> > > >  	return 0;
> > > > diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
> > > > index 7dc7a80..84b04f6 100644
> > > > --- a/drivers/gpu/drm/i915/gvt/gvt.h
> > > > +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> > > > @@ -82,7 +82,6 @@ struct intel_gvt_device_info {
> > > >  struct intel_vgpu_gm {
> > > >  	u64 aperture_sz;
> > > >  	u64 hidden_sz;
> > > > -	void *aperture_va;
> > > >  	struct drm_mm_node low_gm_node;
> > > >  	struct drm_mm_node high_gm_node;
> > > >  };
> > > > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > > > index 70f03e9..3c68727 100644
> > > > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> > > > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > > > @@ -651,6 +651,39 @@ static int intel_vgpu_bar_rw(struct intel_vgpu *vgpu, int bar, uint64_t off,
> > > >  	return ret;
> > > >  }
> > > >  
> > > > +static inline bool intel_vgpu_in_aperture(struct intel_vgpu *vgpu, uint64_t off)
> > > > +{
> > > > +	return off >= vgpu_aperture_offset(vgpu) &&
> > > > +	       off < vgpu_aperture_offset(vgpu) + vgpu_aperture_sz(vgpu);
> > > > +}
> > > > +
> > > > +static int intel_vgpu_aperture_rw(struct intel_vgpu *vgpu, uint64_t off,
> > > > +		void *buf, unsigned long count, bool is_write)
> > > > +{
> > > > +	void *aperture_va;
> > > > +
> > > > +	if (!intel_vgpu_in_aperture(vgpu, off) ||
> > > > +	    !intel_vgpu_in_aperture(vgpu, off + count)) {
> > > > +		gvt_vgpu_err("Invalid aperture offset %llu\n", off);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	aperture_va = io_mapping_map_wc(&vgpu->gvt->dev_priv->ggtt.iomap,
> > > > +					ALIGN_DOWN(off, PAGE_SIZE),
> > > > +					PAGE_ALIGN(count + offset_in_page(off)));
> > > 
> > > I don't think we need page aligned size? Other looks fine to me.
> > > 
> > 'Using an offset which is not page aligned yields an undefined result.' This is
> > from the document, though it is not a problem on x86 arch.
> >
> 
> offset should be page aligned as is assumed by io_map atomic map,
> but not require for size, and as we don't support 32bit host yet, it
> just degrades to ioremap.
> 
ok, seems no requirement for size. Will remove it.

> -- 
> Open Source Technology Center, Intel ltd.
> 
> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827



> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev


-- 
Thanks,
Changbin Du