Re: [PATCH v2 06/17] KVM: arm64: Introduce accessor for ctxt->sys_reg

[Marc Zyngier <maz@kernel.org>]

Hi Alex,

On 2020-06-26 16:39, Alexandru Elisei wrote:
> Hi,
> 
> On 6/15/20 2:27 PM, Marc Zyngier wrote:
>> In order to allow the disintegration of the per-vcpu sysreg array,
>> let's introduce a new helper (ctxt_sys_reg()) that returns the
>> in-memory copy of a system register, picked from a given context.
>> 
>> __vcpu_sys_reg() is rewritten to use this helper.
>> 
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  arch/arm64/include/asm/kvm_host.h | 15 ++++++++++-----
>>  1 file changed, 10 insertions(+), 5 deletions(-)
>> 
>> diff --git a/arch/arm64/include/asm/kvm_host.h 
>> b/arch/arm64/include/asm/kvm_host.h
>> index e7fd03271e52..5314399944e7 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -405,12 +405,17 @@ struct kvm_vcpu_arch {
>>  #define vcpu_gp_regs(v)		(&(v)->arch.ctxt.gp_regs)
>> 
>>  /*
>> - * Only use __vcpu_sys_reg if you know you want the memory backed 
>> version of a
>> - * register, and not the one most recently accessed by a running 
>> VCPU.  For
>> - * example, for userspace access or for system registers that are 
>> never context
>> - * switched, but only emulated.
>> + * Only use __vcpu_sys_reg/ctxt_sys_reg if you know you want the
>> + * memory backed version of a register, and not the one most recently
>> + * accessed by a running VCPU.  For example, for userspace access or
>> + * for system registers that are never context switched, but only
>> + * emulated.
>>   */
>> -#define __vcpu_sys_reg(v,r)	((v)->arch.ctxt.sys_regs[(r)])
>> +#define __ctxt_sys_reg(c,r)	(&(c)->sys_regs[(r)])
>> +
>> +#define ctxt_sys_reg(c,r)	(*__ctxt_sys_reg(c,r))
>> +
>> +#define __vcpu_sys_reg(v,r)	(ctxt_sys_reg(&(v)->arch.ctxt, (r)))
> 
> This is confusing - __vcpu_sys_reg() returns the value, but 
> __ctxt_sys_reg()
> return a pointer to the value. Because of that, I made the mistake of 
> thinking
> that __vcpu_sys_reg() returns a pointer when reviewing the next patch 
> in the
> series, and I got really worried that stuff was seriously broken (it 
> was not).

This is intentional (the behaviour, not the confusing aspect... ;-), as
__ctx_sys_reg() gets further rewritten as such:

-#define __ctxt_sys_reg(c,r)	(&(c)->sys_regs[(r)])
+static inline u64 *__ctxt_sys_reg(const struct kvm_cpu_context *ctxt, 
int r)
+{
+	if (unlikely(r >= __VNCR_START__ && ctxt->vncr_array))
+		return &ctxt->vncr_array[r - __VNCR_START__];
+
+	return (u64 *)&ctxt->sys_regs[r];
+}

to deal with the VNCR page (depending on whether you use nesting or not,
the sysreg is backed by the VNCR page or the usual sysreg array).

To be clear, there shouldn't be much use of __ctxt_sys_reg (there is 
only
3 in the current code), all for good reasons (core_reg_addr definitely
wants the address of a register).

> I'm not sure what the reasonable solution is, or even if there is one.
> 
> Some thoughts: we could have just one macro, ctxt_sys_reg() and 
> dereference that
> when we want the value; we could keep both and swap the macro 
> definitions; or we
> could encode the fact that a macro returns a pointer in the macro name 
> (so we
> would end up with __ctxt_sys_reg() -> __ctxt_sys_regp() and 
> ctxt_sys_reg ->
> __ctxt_sys_reg()).
> 
> What do you think?

I'm not opposed to any of this, provided that it doesn't create
unnecessary churn and additional confusion. I'll keep it as such
in the meantime, but I'm definitely willing to take a patch going
over this if you think this is necessary.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm