On 5/13/2023 8:35 AM, Sean Christopherson wrote:
> From: Yan Zhao <yan.y.zhao@xxxxxxxxx>
>
Acked-by: Zhi Wang <zhi.a.wang@xxxxxxxxx>
This was previously to avoid stepping down to the lower level ASAP
when a guest page is not used but still stays in the GPU page table.
(mostly in windows VM, as this is a behavior of WDDM GPU MM). But
it doesn't totally solve the the trapping flood in practice. I am
totally fine that it is removed. Post shadow in the trapping
path might be extended to handle this problem.
> Currently intel_gvt_is_valid_gfn() is called in two places:
> (1) shadowing guest GGTT entry
> (2) shadowing guest PPGTT leaf entry,
> which was introduced in commit cc753fbe1ac4
> ("drm/i915/gvt: validate gfn before set shadow page entry").
>
> However, now it's not necessary to call this interface any more, because
> a. GGTT partial write issue has been fixed by
> commit bc0686ff5fad
> ("drm/i915/gvt: support inconsecutive partial gtt entry write")
> commit 510fe10b6180
> ("drm/i915/gvt: fix a bug of partially write ggtt enties")
> b. PPGTT resides in normal guest RAM and we only treat 8-byte writes
> as valid page table writes. Any invalid GPA found is regarded as
> an error, either due to guest misbehavior/attack or bug in host
> shadow code.
> So,rather than do GFN pre-checking and replace invalid GFNs with
> scratch GFN and continue silently, just remove the pre-checking and
> abort PPGTT shadowing on error detected.
> c. GFN validity check is still performed in
> intel_gvt_dma_map_guest_page() --> gvt_pin_guest_page().
> It's more desirable to call VFIO interface to do both validity check
> and mapping.
> Calling intel_gvt_is_valid_gfn() to do GFN validity check from KVM side
> while later mapping the GFN through VFIO interface is unnecessarily
> fragile and confusing for unaware readers.
>
> Signed-off-by: Yan Zhao <yan.y.zhao@xxxxxxxxx>
> [sean: remove now-unused local variables]
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/gvt/gtt.c | 36 +---------------------------------
> 1 file changed, 1 insertion(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
> index 58b9b316ae46..f30922c55a0c 100644
> --- a/drivers/gpu/drm/i915/gvt/gtt.c
> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> @@ -49,22 +49,6 @@
> static bool enable_out_of_sync = false;
> static int preallocated_oos_pages = 8192;
>
> -static bool intel_gvt_is_valid_gfn(struct intel_vgpu *vgpu, unsigned long gfn)
> -{
> - struct kvm *kvm = vgpu->vfio_device.kvm;
> - int idx;
> - bool ret;
> -
> - if (!test_bit(INTEL_VGPU_STATUS_ATTACHED, vgpu->status))
> - return false;
> -
> - idx = srcu_read_lock(&kvm->srcu);
> - ret = kvm_is_visible_gfn(kvm, gfn);
> - srcu_read_unlock(&kvm->srcu, idx);
> -
> - return ret;
> -}
> -
> /*
> * validate a gm address and related range size,
> * translate it to host gm address
> @@ -1333,11 +1317,9 @@ static int ppgtt_populate_shadow_entry(struct intel_vgpu *vgpu,
> static int ppgtt_populate_spt(struct intel_vgpu_ppgtt_spt *spt)
> {
> struct intel_vgpu *vgpu = spt->vgpu;
> - struct intel_gvt *gvt = vgpu->gvt;
> - const struct intel_gvt_gtt_pte_ops *ops = gvt->gtt.pte_ops;
> struct intel_vgpu_ppgtt_spt *s;
> struct intel_gvt_gtt_entry se, ge;
> - unsigned long gfn, i;
> + unsigned long i;
> int ret;
>
> trace_spt_change(spt->vgpu->id, "born", spt,
> @@ -1354,13 +1336,6 @@ static int ppgtt_populate_spt(struct intel_vgpu_ppgtt_spt *spt)
> ppgtt_generate_shadow_entry(&se, s, &ge);
> ppgtt_set_shadow_entry(spt, &se, i);
> } else {
> - gfn = ops->get_pfn(&ge);
> - if (!intel_gvt_is_valid_gfn(vgpu, gfn)) {
> - ops->set_pfn(&se, gvt->gtt.scratch_mfn);
> - ppgtt_set_shadow_entry(spt, &se, i);
> - continue;
> - }
> -
> ret = ppgtt_populate_shadow_entry(vgpu, spt, i, &ge);
> if (ret)
> goto fail;
> @@ -2335,14 +2310,6 @@ static int emulate_ggtt_mmio_write(struct intel_vgpu *vgpu, unsigned int off,
> m.val64 = e.val64;
> m.type = e.type;
>
> - /* one PTE update may be issued in multiple writes and the
> - * first write may not construct a valid gfn
> - */
> - if (!intel_gvt_is_valid_gfn(vgpu, gfn)) {
> - ops->set_pfn(&m, gvt->gtt.scratch_mfn);
> - goto out;
> - }
> -
> ret = intel_gvt_dma_map_guest_page(vgpu, gfn, PAGE_SIZE,
> &dma_addr);
> if (ret) {
> @@ -2359,7 +2326,6 @@ static int emulate_ggtt_mmio_write(struct intel_vgpu *vgpu, unsigned int off,
> ops->clear_present(&m);
> }
>
> -out:
> ggtt_set_guest_entry(ggtt_mm, &e, g_gtt_index);
>
> ggtt_get_host_entry(ggtt_mm, &e, g_gtt_index);
