[PATCH v2] drm/i915/gvt: Support BAR0 8-byte reads/writes
Zhang, Tina
tina.zhang at intel.com
Sun Feb 11 06:28:51 UTC 2018
> -----Original Message-----
> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces at lists.freedesktop.org] On
> Behalf Of Zhenyu Wang
> Sent: Friday, February 9, 2018 10:40 AM
> 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.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.
Just notice that it's true on staging branch, not 4.14-stable.
OK, then.
>
> > >
> > > > + 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
I'm not sure. Is it might be easier if this patch is applied first.
After all, I cannot see why the duplicating code removal logic should be included in this patch
which is going to fix a bug.
Thanks.
BR,
Tina
>
>
> --
> Open Source Technology Center, Intel ltd.
>
> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
More information about the intel-gvt-dev
mailing list