Re: [PATCH v2] drm/xe/migrate: Atomicize CCS copy command setup

[Matthew Brost <matthew.brost@intel.com>]

On Fri, Sep 26, 2025 at 01:14:37PM +0000, Satyanarayana K V P wrote:
> The CCS copy command is a 5-dword sequence. If the vCPU halts during
> save/restore while this sequence is being programmed, partial writes may
> trigger page faults when saving IGPU CCS metadata. Use the VMOVDQU
> instruction to write the sequence atomically.
> 
> Since VMOVDQU operates on 128-bit chunks, update EMIT_COPY_CCS_DW to emit
> 8 dwords instead of 5 dwords.
> 
> Signed-off-by: Satyanarayana K V P <satyanarayana.k.v.p@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>
> 
> ---
> V1 -> V2:
> - Use _memcpy_vmovdqu only for x86 arch and for VF. Else use memcpy
>   (Auld, Matthew)
> - Fix issues reported by patchworks.
> ---
>  drivers/gpu/drm/xe/xe_migrate.c | 51 +++++++++++++++++++++++++++------
>  1 file changed, 42 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
> index e2fff2107f68..88ed4d09a97e 100644
> --- a/drivers/gpu/drm/xe/xe_migrate.c
> +++ b/drivers/gpu/drm/xe/xe_migrate.c
> @@ -5,6 +5,7 @@
>  
>  #include "xe_migrate.h"
>  
> +#include <asm/fpu/api.h>
>  #include <linux/bitfield.h>
>  #include <linux/sizes.h>
>  
> @@ -644,18 +645,39 @@ static void emit_pte(struct xe_migrate *m,
>  	}
>  }
>  
> -#define EMIT_COPY_CCS_DW 5
> +#ifdef CONFIG_X86
> +/*
> + * The CCS copy command is a 5-dword sequence. If the vCPU halts during
> + * save/restore while this sequence is being issued, partial writes may trigger
> + * page faults when saving iGPU CCS metadata. Use the VMOVDQU instruction to
> + * write the sequence atomically.
> + */
> +static void memcpy_vmovdqu(void *dst, const void *src)
> +{
> +	kernel_fpu_begin();
> +
> +	asm("vmovdqu (%0), %%ymm0\n"
> +	    "vmovups %%ymm0,   (%1)\n"
> +	    :: "r" (src), "r" (dst) : "memory");
> +
> +	kernel_fpu_end();
> +}
> +#endif

I'd write this part like:

#ifdef CONFIG_X86
/* The code you have above */
#else
static void memcpy_vmovdqu(void *dst, const void *src)
{
	WARN_ON_ONCE("NOT POSSIBLE");
}
#endif

> +
> +#define EMIT_COPY_CCS_DW 8
>  static void emit_copy_ccs(struct xe_gt *gt, struct xe_bb *bb,
>  			  u64 dst_ofs, bool dst_is_indirect,
>  			  u64 src_ofs, bool src_is_indirect,
>  			  u32 size)
>  {
> +	u32 dw[EMIT_COPY_CCS_DW] = {MI_NOOP};
>  	struct xe_device *xe = gt_to_xe(gt);
>  	u32 *cs = bb->cs + bb->len;
>  	u32 num_ccs_blks;
>  	u32 num_pages;
>  	u32 ccs_copy_size;
>  	u32 mocs;
> +	u32 i = 0;
>  
>  	if (GRAPHICS_VERx100(xe) >= 2000) {
>  		num_pages = DIV_ROUND_UP(size, XE_PAGE_SIZE);
> @@ -673,14 +695,25 @@ static void emit_copy_ccs(struct xe_gt *gt, struct xe_bb *bb,
>  		mocs = FIELD_PREP(XY_CTRL_SURF_MOCS_MASK, gt->mocs.uc_index);
>  	}
>  
> -	*cs++ = XY_CTRL_SURF_COPY_BLT |
> -		(src_is_indirect ? 0x0 : 0x1) << SRC_ACCESS_TYPE_SHIFT |
> -		(dst_is_indirect ? 0x0 : 0x1) << DST_ACCESS_TYPE_SHIFT |
> -		ccs_copy_size;
> -	*cs++ = lower_32_bits(src_ofs);
> -	*cs++ = upper_32_bits(src_ofs) | mocs;
> -	*cs++ = lower_32_bits(dst_ofs);
> -	*cs++ = upper_32_bits(dst_ofs) | mocs;
> +	dw[i++] = XY_CTRL_SURF_COPY_BLT |
> +		  (src_is_indirect ? 0x0 : 0x1) << SRC_ACCESS_TYPE_SHIFT |
> +		  (dst_is_indirect ? 0x0 : 0x1) << DST_ACCESS_TYPE_SHIFT |
> +		  ccs_copy_size;
> +	dw[i++] = lower_32_bits(src_ofs);
> +	dw[i++] = upper_32_bits(src_ofs) | mocs;
> +	dw[i++] = lower_32_bits(dst_ofs);
> +	dw[i++] = upper_32_bits(dst_ofs) | mocs;
> +
> +#ifdef CONFIG_X86
> +	if (IS_SRIOV_VF(gt_to_xe(gt)) && static_cpu_has(X86_FEATURE_AVX))
> +		memcpy_vmovdqu(cs, dw);
> +	else
> +		memcpy(cs, dw, sizeof(u32) * EMIT_COPY_CCS_DW);
> +#else
> +	memcpy(cs, dw, sizeof(u32) * EMIT_COPY_CCS_DW);
> +#endif

Then here you won't need ifdef CONFIG_X86, rather just this:

	if (IS_SRIOV_VF(gt_to_xe(gt)) && static_cpu_has(X86_FEATURE_AVX))
		memcpy_vmovdqu(cs, dw);
	else
		memcpy(cs, dw, sizeof(u32) * EMIT_COPY_CCS_DW);

I also think emit_flush_invalidate needs to use memcpy_vmovdqu too.

I think it generally fine for the emit_pte() to be interrupted aside
from maybe the first two DW, so maybe convert that part into a simple QW
store too /w WRITE_ONCE + smp_wmb() so the barriers are correct.

Also the free side could be problematic too. We might need to carefully
use memcpy_vmovdqu to stragically reinsert NOPs in way that is safe
(e.g. clobber emit_flush_invalidate, emit_copy_ccs in single
instructions, for emit_pte work backwards clearing out the write value
first and then final clear first DW /w smp_wmb() + WRITE_ONCE.

We might need to block CCS BO allocations during VF post migration
recovery too, I'll think that part through but if needed we can do that
in a follow up.

Matt

> +
> +	cs += EMIT_COPY_CCS_DW;
>  
>  	bb->len = cs - bb->cs;
>  }
> -- 
> 2.43.0
>