Re: [PATCH v2 03/27] drm/i915/gvt: remove interface intel_gvt_is_valid_gfn

[Yan Zhao <yan.y.zhao@intel.com>]

Tested-by: Yan Zhao <yan.y.zhao@intel.com>

On Fri, Mar 10, 2023 at 04:22:34PM -0800, Sean Christopherson wrote:
> From: Yan Zhao <yan.y.zhao@intel.com>
> 
> 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);
> -- 
> 2.40.0.rc1.284.g88254d51c5-goog
>