[PATCH] drm/i915/gvt: support inconsecutive partial PTE update

Zhenyu Wang zhenyuw at linux.intel.com
Mon Sep 17 02:58:09 UTC 2018


On 2018.09.14 19:08:25 +0800, hang.yuan at linux.intel.com wrote:
> From: Hang Yuan <hang.yuan at linux.intel.com>
> 
> Previously we assumed two 4-byte writes to the same PTE coming in sequence.
> But recently we observed inconsecutive partial write happening as well. So
> this patch enhances the previous solution. It now uses a list to save more
> partial writes. If one partial write can be combined with another one in
> the list to construct a full PTE, update its shadow entry. Otherwise, save
> the partial write in the list.
> 
> This patch also update ggtt_mmio_write more to remove unnecessary virtual
> PTE read and add old shadow PTE invalidation on PTE update case.
> 
> v2: invalidate old entry and flush ggtt (Zhenyu)
> 
> Signed-off-by: Hang Yuan <hang.yuan at linux.intel.com>
> Cc: Yan Zhao <yan.y.zhao at intel.com>
> Cc: Xiaolin Zhang <xiaolin.zhang at intel.com>
> Cc: Zhenyu Wang <zhenyu.z.wang at intel.com>
> ---
>  drivers/gpu/drm/i915/gvt/gtt.c | 119 +++++++++++++++++++++--------------------
>  drivers/gpu/drm/i915/gvt/gtt.h |   9 +++-
>  2 files changed, 69 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
> index 2402395..cf11f8f 100644
> --- a/drivers/gpu/drm/i915/gvt/gtt.c
> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> @@ -1905,7 +1905,6 @@ static struct intel_vgpu_mm *intel_vgpu_create_ggtt_mm(struct intel_vgpu *vgpu)
>  		vgpu_free_mm(mm);
>  		return ERR_PTR(-ENOMEM);
>  	}
> -	mm->ggtt_mm.last_partial_off = -1UL;
>  
>  	return mm;
>  }
> @@ -1930,7 +1929,6 @@ void _intel_vgpu_mm_release(struct kref *mm_ref)
>  		invalidate_ppgtt_mm(mm);
>  	} else {
>  		vfree(mm->ggtt_mm.virtual_ggtt);
> -		mm->ggtt_mm.last_partial_off = -1UL;
>  	}
>  
>  	vgpu_free_mm(mm);
> @@ -2168,6 +2166,8 @@ static int emulate_ggtt_mmio_write(struct intel_vgpu *vgpu, unsigned int off,
>  	struct intel_gvt_gtt_entry e, m;
>  	dma_addr_t dma_addr;
>  	int ret;
> +	struct intel_gvt_partial_pte *partial_pte, *pos, *n;
> +	bool partial_update = false;
>  
>  	if (bytes != 4 && bytes != 8)
>  		return -EINVAL;
> @@ -2178,68 +2178,61 @@ static int emulate_ggtt_mmio_write(struct intel_vgpu *vgpu, unsigned int off,
>  	if (!vgpu_gmadr_is_valid(vgpu, gma))
>  		return 0;
>  
> -	ggtt_get_guest_entry(ggtt_mm, &e, g_gtt_index);
> -
> +	e.type = GTT_TYPE_GGTT_PTE;
>  	memcpy((void *)&e.val64 + (off & (info->gtt_entry_size - 1)), p_data,
>  			bytes);
>  
>  	/* If ggtt entry size is 8 bytes, and it's split into two 4 bytes
> -	 * write, we assume the two 4 bytes writes are consecutive.
> -	 * Otherwise, we abort and report error
> +	 * write, save the first 4 bytes in a list and update virtual
> +	 * PTE. Only update shadow PTE when the second 4 bytes comes.
>  	 */
>  	if (bytes < info->gtt_entry_size) {
> -		if (ggtt_mm->ggtt_mm.last_partial_off == -1UL) {
> -			/* the first partial part*/
> -			ggtt_mm->ggtt_mm.last_partial_off = off;
> -			ggtt_mm->ggtt_mm.last_partial_data = e.val64;
> -			return 0;
> -		} else if ((g_gtt_index ==
> -				(ggtt_mm->ggtt_mm.last_partial_off >>
> -				info->gtt_entry_size_shift)) &&
> -			(off !=	ggtt_mm->ggtt_mm.last_partial_off)) {
> -			/* the second partial part */
> -
> -			int last_off = ggtt_mm->ggtt_mm.last_partial_off &
> -				(info->gtt_entry_size - 1);
> -
> -			memcpy((void *)&e.val64 + last_off,
> -				(void *)&ggtt_mm->ggtt_mm.last_partial_data +
> -				last_off, bytes);
> -
> -			ggtt_mm->ggtt_mm.last_partial_off = -1UL;
> -		} else {
> -			int last_offset;
> -
> -			gvt_vgpu_err("failed to populate guest ggtt entry: abnormal ggtt entry write sequence, last_partial_off=%lx, offset=%x, bytes=%d, ggtt entry size=%d\n",
> -					ggtt_mm->ggtt_mm.last_partial_off, off,
> -					bytes, info->gtt_entry_size);
> -
> -			/* set host ggtt entry to scratch page and clear
> -			 * virtual ggtt entry as not present for last
> -			 * partially write offset
> -			 */
> -			last_offset = ggtt_mm->ggtt_mm.last_partial_off &
> -					(~(info->gtt_entry_size - 1));
> -
> -			ggtt_get_host_entry(ggtt_mm, &m, last_offset);
> -			ggtt_invalidate_pte(vgpu, &m);
> -			ops->set_pfn(&m, gvt->gtt.scratch_mfn);
> -			ops->clear_present(&m);
> -			ggtt_set_host_entry(ggtt_mm, &m, last_offset);
> -			ggtt_invalidate(gvt->dev_priv);
> -
> -			ggtt_get_guest_entry(ggtt_mm, &e, last_offset);
> -			ops->clear_present(&e);
> -			ggtt_set_guest_entry(ggtt_mm, &e, last_offset);
> -
> -			ggtt_mm->ggtt_mm.last_partial_off = off;
> -			ggtt_mm->ggtt_mm.last_partial_data = e.val64;
> +		bool found = false;
> +
> +		list_for_each_entry_safe(pos, n,
> +				&ggtt_mm->ggtt_mm.partial_pte_list, list) {
> +			if (g_gtt_index == pos->offset >>
> +					info->gtt_entry_size_shift) {
> +				if (off != pos->offset) {
> +					/* the second partial part*/
> +
> +					int last_off = pos->offset &
> +						(info->gtt_entry_size - 1);
> +
> +					memcpy((void *)&e.val64 + last_off,
> +						(void *)&pos->data + last_off,
> +						bytes);
> +
> +					list_del(&pos->list);
> +					kfree(pos);
> +				} else {
> +					/* update of the first partial part */
> +
> +					pos->data = e.val64;
> +					ggtt_set_guest_entry(ggtt_mm, &e,
> +						g_gtt_index);
> +					return 0;
> +				}
> +				found = true;
> +				break;

Move this two lines in above if (off != pos->offset) is easier to read.

> +			}
> +		}
>  
> -			return 0;
> +		if (!found) {
> +			/* the first partial part */
> +
> +			partial_pte = kzalloc(sizeof(*partial_pte), GFP_KERNEL);
> +			if (!partial_pte)
> +				return -ENOMEM;
> +			partial_pte->offset = off;
> +			partial_pte->data = e.val64;
> +			list_add_tail(&partial_pte->list,
> +				&ggtt_mm->ggtt_mm.partial_pte_list);
> +			partial_update = true;
>  		}
>  	}
>  
> -	if (ops->test_present(&e)) {
> +	if (!partial_update && (ops->test_present(&e))) {
>  		gfn = ops->get_pfn(&e);
>  		m = e;
>  
> @@ -2263,16 +2256,18 @@ static int emulate_ggtt_mmio_write(struct intel_vgpu *vgpu, unsigned int off,
>  		} else
>  			ops->set_pfn(&m, dma_addr >> PAGE_SHIFT);
>  	} else {
> -		ggtt_get_host_entry(ggtt_mm, &m, g_gtt_index);
> -		ggtt_invalidate_pte(vgpu, &m);
>  		ops->set_pfn(&m, gvt->gtt.scratch_mfn);
>  		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);
> +	ggtt_invalidate_pte(vgpu, &e);

For normal 8b size update of ggtt entry, does this invalidate of old pte page
always required? If yes, looks better split this as a separate fix instead.

> +
>  	ggtt_set_host_entry(ggtt_mm, &m, g_gtt_index);
>  	ggtt_invalidate(gvt->dev_priv);
> -	ggtt_set_guest_entry(ggtt_mm, &e, g_gtt_index);
>  	return 0;
>  }
>  
> @@ -2430,6 +2425,8 @@ int intel_vgpu_init_gtt(struct intel_vgpu *vgpu)
>  
>  	intel_vgpu_reset_ggtt(vgpu, false);
>  
> +	INIT_LIST_HEAD(&gtt->ggtt_mm->ggtt_mm.partial_pte_list);
> +
>  	return create_scratch_page_tree(vgpu);
>  }
>  
> @@ -2454,6 +2451,14 @@ static void intel_vgpu_destroy_all_ppgtt_mm(struct intel_vgpu *vgpu)
>  
>  static void intel_vgpu_destroy_ggtt_mm(struct intel_vgpu *vgpu)
>  {
> +	struct intel_gvt_partial_pte *pos;
> +
> +	list_for_each_entry(pos,
> +			&vgpu->gtt.ggtt_mm->ggtt_mm.partial_pte_list, list) {
> +		gvt_dbg_mm("partial PTE update on hold 0x%lx : 0x%llx\n",
> +			pos->offset, pos->data);
> +		kfree(pos);
> +	}
>  	intel_vgpu_destroy_mm(vgpu->gtt.ggtt_mm);
>  	vgpu->gtt.ggtt_mm = NULL;
>  }
> diff --git a/drivers/gpu/drm/i915/gvt/gtt.h b/drivers/gpu/drm/i915/gvt/gtt.h
> index 7a9b361..a11bfee 100644
> --- a/drivers/gpu/drm/i915/gvt/gtt.h
> +++ b/drivers/gpu/drm/i915/gvt/gtt.h
> @@ -133,6 +133,12 @@ enum intel_gvt_mm_type {
>  
>  #define GVT_RING_CTX_NR_PDPS	GEN8_3LVL_PDPES
>  
> +struct intel_gvt_partial_pte {
> +	unsigned long offset;
> +	u64 data;
> +	struct list_head list;
> +};
> +
>  struct intel_vgpu_mm {
>  	enum intel_gvt_mm_type type;
>  	struct intel_vgpu *vgpu;
> @@ -157,8 +163,7 @@ struct intel_vgpu_mm {
>  		} ppgtt_mm;
>  		struct {
>  			void *virtual_ggtt;
> -			unsigned long last_partial_off;
> -			u64 last_partial_data;
> +			struct list_head partial_pte_list;
>  		} ggtt_mm;
>  	};
>  };
> -- 
> 2.7.4
> 

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gvt-dev/attachments/20180917/f7ee334a/attachment.sig>


More information about the intel-gvt-dev mailing list