Re: [PATCH v3 02/28] drm/i915/gvt: remove interface intel_gvt_is_valid_gfn

["Wang, Zhi A" <zhi.a.wang@intel.com>]

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);