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

[Alexandru Elisei]

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

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?

Thanks,
Alex
>  
>  u64 vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, int reg);
>  void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, int reg);