| Message ID | 20230801230520.1509954-2-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4.1] drm/i915/gvt: Explicitly check that vGPU is attached before shadowing | expand |
Reviewed-by: Yan Zhao <yan.y.zhao@intel.com> Tested-by: Yan Zhao <yan.y.zhao@intel.com> On Tue, Aug 01, 2023 at 04:05:21PM -0700, Sean Christopherson wrote: > Move the check that a vGPU is attached from is_2MB_gtt_possible() all the > way up to shadow_ppgtt_mm() to avoid unnecessary work, and to make it more > obvious that a future cleanup of is_2MB_gtt_possible() isn't introducing a > bug. > > is_2MB_gtt_possible() has only one caller, ppgtt_populate_shadow_entry(), > and all paths in ppgtt_populate_shadow_entry() eventually check for > attachment by way of intel_gvt_dma_map_guest_page(). > > And of the paths that lead to ppgtt_populate_shadow_entry(), > shadow_ppgtt_mm() is the only one that doesn't already check for > INTEL_VGPU_STATUS_ACTIVE or INTEL_VGPU_STATUS_ATTACHED. > > workload_thread() <= pick_next_workload() => INTEL_VGPU_STATUS_ACTIVE > | > -> dispatch_workload() > | > |-> prepare_workload() > | > -> intel_vgpu_sync_oos_pages() > | | > | |-> ppgtt_set_guest_page_sync() > | | > | |-> sync_oos_page() > | | > | |-> ppgtt_populate_shadow_entry() > | > |-> intel_vgpu_flush_post_shadow() > | > 1: |-> ppgtt_handle_guest_write_page_table() > | > |-> ppgtt_handle_guest_entry_add() > | > 2: | -> ppgtt_populate_spt_by_guest_entry() > | | > | |-> ppgtt_populate_spt() > | | > | |-> ppgtt_populate_shadow_entry() > | | > | |-> ppgtt_populate_spt_by_guest_entry() [see 2] > | > |-> ppgtt_populate_shadow_entry() > > kvmgt_page_track_write() <= KVM callback => INTEL_VGPU_STATUS_ATTACHED > | > |-> intel_vgpu_page_track_handler() > | > |-> ppgtt_write_protection_handler() > | > |-> ppgtt_handle_guest_write_page_table_bytes() > | > |-> ppgtt_handle_guest_write_page_table() [see 1] > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > > v4.1: > > - Actually make the code do what the changelog says. [Yan] > - Fix a typo in the changelog. [Yan] > > drivers/gpu/drm/i915/gvt/gtt.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c > index 5426a27c1b71..de6a484090d7 100644 > --- a/drivers/gpu/drm/i915/gvt/gtt.c > +++ b/drivers/gpu/drm/i915/gvt/gtt.c > @@ -1163,8 +1163,6 @@ static int is_2MB_gtt_possible(struct intel_vgpu *vgpu, > if (!HAS_PAGE_SIZES(vgpu->gvt->gt->i915, I915_GTT_PAGE_SIZE_2M)) > return 0; > > - if (!test_bit(INTEL_VGPU_STATUS_ATTACHED, vgpu->status)) > - return -EINVAL; > pfn = gfn_to_pfn(vgpu->vfio_device.kvm, ops->get_pfn(entry)); > if (is_error_noslot_pfn(pfn)) > return -EINVAL; > @@ -1827,6 +1825,9 @@ static int shadow_ppgtt_mm(struct intel_vgpu_mm *mm) > if (mm->ppgtt_mm.shadowed) > return 0; > > + if (!test_bit(INTEL_VGPU_STATUS_ATTACHED, vgpu->status)) > + return -EINVAL; > + > mm->ppgtt_mm.shadowed = true; > > for (index = 0; index < ARRAY_SIZE(mm->ppgtt_mm.guest_pdps); index++) { > > base-commit: 03e8f77e106ba1d2fd980f8b38339dad33333a07 > -- > 2.41.0.585.gd2178a4bd4-goog >
diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c index 5426a27c1b71..de6a484090d7 100644 --- a/drivers/gpu/drm/i915/gvt/gtt.c +++ b/drivers/gpu/drm/i915/gvt/gtt.c @@ -1163,8 +1163,6 @@ static int is_2MB_gtt_possible(struct intel_vgpu *vgpu, if (!HAS_PAGE_SIZES(vgpu->gvt->gt->i915, I915_GTT_PAGE_SIZE_2M)) return 0; - if (!test_bit(INTEL_VGPU_STATUS_ATTACHED, vgpu->status)) - return -EINVAL; pfn = gfn_to_pfn(vgpu->vfio_device.kvm, ops->get_pfn(entry)); if (is_error_noslot_pfn(pfn)) return -EINVAL; @@ -1827,6 +1825,9 @@ static int shadow_ppgtt_mm(struct intel_vgpu_mm *mm) if (mm->ppgtt_mm.shadowed) return 0; + if (!test_bit(INTEL_VGPU_STATUS_ATTACHED, vgpu->status)) + return -EINVAL; + mm->ppgtt_mm.shadowed = true; for (index = 0; index < ARRAY_SIZE(mm->ppgtt_mm.guest_pdps); index++) {
Move the check that a vGPU is attached from is_2MB_gtt_possible() all the way up to shadow_ppgtt_mm() to avoid unnecessary work, and to make it more obvious that a future cleanup of is_2MB_gtt_possible() isn't introducing a bug. is_2MB_gtt_possible() has only one caller, ppgtt_populate_shadow_entry(), and all paths in ppgtt_populate_shadow_entry() eventually check for attachment by way of intel_gvt_dma_map_guest_page(). And of the paths that lead to ppgtt_populate_shadow_entry(), shadow_ppgtt_mm() is the only one that doesn't already check for INTEL_VGPU_STATUS_ACTIVE or INTEL_VGPU_STATUS_ATTACHED. workload_thread() <= pick_next_workload() => INTEL_VGPU_STATUS_ACTIVE | -> dispatch_workload() | |-> prepare_workload() | -> intel_vgpu_sync_oos_pages() | | | |-> ppgtt_set_guest_page_sync() | | | |-> sync_oos_page() | | | |-> ppgtt_populate_shadow_entry() | |-> intel_vgpu_flush_post_shadow() | 1: |-> ppgtt_handle_guest_write_page_table() | |-> ppgtt_handle_guest_entry_add() | 2: | -> ppgtt_populate_spt_by_guest_entry() | | | |-> ppgtt_populate_spt() | | | |-> ppgtt_populate_shadow_entry() | | | |-> ppgtt_populate_spt_by_guest_entry() [see 2] | |-> ppgtt_populate_shadow_entry() kvmgt_page_track_write() <= KVM callback => INTEL_VGPU_STATUS_ATTACHED | |-> intel_vgpu_page_track_handler() | |-> ppgtt_write_protection_handler() | |-> ppgtt_handle_guest_write_page_table_bytes() | |-> ppgtt_handle_guest_write_page_table() [see 1] Signed-off-by: Sean Christopherson <seanjc@google.com> --- v4.1: - Actually make the code do what the changelog says. [Yan] - Fix a typo in the changelog. [Yan] drivers/gpu/drm/i915/gvt/gtt.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) base-commit: 03e8f77e106ba1d2fd980f8b38339dad33333a07