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

[Miguel Luis]

> On 4 Jun 2025, at 15:17, Marc Zyngier <maz at kernel.org> wrote:
> 
> On Wed, 04 Jun 2025 11:47:57 +0100,
> Miguel Luis <miguel.luis at oracle.com> wrote:
>> 
>> Hi Marc,
>> 
>>> On 3 Jun 2025, at 07:08, Marc Zyngier <maz at 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 at 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.

I believe this should give a clearer idea while avoids repeating the assignment everywhere
the accessor is used. Just replace ‘&=‘ and ‘|=‘ by ‘&’ and ‘|’.

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index b4ac2f515f94..bde1e31b4dee 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1121,7 +1121,7 @@ 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)) op##= (val);          \
                if (vcpu_has_nv((v)) && (r) >= __SANITISED_REG_START__) \
                        __v = kvm_vcpu_apply_reg_masks((v), (r), __v);  \
                                                                        \
Miguel

> 
> 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.