[PATCH v7 1/6] riscv: vector: refactor vector context operations

[PATCH v7 1/6] riscv: vector: refactor vector context operations

[Andy Chiu]

Lift riscv_v_{enable,disable} out of __*vstate_{save,restore,discard} so
that we can reuse some functions without repeatedly turning on/off
vector.

Also, refactor and document about the user context save in preempt_v to
make code more readable.

Signed-off-by: Andy Chiu <tchiu@tenstorrent.com>
---
Changelog v5:
 - rebase on top of the kvm fix[1]
Changelog v4:
 - fix an unused variable warning (Olof)
Changelog v3:
 - new patch since v3
---
 arch/riscv/include/asm/kvm_vcpu_vector.h |  8 ++++++--
 arch/riscv/include/asm/vector.h          | 15 ++++++++-------
 arch/riscv/kernel/kernel_mode_vector.c   |  4 ++++
 arch/riscv/kvm/vcpu.c                    |  2 +-
 arch/riscv/kvm/vcpu_vector.c             |  6 +++---
 5 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/arch/riscv/include/asm/kvm_vcpu_vector.h b/arch/riscv/include/asm/kvm_vcpu_vector.h
index 50d4789b9cac..60fa28030944 100644
--- a/arch/riscv/include/asm/kvm_vcpu_vector.h
+++ b/arch/riscv/include/asm/kvm_vcpu_vector.h
@@ -16,14 +16,18 @@
 #ifdef CONFIG_RISCV_ISA_V
 #include <asm/kvm_host.h>
 
-static __always_inline void __kvm_riscv_vector_save(struct kvm_cpu_context *context)
+static __always_inline void kvm_riscv_vector_save(struct kvm_cpu_context *context)
 {
+	riscv_v_enable();
 	__riscv_v_vstate_save(&context->vector, context->vector.datap);
+	riscv_v_disable();
 }
 
-static __always_inline void __kvm_riscv_vector_restore(struct kvm_cpu_context *context)
+static __always_inline void kvm_riscv_vector_restore(struct kvm_cpu_context *context)
 {
+	riscv_v_enable();
 	__riscv_v_vstate_restore(&context->vector, context->vector.datap);
+	riscv_v_disable();
 }
 
 void kvm_riscv_vcpu_vector_reset(struct kvm_vcpu *vcpu);
diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
index 3381a535abb4..8c88a2258e61 100644
--- a/arch/riscv/include/asm/vector.h
+++ b/arch/riscv/include/asm/vector.h
@@ -203,7 +203,6 @@ static inline void __riscv_v_vstate_save(struct __riscv_v_ext_state *save_to,
 {
 	unsigned long vl;
 
-	riscv_v_enable();
 	__vstate_csr_save(save_to);
 	if (has_xtheadvector()) {
 		asm volatile (
@@ -232,7 +231,6 @@ static inline void __riscv_v_vstate_save(struct __riscv_v_ext_state *save_to,
 			".option pop\n\t"
 			: "=&r" (vl), "+r" (datap) : : "memory");
 	}
-	riscv_v_disable();
 }
 
 static inline void __riscv_v_vstate_restore(struct __riscv_v_ext_state *restore_from,
@@ -240,7 +238,6 @@ static inline void __riscv_v_vstate_restore(struct __riscv_v_ext_state *restore_
 {
 	unsigned long vl;
 
-	riscv_v_enable();
 	if (has_xtheadvector()) {
 		asm volatile (
 			"mv t0, %0\n\t"
@@ -269,14 +266,12 @@ static inline void __riscv_v_vstate_restore(struct __riscv_v_ext_state *restore_
 			: "=&r" (vl), "+r" (datap) : : "memory");
 	}
 	__vstate_csr_restore(restore_from);
-	riscv_v_disable();
 }
 
 static inline void __riscv_v_vstate_discard(void)
 {
 	unsigned long vl, vtype_inval = 1UL << (BITS_PER_LONG - 1);
 
-	riscv_v_enable();
 	if (has_xtheadvector())
 		asm volatile (THEAD_VSETVLI_T4X0E8M8D1 : : : "t4");
 	else
@@ -296,14 +291,14 @@ static inline void __riscv_v_vstate_discard(void)
 		"vsetvl		%0, x0, %1\n\t"
 		".option pop\n\t"
 		: "=&r" (vl) : "r" (vtype_inval));
-
-	riscv_v_disable();
 }
 
 static inline void riscv_v_vstate_discard(struct pt_regs *regs)
 {
 	if (riscv_v_vstate_query(regs)) {
+		riscv_v_enable();
 		__riscv_v_vstate_discard();
+		riscv_v_disable();
 		__riscv_v_vstate_dirty(regs);
 	}
 }
@@ -312,7 +307,9 @@ static inline void riscv_v_vstate_save(struct __riscv_v_ext_state *vstate,
 				       struct pt_regs *regs)
 {
 	if (__riscv_v_vstate_check(regs->status, DIRTY)) {
+		riscv_v_enable();
 		__riscv_v_vstate_save(vstate, vstate->datap);
+		riscv_v_disable();
 		__riscv_v_vstate_clean(regs);
 	}
 }
@@ -323,7 +320,9 @@ static inline void riscv_v_vstate_restore(struct __riscv_v_ext_state *vstate,
 	WARN_ON_ONCE(preemptible());
 
 	if (riscv_v_vstate_query(regs)) {
+		riscv_v_enable();
 		__riscv_v_vstate_restore(vstate, vstate->datap);
+		riscv_v_disable();
 		__riscv_v_vstate_clean(regs);
 	}
 }
@@ -385,8 +384,10 @@ static inline void __switch_to_vector(struct task_struct *prev,
 			riscv_v_disable();
 			prev->thread.riscv_v_flags |= RISCV_PREEMPT_V_IN_SCHEDULE;
 		} else if (riscv_preempt_v_dirty(prev)) {
+			riscv_v_enable();
 			__riscv_v_vstate_save(&prev->thread.kernel_vstate,
 					      prev->thread.kernel_vstate.datap);
+			riscv_v_disable();
 			riscv_preempt_v_clear_dirty(prev);
 		}
 	} else {
diff --git a/arch/riscv/kernel/kernel_mode_vector.c b/arch/riscv/kernel/kernel_mode_vector.c
index b9481a3e40e3..280180114899 100644
--- a/arch/riscv/kernel/kernel_mode_vector.c
+++ b/arch/riscv/kernel/kernel_mode_vector.c
@@ -171,7 +171,9 @@ static int riscv_v_start_kernel_context(void)
 		WARN_ON(riscv_v_ctx_get_depth() == 0);
 		get_cpu_vector_context();
 		if (riscv_preempt_v_dirty(current)) {
+			riscv_v_enable();
 			__riscv_v_vstate_save(kvstate, kvstate->datap);
+			riscv_v_disable();
 			riscv_preempt_v_clear_dirty(current);
 		}
 		riscv_preempt_v_set_restore(current);
@@ -215,7 +217,9 @@ asmlinkage void riscv_v_context_nesting_end(struct pt_regs *regs)
 	depth = riscv_v_ctx_get_depth();
 	if (depth == 0) {
 		if (riscv_preempt_v_restore(current)) {
+			riscv_v_enable();
 			__riscv_v_vstate_restore(vstate, vstate->datap);
+			riscv_v_disable();
 			__riscv_v_vstate_clean(regs);
 			riscv_preempt_v_reset_flags();
 		}
diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
index dd786f758eb7..caf6e0ad72ef 100644
--- a/arch/riscv/kvm/vcpu.c
+++ b/arch/riscv/kvm/vcpu.c
@@ -825,7 +825,7 @@ static void noinstr kvm_riscv_vcpu_enter_exit(struct kvm_vcpu *vcpu,
 	if (current->thread.riscv_v_flags & RISCV_V_VCPU_NEED_RESTORE) {
 		current->thread.riscv_v_flags &= ~RISCV_V_VCPU_NEED_RESTORE;
 		current->thread.riscv_v_flags |= RISCV_V_VCPU_CTX;
-		__kvm_riscv_vector_restore(gcntx);
+		kvm_riscv_vector_restore(gcntx);
 		gcntx->sstatus = (gcntx->sstatus & ~SR_VS) | SR_VS_CLEAN;
 	}
 
diff --git a/arch/riscv/kvm/vcpu_vector.c b/arch/riscv/kvm/vcpu_vector.c
index fefc2857f753..730c32f7de50 100644
--- a/arch/riscv/kvm/vcpu_vector.c
+++ b/arch/riscv/kvm/vcpu_vector.c
@@ -47,7 +47,7 @@ void kvm_riscv_vcpu_guest_vector_save(struct kvm_cpu_context *cntx,
 {
 	if ((cntx->sstatus & SR_VS) == SR_VS_DIRTY) {
 		if (riscv_isa_extension_available(isa, V))
-			__kvm_riscv_vector_save(cntx);
+			kvm_riscv_vector_save(cntx);
 		kvm_riscv_vcpu_vector_clean(cntx);
 	}
 }
@@ -65,13 +65,13 @@ void kvm_riscv_vcpu_host_vector_save(struct kvm_cpu_context *cntx)
 {
 	/* No need to check host sstatus as it can be modified outside */
 	if (!kvm_riscv_isa_check_host(V))
-		__kvm_riscv_vector_save(cntx);
+		kvm_riscv_vector_save(cntx);
 }
 
 void kvm_riscv_vcpu_host_vector_restore(struct kvm_cpu_context *cntx)
 {
 	if (!kvm_riscv_isa_check_host(V))
-		__kvm_riscv_vector_restore(cntx);
+		kvm_riscv_vector_restore(cntx);
 	riscv_v_flags_set(riscv_v_flags() & ~(RISCV_V_VCPU_CTX | RISCV_V_VCPU_NEED_RESTORE));
 }
 
-- 
2.43.0

Re: [PATCH v7 1/6] riscv: vector: refactor vector context operations

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] riscv: kvm: unrenamed __kvm_riscv_vector_restore() stub breaks
  !ISA_V build
- [Low] riscv: vector: commit message describes preempt_v changes not in
  diff
--

commit 7053b3ce403daff58cb731616963dcc0c16f7edb
Author: Andy Chiu <tchiu@tenstorrent.com>

riscv: vector: refactor vector context operations

This commit lifts riscv_v_enable() and riscv_v_disable() out of the
low-level __riscv_v_vstate_{save,restore,discard}() helpers into their
callers and renames the KVM vector save/restore helpers.

>     Also, refactor and document about the user context save in preempt_v to
>     make code more readable.

[Severity: Low]
This isn't a bug, but the preempt_v refactoring and comment described here
are not in this patch; they were already included in commit b5060a4aa33d
("RISC-V: KVM: fix vcpu vector context handling for kernel-mode vector").

> diff --git a/arch/riscv/include/asm/kvm_vcpu_vector.h b/arch/riscv/include/asm/kvm_vcpu_vector.h
> index 50d4789b9cac9..60fa280309444 100644
> --- a/arch/riscv/include/asm/kvm_vcpu_vector.h
> +++ b/arch/riscv/include/asm/kvm_vcpu_vector.h
> @@ -16,14 +16,18 @@
>  #ifdef CONFIG_RISCV_ISA_V
>  #include <asm/kvm_host.h>
>  
> -static __always_inline void __kvm_riscv_vector_save(struct kvm_cpu_context *context)
> +static __always_inline void kvm_riscv_vector_save(struct kvm_cpu_context *context)
>  {
> +	riscv_v_enable();
>  	__riscv_v_vstate_save(&context->vector, context->vector.datap);
> +	riscv_v_disable();
>  }
>  
> -static __always_inline void __kvm_riscv_vector_restore(struct kvm_cpu_context *context)
> +static __always_inline void kvm_riscv_vector_restore(struct kvm_cpu_context *context)
>  {
> +	riscv_v_enable();
>  	__riscv_v_vstate_restore(&context->vector, context->vector.datap);
> +	riscv_v_disable();
>  }

[ ... ]

> diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> index dd786f758eb76..caf6e0ad72efd 100644
> --- a/arch/riscv/kvm/vcpu.c
> +++ b/arch/riscv/kvm/vcpu.c
> @@ -825,7 +825,7 @@ static void noinstr kvm_riscv_vcpu_enter_exit(struct kvm_vcpu *vcpu,
>  	if (current->thread.riscv_v_flags & RISCV_V_VCPU_NEED_RESTORE) {
>  		current->thread.riscv_v_flags &= ~RISCV_V_VCPU_NEED_RESTORE;
>  		current->thread.riscv_v_flags |= RISCV_V_VCPU_CTX;
> -		__kvm_riscv_vector_restore(gcntx);
> +		kvm_riscv_vector_restore(gcntx);
>  		gcntx->sstatus = (gcntx->sstatus & ~SR_VS) | SR_VS_CLEAN;
>  	}

[Severity: High]
Does this break the build when CONFIG_KVM is enabled and CONFIG_RISCV_ISA_V
is disabled?  The stub in the #else branch of
arch/riscv/include/asm/kvm_vcpu_vector.h is still named
__kvm_riscv_vector_restore(), so kvm_riscv_vcpu_enter_exit() calls an
undeclared function.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111184359.1239821-1-tchiu@tenstorrent.com?part=1

[PATCH v7 6/6] riscv: KVM: do not save host vector context for vcpu

[Andy Chiu]

Host's vector contexts are already managed in per-thread's struct by
regular context switch routines. Saving and restoring the context in
preempt-notifier is redundant.

Signed-off-by: Andy Chiu <tchiu@tenstorrent.com>
---
Note: I haven't been able to get a statistically significant result on
how this help/regress KVM performance running a nginx server in guest.
I am happy to carry out a test again and post the number if there is any
suggestions on what workload can benefit from this, thanks!

Changelog v7:
 - new patch since v7
---
 arch/riscv/include/asm/kvm_vcpu_vector.h |  9 ++-------
 arch/riscv/kvm/vcpu.c                    |  3 +--
 arch/riscv/kvm/vcpu_vector.c             | 20 ++------------------
 3 files changed, 5 insertions(+), 27 deletions(-)

diff --git a/arch/riscv/include/asm/kvm_vcpu_vector.h b/arch/riscv/include/asm/kvm_vcpu_vector.h
index 60fa28030944..06409c870a06 100644
--- a/arch/riscv/include/asm/kvm_vcpu_vector.h
+++ b/arch/riscv/include/asm/kvm_vcpu_vector.h
@@ -35,8 +35,7 @@ void kvm_riscv_vcpu_guest_vector_save(struct kvm_cpu_context *cntx,
 				      unsigned long *isa);
 void kvm_riscv_vcpu_guest_vector_restore(struct kvm_cpu_context *cntx,
 					 unsigned long *isa);
-void kvm_riscv_vcpu_host_vector_save(struct kvm_cpu_context *cntx);
-void kvm_riscv_vcpu_host_vector_restore(struct kvm_cpu_context *cntx);
+void kvm_riscv_vcpu_host_vector_restore(void);
 int kvm_riscv_vcpu_alloc_vector_context(struct kvm_vcpu *vcpu);
 void kvm_riscv_vcpu_free_vector_context(struct kvm_vcpu *vcpu);
 void kvm_riscv_register_vctx_callback(void (*func)(void));
@@ -77,11 +76,7 @@ static inline void kvm_riscv_vcpu_guest_vector_restore(struct kvm_cpu_context *c
 {
 }
 
-static inline void kvm_riscv_vcpu_host_vector_save(struct kvm_cpu_context *cntx)
-{
-}
-
-static inline void kvm_riscv_vcpu_host_vector_restore(struct kvm_cpu_context *cntx)
+static inline void kvm_riscv_vcpu_host_vector_restore(void)
 {
 }
 
diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
index caf6e0ad72ef..d9d31851ee62 100644
--- a/arch/riscv/kvm/vcpu.c
+++ b/arch/riscv/kvm/vcpu.c
@@ -650,7 +650,6 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 	kvm_riscv_vcpu_guest_fp_restore(&vcpu->arch.guest_context,
 					vcpu->arch.isa);
 	get_cpu_vector_context();
-	kvm_riscv_vcpu_host_vector_save(&vcpu->arch.host_context);
 	kvm_riscv_vcpu_guest_vector_restore(&vcpu->arch.guest_context,
 					    vcpu->arch.isa);
 	put_cpu_vector_context();
@@ -677,7 +676,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 	get_cpu_vector_context();
 	kvm_riscv_vcpu_guest_vector_save(&vcpu->arch.guest_context,
 					 vcpu->arch.isa);
-	kvm_riscv_vcpu_host_vector_restore(&vcpu->arch.host_context);
+	kvm_riscv_vcpu_host_vector_restore();
 	put_cpu_vector_context();
 
 	if (kvm_riscv_nacl_available()) {
diff --git a/arch/riscv/kvm/vcpu_vector.c b/arch/riscv/kvm/vcpu_vector.c
index 730c32f7de50..6129a6e18404 100644
--- a/arch/riscv/kvm/vcpu_vector.c
+++ b/arch/riscv/kvm/vcpu_vector.c
@@ -61,17 +61,9 @@ void kvm_riscv_vcpu_guest_vector_restore(struct kvm_cpu_context *cntx,
 	}
 }
 
-void kvm_riscv_vcpu_host_vector_save(struct kvm_cpu_context *cntx)
+void kvm_riscv_vcpu_host_vector_restore(void)
 {
-	/* No need to check host sstatus as it can be modified outside */
-	if (!kvm_riscv_isa_check_host(V))
-		kvm_riscv_vector_save(cntx);
-}
-
-void kvm_riscv_vcpu_host_vector_restore(struct kvm_cpu_context *cntx)
-{
-	if (!kvm_riscv_isa_check_host(V))
-		kvm_riscv_vector_restore(cntx);
+	riscv_v_vstate_set_restore(current, task_pt_regs(current));
 	riscv_v_flags_set(riscv_v_flags() & ~(RISCV_V_VCPU_CTX | RISCV_V_VCPU_NEED_RESTORE));
 }
 
@@ -81,20 +73,12 @@ int kvm_riscv_vcpu_alloc_vector_context(struct kvm_vcpu *vcpu)
 	if (!vcpu->arch.guest_context.vector.datap)
 		return -ENOMEM;
 
-	vcpu->arch.host_context.vector.datap = kzalloc(riscv_v_vsize, GFP_KERNEL_ACCOUNT);
-	if (!vcpu->arch.host_context.vector.datap) {
-		kfree(vcpu->arch.guest_context.vector.datap);
-		vcpu->arch.guest_context.vector.datap = NULL;
-		return -ENOMEM;
-	}
-
 	return 0;
 }
 
 void kvm_riscv_vcpu_free_vector_context(struct kvm_vcpu *vcpu)
 {
 	kfree(vcpu->arch.guest_context.vector.datap);
-	kfree(vcpu->arch.host_context.vector.datap);
 }
 
 void kvm_riscv_vcpu_flush_vector(void)
-- 
2.43.0

Re: [PATCH v7 6/6] riscv: KVM: do not save host vector context for vcpu

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] riscv: KVM: build failure in kvm_riscv_vcpu_enter_exit() when
  !CONFIG_RISCV_ISA_V
- [High] riscv: KVM: guest CSR_VCSR state leaks to host in
  kvm_riscv_vcpu_host_vector_restore()
--

commit e0f5ce69db9b4e847c776901421a3c6de8e57845
Author: Andy Chiu <tchiu@tenstorrent.com>

riscv: KVM: do not save host vector context for vcpu

This commit removes the per-vCPU host vector context buffer and replaces the
explicit host vector context save and restore in kvm_arch_vcpu_load() and
kvm_arch_vcpu_put() with deferred thread vector state restoration.

> diff --git a/arch/riscv/include/asm/kvm_vcpu_vector.h b/arch/riscv/include/asm/kvm_vcpu_vector.h
> index 60fa280309444..06409c870a069 100644
> --- a/arch/riscv/include/asm/kvm_vcpu_vector.h
> +++ b/arch/riscv/include/asm/kvm_vcpu_vector.h
[ ... ]
> @@ -77,11 +76,7 @@ static inline void kvm_riscv_vcpu_guest_vector_restore(struct kvm_cpu_context *c
>  {
>  }
>  
> -static inline void kvm_riscv_vcpu_host_vector_save(struct kvm_cpu_context *cntx)
> -{
> -}
> -
> -static inline void kvm_riscv_vcpu_host_vector_restore(struct kvm_cpu_context *cntx)
> +static inline void kvm_riscv_vcpu_host_vector_restore(void)
>  {
>  }

[Severity: High]
Does building with CONFIG_KVM enabled and CONFIG_RISCV_ISA_V disabled fail in
kvm_riscv_vcpu_enter_exit()?

Earlier in the series, commit "riscv: vector: refactor vector context
operations" renamed __kvm_riscv_vector_restore() to
kvm_riscv_vector_restore() under #ifdef CONFIG_RISCV_ISA_V and at the call
site in kvm_riscv_vcpu_enter_exit(), which is compiled unconditionally, but
the !CONFIG_RISCV_ISA_V stub in arch/riscv/include/asm/kvm_vcpu_vector.h is
still named __kvm_riscv_vector_restore().

[ ... ]

> diff --git a/arch/riscv/kvm/vcpu_vector.c b/arch/riscv/kvm/vcpu_vector.c
> index 730c32f7de504..6129a6e184049 100644
> --- a/arch/riscv/kvm/vcpu_vector.c
> +++ b/arch/riscv/kvm/vcpu_vector.c
> @@ -61,17 +61,9 @@ void kvm_riscv_vcpu_guest_vector_restore(struct kvm_cpu_context *cntx,
>  	}
>  }
>  
> -void kvm_riscv_vcpu_host_vector_save(struct kvm_cpu_context *cntx)
> +void kvm_riscv_vcpu_host_vector_restore(void)
>  {
> -	/* No need to check host sstatus as it can be modified outside */
> -	if (!kvm_riscv_isa_check_host(V))
> -		kvm_riscv_vector_save(cntx);
> -}
> -
> -void kvm_riscv_vcpu_host_vector_restore(struct kvm_cpu_context *cntx)
> -{
> -	if (!kvm_riscv_isa_check_host(V))
> -		kvm_riscv_vector_restore(cntx);
> +	riscv_v_vstate_set_restore(current, task_pt_regs(current));
>  	riscv_v_flags_set(riscv_v_flags() & ~(RISCV_V_VCPU_CTX | RISCV_V_VCPU_NEED_RESTORE));
>  }

[Severity: High]
Can the guest's CSR_VCSR value leak into host userspace after exiting the
guest?

When the host task enters ioctl(KVM_RUN) via do_trap_ecall_u(),
riscv_v_vstate_discard() sets task_pt_regs(current)->status to SR_VS_INITIAL
(or leaves it at SR_VS_OFF if the task has not used vector instructions).
While running the vCPU, kvm_riscv_vcpu_enter_exit() ->
kvm_riscv_vector_restore() loads the guest's vcsr into hardware CSR_VCSR (or
CSR_VXRM and CSR_VXSAT on XTheadVector), and the guest can also modify it
directly.

On vCPU put, kvm_riscv_vcpu_host_vector_restore() now only schedules a
deferred restore via riscv_v_vstate_set_restore().  When returning to host
userspace with SR_VS_INITIAL, riscv_v_vstate_restore() calls
__riscv_v_vstate_discard(), which resets v0..v31, vl, vtype, and vstart, but
never writes CSR_VCSR:

arch/riscv/include/asm/vector.h:riscv_v_vstate_restore() {
	...
	riscv_v_enable();
	if (__riscv_v_vstate_check(regs->status, INITIAL))
		__riscv_v_vstate_discard();
	else if (__riscv_v_vstate_check(regs->status, CLEAN))
		__riscv_v_vstate_restore(vstate, vstate->datap);
	riscv_v_disable();
	...
}

This leaves the guest's CSR_VCSR active in hardware when host userspace
resumes (and across riscv_v_ucontext_save() on signal delivery or ptrace), or
for the next task that hits __riscv_v_vstate_discard() on that CPU if the VMM
task had SR_VS_OFF.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111184359.1239821-1-tchiu@tenstorrent.com?part=6