Re: [PATCH v2 0/4] KVM: arm64: vcpu sysreg accessor rework

[Marc Zyngier <maz@kernel.org>]

On Wed, 04 Jun 2025 11:47:57 +0100,
Miguel Luis <miguel.luis@oracle.com> wrote:
> 
> Hi Marc,
> 
> > On 3 Jun 2025, at 07:08, Marc Zyngier <maz@kernel.org> wrote:
> > 
> > This series tries to bring some sanity to the way the RESx masks
> > are applied when accessing the in-memory view of the guest's
> > system registers.
> > 
> > Currently, we have *one* accessor (__vcpu_sys_reg()) that can either
> > be used as a rvalue or lvalue while that applies the RESx masks behind
> > the scenes. This works fine when used as a rvalue.
> > 
> > However, when used as a lvalue, it does the wrong thing, as it only
> > sanitises the value we're about to overwrite. This is pointless work
> > and potentially hides bugs.
> > 
> > I propose that we move to a set of store-specific accessors (for
> > assignments and RMW) instead of the lvalue hack, ensuring that the
> > assigned value is the one that gets sanitised. This then allows the 
> > legacy accessor to be converted to rvalue-only.
> > 
> > Given the level of churn this introduces, I'd like this to land very
> > early in the cycle. Either before 6.16-rc2, or early in 6.17.
> > 
> 
> For the series:
> Reviewed-by: Miguel Luis <miguel.luis@oracle.com>

Thanks.

> 
> nit: the rmw accessor implies an implicit assignment which could be specified
> within its macro instead but it's fine by me.

I'm not sure what you mean. Looking at this macro again, the early
writeback to the sysreg array is pretty unfortunate, and could be
avoided.

Does the following change address your concern? If not, please clearly
point out what you're after.

	M.

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index b4ac2f515f94..382b382d14da 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1121,7 +1121,8 @@ u64 kvm_vcpu_apply_reg_masks(const struct kvm_vcpu *, enum vcpu_sysreg, u64);
 #define __vcpu_rmw_sys_reg(v, r, op, val)				\
 	do {								\
 		const struct kvm_cpu_context *ctxt = &(v)->arch.ctxt;	\
-		u64 __v = ctxt_sys_reg(ctxt, (r)) op (val);		\
+		u64 __v = ctxt_sys_reg(ctxt, (r));			\
+		__v op (val);						\
 		if (vcpu_has_nv((v)) && (r) >= __SANITISED_REG_START__)	\
 			__v = kvm_vcpu_apply_reg_masks((v), (r), __v);	\
 									\

-- 
Jazz isn't dead. It just smells funny.