[PATCH v2] drm/i915/gvt: fix a bug of partially write ggtt enties

Zhenyu Wang zhenyuw at linux.intel.com
Tue Jun 26 02:05:13 UTC 2018


On 2018.06.22 08:28:05 +0800, Zhao, Yan Y wrote:
> hi zhenyu
> 
> 
> On 6/20/2018 3:05 PM, Zhenyu Wang wrote:
> 
>     On 2018.06.19 15:44:11 +0800, Zhao Yan wrote:
> 
>         when guest writes ggtt entries, it could write 8 bytes a time if
>         gtt_entry_size is 8. But, qemu could split the 8 bytes into 2 consecutive
>         4-byte writes.
> 
>         If each 4-byte partial write could trigger a host ggtt write, it is very
>         possible that a wrong combination is written to the host ggtt. E.g.
>         the higher 4 bytes is the old value, but the lower 4 bytes is the new
>         value, and this 8-byte combination is wrong but written to the ggtt, thus
>         causing bugs.
> 
>         To handle this condition, we just record the first 4-byte write, then wait
>         until the second 4-byte write comes and write the combined 64-bit data to
>         host ggtt table.
> 
>         To save memory space and to spot partial write as early as possible, we
>         don't keep this information for every ggtt index. Instread, we just record
>         the last ggtt write position, and assume the two 4-byte writes come in
>         consecutively for each vgpu.
> 
>         This assumption is right based on the characteristic of ggtt entry which
>         stores memory address. When gtt_entry_size is 8, the guest memory physical
>         address should be 64 bits, so any sane guest driver should write 8-byte
>         long data at a time, so 2 consecutive 4-byte writes at the same ggtt index
>         should be trapped in gvt.
> 
>         v2:
>         when incomplete ggtt entry write is located, e.g.
>             1. guest only writes 4 bytes at a ggtt offset and no long writes the
>                rest 4 bytes.
>             2. guest writes 4 bytes of a ggtt offset, then write at other ggtt
>                offsets, then return back to write the left 4 bytes of the first
>                ggtt offset.
>         add error handling logic to remap host entry to scratch page, and mark
>         guest virtual ggtt entry as not present.  (zhenyu wang)
> 
>         Signed-off-by: Zhao Yan <yan.y.zhao at intel.com>
>         ---
>          drivers/gpu/drm/i915/gvt/gtt.c | 58 ++++++++++++++++++++++++++++++++++++++++++
>          drivers/gpu/drm/i915/gvt/gtt.h |  2 ++
>          2 files changed, 60 insertions(+)
> 
>         diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
>         index 78e55aa..b4d8a7d 100644
>         --- a/drivers/gpu/drm/i915/gvt/gtt.c
>         +++ b/drivers/gpu/drm/i915/gvt/gtt.c
>         @@ -1591,6 +1591,7 @@ static struct intel_vgpu_mm *intel_vgpu_create_ggtt_mm(struct intel_vgpu *vgpu)
>                         vgpu_free_mm(mm);
>                         return ERR_PTR(-ENOMEM);
>                 }
>         +       mm->ggtt_mm.last_partial_off = -1UL;
> 
>                 return mm;
>          }
>         @@ -1615,6 +1616,7 @@ void _intel_vgpu_mm_release(struct kref *mm_ref)
>                         invalidate_ppgtt_mm(mm);
>                 } else {
>                         vfree(mm->ggtt_mm.virtual_ggtt);
>         +               mm->ggtt_mm.last_partial_off = -1UL;
>                 }
> 
>                 vgpu_free_mm(mm);
>         @@ -1867,6 +1869,62 @@ static int emulate_ggtt_mmio_write(struct intel_vgpu *vgpu, unsigned int off,
>                 memcpy((void *)&e.val64 + (off & (info->gtt_entry_size - 1)), p_data,
>                                 bytes);
> 
>         +       /* If ggtt entry size is 8 bytes, and it's split into two 4 bytes
>         +        * write, we assume the two 4 bytes writes are consecutive.
>         +        * Otherwise, we abort and report error
>         +        */
>         +       if (bytes < info->gtt_entry_size) {
>         +               if (ggtt_mm->ggtt_mm.last_partial_off == -1UL) {
>         +                       /* the first partial part*/
>         +                       ggtt_mm->ggtt_mm.last_partial_off = off;
>         +                       ggtt_mm->ggtt_mm.last_partial_data = e.val64;
>         +                       return 0;
>         +               } else if ((g_gtt_index ==
>         +                               (ggtt_mm->ggtt_mm.last_partial_off >>
>         +                               info->gtt_entry_size_shift)) &&
>         +                       (off != ggtt_mm->ggtt_mm.last_partial_off)) {
>         +                       /* the second partial part */
>         +
>         +                       int last_off = ggtt_mm->ggtt_mm.last_partial_off &
>         +                               (info->gtt_entry_size - 1);
>         +
>         +                       memcpy((void *)&e.val64 + last_off,
>         +                               (void *)&ggtt_mm->ggtt_mm.last_partial_data +
>         +                               last_off, bytes);
>         +
>         +                       ggtt_mm->ggtt_mm.last_partial_off = -1UL;
>         +               } else {
>         +                       int last_offset;
>         +
>         +                       gvt_vgpu_err("failed to populate guest ggtt entry: abnormal ggtt entry write sequence, last_partial_off=%lx, offset=%x, bytes=%d, ggtt entry size=%d\n",
>         +                                       ggtt_mm->ggtt_mm.last_partial_off, off,
>         +                                       bytes, info->gtt_entry_size);
>         +
>         +                       /* set host ggtt entry to scratch page and clear
>         +                        * virtual ggtt entry as not present for last
>         +                        * partially write offset
>         +                        */
>         +                       last_offset = ggtt_mm->ggtt_mm.last_partial_off &
>         +                                       (~(info->gtt_entry_size - 1));
>         +
>         +                       ggtt_get_host_entry(ggtt_mm, &m, last_offset);
>         +                       ggtt_invalidate_pte(vgpu, &m);
>         +                       ops->set_pfn(&m, gvt->gtt.scratch_mfn);
>         +                       ops->clear_present(&m);
> 
>     As have set to scratch page, still need to clear present on host?
> 
> yes, you are right, either setting to scratch page or clearing present is
> actually enough.
> But I still added "clear_present" here, based on two reasons:
> 1.  aligning present status of host entry and guest entry (as I will
> clear_present of guest entry immediately)
> 2.  keeping consistence with existing code which also clear_present of a entry
> to scratch page when a non-present guest entry is found.
> 
> So, do you think it's ok?

yep, and applied this to fixes.

Thanks!

> 
> 
>         +                       ggtt_set_host_entry(ggtt_mm, &m, last_offset);
>         +                       ggtt_invalidate(gvt->dev_priv);
>         +
>         +                       ggtt_get_guest_entry(ggtt_mm, &e, last_offset);
>         +                       ops->clear_present(&e);
>         +                       ggtt_set_guest_entry(ggtt_mm, &e, last_offset);
>         +
>         +                       ggtt_mm->ggtt_mm.last_partial_off = off;
>         +                       ggtt_mm->ggtt_mm.last_partial_data = e.val64;
>         +
>         +                       return 0;
>         +               }
>         +       }
>         +
>                 if (ops->test_present(&e)) {
>                         gfn = ops->get_pfn(&e);
>                         m = e;
>         diff --git a/drivers/gpu/drm/i915/gvt/gtt.h b/drivers/gpu/drm/i915/gvt/gtt.h
>         index 3792f2b..97e6264 100644
>         --- a/drivers/gpu/drm/i915/gvt/gtt.h
>         +++ b/drivers/gpu/drm/i915/gvt/gtt.h
>         @@ -150,6 +150,8 @@ struct intel_vgpu_mm {
>                         } ppgtt_mm;
>                         struct {
>                                 void *virtual_ggtt;
>         +                       unsigned long last_partial_off;
>         +                       u64 last_partial_data;
>                         } ggtt_mm;
>                 };
>          };
>         --
>         1.9.1
> 
>         _______________________________________________
>         intel-gvt-dev mailing list
>         intel-gvt-dev at lists.freedesktop.org
>         https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> 
> 
>    
>     _______________________________________________
>     intel-gvt-dev mailing list
>     intel-gvt-dev at lists.freedesktop.org
>     https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> 
> 

> _______________________________________________
> 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
-------------- 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/20180626/bd3bc109/attachment.sig>


More information about the intel-gvt-dev mailing list