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

[Miguel Luis]

> On 4 Jun 2025, at 18:58, Marc Zyngier <maz at kernel.org> wrote:
> 
> On Wed, 04 Jun 2025 16:53:01 +0100,
> Miguel Luis <miguel.luis at oracle.com> wrote:
>> 
>> 
>> 
>>> 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 ‘|’.
> 
> I have the opposite view. I think the '=' sign conveys an important
> meaning, and hiding it makes things less obvious. If I had lost common
> sense and was writing C++ , I would have simply overloaded the various
> operators, retaining the existing syntax. This macro does the hard job
> of applying the RESx masks, but the operator is what is semantically
> significant.
> 

That’s understandable. The Read-Modify-Write or ‘rmw’ on the accessor
reinforces the same important meaning as the ‘=‘ sign is conveying when
expressed on the accessor usage. Per se not a problem.

Thanks,
Miguel

> Thanks,
> 
> M.
> 
> -- 
> Jazz isn't dead. It just smells funny.