[PATCH v2] drm/i915/gvt: Support BAR0 8-byte reads/writes

[Zhang, Tina]

> -----Original Message-----
> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces at lists.freedesktop.org] On
> Behalf Of Zhenyu Wang
> Sent: Friday, February 2, 2018 2:27 PM
> To: Zhang, Tina <tina.zhang at intel.com>
> Cc: Du, Changbin <changbin.du at intel.com>; intel-gvt-
> dev at lists.freedesktop.org; Wang, Zhi A <zhi.a.wang at intel.com>
> Subject: Re: [PATCH v2] drm/i915/gvt: Support BAR0 8-byte reads/writes
> 
> On 2018.02.02 10:37:08 +0800, Tina Zhang wrote:
> > GGTT is in BAR0 with 8 bytes aligned. With a qemu patch (commit:
> > 38d49e8c1523d97d2191190d3f7b4ce7a0ab5aa3), VFIO can use 8-byte
> reads/
> > writes to access it.
> >
> > This patch is to support the 8-byte GGTT reads/writes.
> >
> > Ideally, we would like to support 8-byte reads/writes for the total BAR0.
> > But it needs more work for handling 8-byte MMIO reads/writes.
> >
> > This patch can fix the issue caused by partial updating GGTT entry,
> > during guest booting up.
> >
> > v2:
> > - Limit to GGTT entry. (Zhenyu)
> >
> > Signed-off-by: Tina Zhang <tina.zhang at intel.com>
> > ---
> >  drivers/gpu/drm/i915/gvt/kvmgt.c | 51
> > ++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 49 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c
> > b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > index f4d5aad..6a90794 100644
> > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > @@ -708,16 +708,47 @@ static ssize_t intel_vgpu_rw(struct mdev_device
> *mdev, char *buf,
> >  	return ret == 0 ? count : ret;
> >  }
> >
> > +static bool gtt_entry(struct mdev_device *mdev, loff_t *ppos) {
> > +	struct intel_vgpu *vgpu = mdev_get_drvdata(mdev);
> > +	struct intel_gvt *gvt = vgpu->gvt;
> > +	int offset;
> > +
> > +	offset = (u64)(*ppos & VFIO_PCI_OFFSET_MASK) -
> > +		(*(u64 *)(vgpu_cfg_space(vgpu) + PCI_BASE_ADDRESS_0) &
> > +			~GENMASK(3, 0));
> > +
> 
> Use intel_vgpu_get_bar_gpa(vgpu, PCI_BASE_ADDRESS_0)?
Kvmgt is a standalone module, which cannot directly use the functions like intel_vgpu_get_bar_gpa(vgpu, PCI_BASE_ADDRESS_0) provided by other modules.

> 
> > +	return (offset >= gvt->device_info.gtt_start_offset &&
> > +		offset < gvt->device_info.gtt_start_offset + gvt_ggtt_sz(gvt)) ?
> > +			true : false;
> > +}
> > +
> >  static ssize_t intel_vgpu_read(struct mdev_device *mdev, char __user *buf,
> >  			size_t count, loff_t *ppos)
> >  {
> >  	unsigned int done = 0;
> > +	unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
> >  	int ret;
> >
> >  	while (count) {
> >  		size_t filled;
> >
> > -		if (count >= 4 && !(*ppos % 4)) {
> > +		/* Only support GGTT entry 8 bytes read */
> > +		if (count >= 8 && !(*ppos % 8) &&
> > +		    (index == VFIO_PCI_BAR0_REGION_INDEX) &&
> > +			gtt_entry(mdev, ppos)) {
> 
> Combine bar0 index and gtt offset in one check function for 8b access?
You mean is one function? For example, check both index and offset in gtt_entry()?

> 
> And after this one, could you pick up duplicate code cleanup for this or
> Changbin can refresh?
In my understanding, functions like intel_vgpu_read() and intel_vgpu_write() are implemented very gracefully.
I would rather not to modify them, unless I have to...
Maybe Changbin could have more ideas on them.
Thanks.

BR,
Tina
> 
> > +			u64 val;
> > +
> > +			ret = intel_vgpu_rw(mdev, (char *)&val, sizeof(val),
> > +					ppos, false);
> > +			if (ret <= 0)
> > +				goto read_err;
> > +
> > +			if (copy_to_user(buf, &val, sizeof(val)))
> > +				goto read_err;
> > +
> > +			filled = 8;
> > +		} else if (count >= 4 && !(*ppos % 4)) {
> >  			u32 val;
> >
> >  			ret = intel_vgpu_rw(mdev, (char *)&val, sizeof(val),
> @@ -772,12
> > +803,28 @@ static ssize_t intel_vgpu_write(struct mdev_device *mdev,
> >  				size_t count, loff_t *ppos)
> >  {
> >  	unsigned int done = 0;
> > +	unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
> >  	int ret;
> >
> >  	while (count) {
> >  		size_t filled;
> >
> > -		if (count >= 4 && !(*ppos % 4)) {
> > +		/* Only support GGTT entry 8 bytes write */
> > +		if (count >= 8 && !(*ppos % 8) &&
> > +		    (index == VFIO_PCI_BAR0_REGION_INDEX) &&
> > +			gtt_entry(mdev, ppos)) {
> > +			u64 val;
> > +
> > +			if (copy_from_user(&val, buf, sizeof(val)))
> > +				goto write_err;
> > +
> > +			ret = intel_vgpu_rw(mdev, (char *)&val, sizeof(val),
> > +					ppos, true);
> > +			if (ret <= 0)
> > +				goto write_err;
> > +
> > +			filled = 8;
> > +		} else if (count >= 4 && !(*ppos % 4)) {
> >  			u32 val;
> >
> >  			if (copy_from_user(&val, buf, sizeof(val)))
> > --
> > 2.7.4
> >
> > _______________________________________________
> > intel-gvt-dev mailing list
> > intel-gvt-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> 
> --
> Open Source Technology Center, Intel ltd.
> 
> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827