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

[Zhenyu Wang]

On 2018.02.09 00:50:29 +0000, Zhang, Tina wrote:
> > 
> > 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.
>

hmm? That's inline function in gvt.h.

> > 
> > > +	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()?
>

yeah, e.g something like below for read (not tested)

diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index d86071a32b6a..643ffee199a2 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -733,6 +733,28 @@ static ssize_t intel_vgpu_rw(struct mdev_device *mdev, char *buf,
 	return ret == 0 ? count : ret;
 }
 
+static bool vgpu_access_ok_64(struct mdev_device *mdev, loff_t *ppos)
+{
+	struct intel_vgpu *vgpu = mdev_get_drvdata(mdev);
+	struct intel_gvt *gvt = vgpu->gvt;
+	unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
+	unsigned offset;
+
+	/* allow for GTT mmio access only */
+	
+	if (index != VFIO_PCI_BAR0_REGION_INDEX)
+		return false;
+
+	offset = (u64)(*ppos & VFIO_PCI_OFFSET_MASK) -
+		intel_vgpu_get_bar_gpa(vgpu, 0);
+
+       if (offset >= gvt->device_info.gtt_start_offset &&
+	    offset < gvt->device_info.gtt_start_offset + gvt_ggtt_sz(gvt))
+		return true;
+	else
+		return false;
+}
+
 static ssize_t intel_vgpu_read(struct mdev_device *mdev, char __user *buf,
 			size_t count, loff_t *ppos)
 {
@@ -742,7 +764,18 @@ static ssize_t intel_vgpu_read(struct mdev_device *mdev, char __user *buf,
 	while (count) {
 		size_t filled;
 
-		if (count >= 4 && !(*ppos % 4)) {
+		if (count >= 8 && !(*ppos % 8) &&
+		    vgpu_access_ok_64(mdev, ppos)) {
+			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),


> > 
> > 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.

I just mean the duplicate code removal patch e.g https://lists.freedesktop.org/archives/intel-gvt-dev/2018-January/002963.html


-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gvt-dev/attachments/20180209/a6ebb7f0/attachment.sig>