Re: [PATCH v5 1/3] drm/xe/migrate: Atomicize CCS copy command setup

["K V P, Satyanarayana" <satyanarayana.k.v.p@intel.com>]

On 10-10-2025 04:36, Matt Roper wrote:
> On Thu, Oct 09, 2025 at 11:49:16AM -0700, Matthew Brost wrote:
>> On Thu, Oct 09, 2025 at 02:35:10PM -0400, Rodrigo Vivi wrote:
>>> On Thu, Oct 09, 2025 at 09:11:13AM -0700, Matthew Brost wrote:
>>>> On Thu, Oct 09, 2025 at 09:00:43AM -0400, Rodrigo Vivi wrote:
>>>>> On Wed, Oct 08, 2025 at 03:58:32PM -0700, Matthew Brost wrote:
>>>>>> On Wed, Oct 08, 2025 at 03:41:47PM +0530, 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 256-bit chunks, update EMIT_COPY_CCS_DW to emit
>>>>>>> 8 dwords instead of 5 dwords.
>>>>>>>
>>>>>>> Update emit_flush_invalidate() to use VMOVDQU operating with 128-bit
>>>>>>> chunks.
>>>>>>>
>>>>>>> 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>
>>>>>>>
>>>>>>> ---
>>>>>>> V4 -> V5:
>>>>>>> - Fixed review comments. (Matt B)
>>>>>>>
>>>>>>> V3 -> V4:
>>>>>>> - Fixed review comments. (Wajdeczko)
>>>>>>> - Fix issues reported by patchworks.
>>>>>>>
>>>>>>> V2 -> V3:
>>>>>>> - Added support for 128 bit and 256 bit instructions with memcpy_vmovdqu
>>>>>>> - Updated emit_flush_invalidate() to use vmovdqu instruction.
>>>>>>>
>>>>>>> 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 | 93 +++++++++++++++++++++++++--------
>>>>>>>   1 file changed, 72 insertions(+), 21 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
>>>>>>> index c39c3b423d05..b629072956ee 100644
>>>>>>> --- a/drivers/gpu/drm/xe/xe_migrate.c
>>>>>>> +++ b/drivers/gpu/drm/xe/xe_migrate.c
>>>>>>> @@ -5,7 +5,9 @@
>>>>>>>   
>>>>>>>   #include "xe_migrate.h"
>>>>>>>   
>>>>>>> +#include <asm/fpu/api.h>
>>>>>>>   #include <linux/bitfield.h>
>>>>>>> +#include <linux/cpufeature.h>
>>>>>>>   #include <linux/sizes.h>
>>>>>>>   
>>>>>>>   #include <drm/drm_managed.h>
>>>>>>> @@ -33,6 +35,7 @@
>>>>>>>   #include "xe_res_cursor.h"
>>>>>>>   #include "xe_sa.h"
>>>>>>>   #include "xe_sched_job.h"
>>>>>>> +#include "xe_sriov_vf_ccs.h"
>>>>>>>   #include "xe_sync.h"
>>>>>>>   #include "xe_trace_bo.h"
>>>>>>>   #include "xe_validation.h"
>>>>>>> @@ -644,18 +647,49 @@ static void emit_pte(struct xe_migrate *m,
>>>>>>>   	}
>>>>>>>   }
>>>>>>>   
>>>>>>> -#define EMIT_COPY_CCS_DW 5
>>>>>>> +static void memcpy_vmovdqu(void *dst, const void *src, u32 size)
>>>>>>> +{
>>>>>>> +#ifdef CONFIG_X86
>>>>>>> +	kernel_fpu_begin();
>>>>>>> +	if (size == SZ_128) {
>>>>>>> +		asm("vmovdqu (%0), %%xmm0\n"
>>>>>>> +		    "vmovups %%xmm0,   (%1)\n"
>>>>>>> +		    :: "r" (src), "r" (dst) : "memory");
>>>>>>> +	} else if (size == SZ_256) {
>>>>>>> +		asm("vmovdqu (%0), %%ymm0\n"
>>>>>>> +		    "vmovups %%ymm0,   (%1)\n"
>>>>>>> +		    :: "r" (src), "r" (dst) : "memory");
>>>>>>> +	}
>>>>>>> +	kernel_fpu_end();
>>>>>>> +#endif
>>>>>>
>>>>>> Everything in this patch LGTM but I think we maintainer input to ensure
>>>>>> we are breaking some rules about inlined asm code in a driver (no idea
>>>>>> if this exists) or if a better place would be somewhere common. Can you
>>>>>> ping Lucas, Thomas, or Rodrigo and ask them about this?
>>>>>
>>>>> Well, it is possible and we have asm code in i915 for instance (i915_memcpy.c)
>>>>>
>>>>> But the rule does exist:
>>>>> https://www.kernel.org/doc/html/latest/process/coding-style.html#inline-assembly
>>>>>
>>>>> "don’t use inline assembly gratuitously when C can do the job. You can and should
>>>>> poke hardware from C when possible"
>>>>>
>>>>> In this case here, please explain why exactly memcpy with smp_wmb barriers and
>>>>> or WRITE_ONCE code combined couldn't solve it.
>>>>>
>>>>> Also, please explain how exactly vmovdqu guarantees the atomicity promised by
>>>>> the commit message. On a quick search here my take is that for this 128 or 256
>>>>> bits, atomicity is not guaranteed.
>>>>
>>>> I don't think cache atomicity is what we're after here—rather, it's vCPU
>>>> halting atomicity.
>>>>
>>>> Consider the following case:
>>>> b++ = XY_CTRL_SURF_COPY_BLT;
>>>> b++ = addr;
>>>>
>>>> If the vCPU is halted during the instruction that stores
>>>> XY_CTRL_SURF_COPY_BLT, the address will be invalid. The GuC executes the
>>>> batch buffer (BB) that is being programmed as part of the VF save. This
>>>> will clearly cause the BB to hang due to a page fault on the copy
>>>> command.
>>>
>>> okay, perhaps this is what is getting me confused most
>>> what I don't understand in the flow is: why GuC is already
>>> executing it or going to execute it while you are going to a halt when
>>> writing the command to the buffer?  and not writing to the buffer first
>>> and then sending it to the exec queue?
>>>
>>
>> It how this feature was architected, will send over SaS link of the list.
> 
> I'm confused by this too.  At the point we're filling in the
> batchbuffer, the GuC isn't aware of the batch at all yet as far as I can
> see.  In xe_migrate_copy(), we've called xe_bb_new() to allocate a new
> batchbuffer, and then we start calling emit_* functions to poke
> instructions into that buffer.  At the point we call
> xe_migrate_ccs_copy(), the hardware still isn't aware that this buffer
> exists, so it shouldn't be possible for it to start executing.  Only
> later on when we eventually create a job for the batchbuffer (after
> we've finished emitting all of the commands) should it be possible for
> the hardware to start executing this.
> 
> If there's some other *future* changes (not present in the driver today)
> that change the design such that we allocate a batchbuffer and tell the
> GuC it's free to start executing it, but only fill in the contents after
> that point, then that needs to be clearly explained in the commit
> message.  But that also sounds like an fundamentally racy design, so I'm
> not sure why vCPU would be the only situations we'd be running into
> problems.
> 
> 
> Matt
> 
HI Matt,
  Please refer to xe_migrate_ccs_rw_copy() function which just creates 
BB and it does not submit job. The idea here is that, we have a 
sub-allocator which is already registered with Guc and the function 
xe_migrate_ccs_rw_copy() is allocating BBs from the sub-allocator.
When the vCPU is paused, GUC automatically submits these BBs to HW. So, 
we are making sure that the BB always contain valid GPU instructions so 
that HW will not report any page faults while executing.
I will share the SAS for this.

-Satya.
>>
>>>>
>>>> If the entire XY_CTRL_SURF_COPY_BLT is stored via an AVX instruction,
>>>> then either the GPU entire instruction is written or none of it is. I
>>>> believe vCPU halting guarantees that a CPU instruction is either fully
>>>> executed or not at all—regardless of how many micro-operations (uOPs) it
>>>> decodes into. If this guarantee does not hold, then the entire
>>>> architecture of CCS save/restore on PTL is fundamentally broken which is
>>>> always possible.
>>>
>>> Okay, this is guaranteed. I mean, the vCPU won't get halted in the middle
>>> of the vmovdqu nor vmovups. only before, between, or after them.
>>>
>>> But is this uncached and/or coherent? isn't there really any possibility that
>>> the command finished, but GuC mid-flight executing things aren't still
>>> seeing different cachelines?
>>>
>>
>> The GuC won't start executing until vCPU unpause on the save flow.
>> Restore flow is bit more tricky as vCPU are live when this happens but
>> we can W/A that race in software I think. That part is not in this
>> series.
>>
>>>>
>>>>>
>>>>> So, imho this patch is introducing a unmaintainable, complex, and fragile code
>>>>> that is not even doing what it is claiming to do. But I will be glad if someone
>>>>> can challenge this and prove me wrong.
>>>>>
>>>>
>>>> Let me know if the above makes any sense.
>>>
>>> Okay. But how to handle cases where AVX might not be available? really not needed?
>>>
>>
>> This iGPU feature for PTL so shouldn't be an issue as PTL has AVX
>> instructions.
>>
>> Matt
>>
>>>>
>>>> Matt
>>>>
>>>>> Thanks,
>>>>> Rodrigo.
>>>>>
>>>>>>
>>>>>> Matt
>>>>>>
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void emit_atomic(struct xe_gt *gt, void *dst, const void *src, u32 size)
>>>>>>> +{
>>>>>>> +	u32 instr_size = size * BITS_PER_BYTE;
>>>>>>> +
>>>>>>> +	xe_gt_assert(gt, instr_size == SZ_128 || instr_size == SZ_256);
>>>>>>> +
>>>>>>> +	if (IS_VF_CCS_READY(gt_to_xe(gt)) && static_cpu_has(X86_FEATURE_AVX))
>>>>>>> +		memcpy_vmovdqu(dst, src, instr_size);
>>>>>>> +	else
>>>>>>> +		memcpy(dst, src, size);
>>>>>>> +}
>>>>>>> +
>>>>>>> +#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,15 +707,23 @@ 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;
>>>>>>>   
>>>>>>> +	/*
>>>>>>> +	 * 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.
>>>>>>> +	 */
>>>>>>> +	emit_atomic(gt, cs, dw, sizeof(dw));
>>>>>>> +	cs += EMIT_COPY_CCS_DW;
>>>>>>>   	bb->len = cs - bb->cs;
>>>>>>>   }
>>>>>>>   
>>>>>>> @@ -993,18 +1035,27 @@ static u64 migrate_vm_ppgtt_addr_tlb_inval(void)
>>>>>>>   	return (NUM_KERNEL_PDE - 2) * XE_PAGE_SIZE;
>>>>>>>   }
>>>>>>>   
>>>>>>> -static int emit_flush_invalidate(u32 *dw, int i, u32 flags)
>>>>>>> +/*
>>>>>>> + * The MI_FLUSH_DW command is a 4-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
>>>>>>> + * emit_atomic() to write the sequence atomically.
>>>>>>> + */
>>>>>>> +#define EMIT_FLUSH_INVALIDATE_DW 4
>>>>>>> +static int emit_flush_invalidate(struct xe_exec_queue *q, u32 *cs, int i, u32 flags)
>>>>>>>   {
>>>>>>>   	u64 addr = migrate_vm_ppgtt_addr_tlb_inval();
>>>>>>> +	u32 dw[EMIT_FLUSH_INVALIDATE_DW] = {MI_NOOP}, j = 0;
>>>>>>> +
>>>>>>> +	dw[j++] = MI_FLUSH_DW | MI_INVALIDATE_TLB | MI_FLUSH_DW_OP_STOREDW |
>>>>>>> +		      MI_FLUSH_IMM_DW | flags;
>>>>>>> +	dw[j++] = lower_32_bits(addr);
>>>>>>> +	dw[j++] = upper_32_bits(addr);
>>>>>>> +	dw[j++] = MI_NOOP;
>>>>>>>   
>>>>>>> -	dw[i++] = MI_FLUSH_DW | MI_INVALIDATE_TLB | MI_FLUSH_DW_OP_STOREDW |
>>>>>>> -		  MI_FLUSH_IMM_DW | flags;
>>>>>>> -	dw[i++] = lower_32_bits(addr);
>>>>>>> -	dw[i++] = upper_32_bits(addr);
>>>>>>> -	dw[i++] = MI_NOOP;
>>>>>>> -	dw[i++] = MI_NOOP;
>>>>>>> +	emit_atomic(q->gt, &cs[i], dw, sizeof(dw));
>>>>>>>   
>>>>>>> -	return i;
>>>>>>> +	return i + j;
>>>>>>>   }
>>>>>>>   
>>>>>>>   /**
>>>>>>> @@ -1049,7 +1100,7 @@ int xe_migrate_ccs_rw_copy(struct xe_tile *tile, struct xe_exec_queue *q,
>>>>>>>   	/* Calculate Batch buffer size */
>>>>>>>   	batch_size = 0;
>>>>>>>   	while (size) {
>>>>>>> -		batch_size += 10; /* Flush + ggtt addr + 2 NOP */
>>>>>>> +		batch_size += EMIT_FLUSH_INVALIDATE_DW * 2; /* Flush + ggtt addr + 1 NOP */
>>>>>>>   		u64 ccs_ofs, ccs_size;
>>>>>>>   		u32 ccs_pt;
>>>>>>>   
>>>>>>> @@ -1090,7 +1141,7 @@ int xe_migrate_ccs_rw_copy(struct xe_tile *tile, struct xe_exec_queue *q,
>>>>>>>   	 * sizes here again before copy command is emitted.
>>>>>>>   	 */
>>>>>>>   	while (size) {
>>>>>>> -		batch_size += 10; /* Flush + ggtt addr + 2 NOP */
>>>>>>> +		batch_size += EMIT_FLUSH_INVALIDATE_DW * 2; /* Flush + ggtt addr + 1 NOP */
>>>>>>>   		u32 flush_flags = 0;
>>>>>>>   		u64 ccs_ofs, ccs_size;
>>>>>>>   		u32 ccs_pt;
>>>>>>> @@ -1113,11 +1164,11 @@ int xe_migrate_ccs_rw_copy(struct xe_tile *tile, struct xe_exec_queue *q,
>>>>>>>   
>>>>>>>   		emit_pte(m, bb, ccs_pt, false, false, &ccs_it, ccs_size, src);
>>>>>>>   
>>>>>>> -		bb->len = emit_flush_invalidate(bb->cs, bb->len, flush_flags);
>>>>>>> +		bb->len = emit_flush_invalidate(q, bb->cs, bb->len, flush_flags);
>>>>>>>   		flush_flags = xe_migrate_ccs_copy(m, bb, src_L0_ofs, src_is_pltt,
>>>>>>>   						  src_L0_ofs, dst_is_pltt,
>>>>>>>   						  src_L0, ccs_ofs, true);
>>>>>>> -		bb->len = emit_flush_invalidate(bb->cs, bb->len, flush_flags);
>>>>>>> +		bb->len = emit_flush_invalidate(q, bb->cs, bb->len, flush_flags);
>>>>>>>   
>>>>>>>   		size -= src_L0;
>>>>>>>   	}
>>>>>>> -- 
>>>>>>> 2.51.0
>>>>>>>
>