| Message ID | 20230513003600.818142-3-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gvt: KVM: KVMGT fixes and page-track cleanups | expand |
On 5/13/2023 8:35 AM, Sean Christopherson wrote: > From: Yan Zhao <yan.y.zhao@intel.com> > Acked-by: Zhi Wang <zhi.a.wang@intel.com> 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@intel.com> > [sean: remove now-unused local variables] > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > 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);
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);