Re: [PATCH 01/12] KVM: arm/arm64: vgic: Remove spurious call to kern_hyp_va

Suzuki K Poulose
2017-12-04T14:27:25Z

On 04/12/17 14:13, Steve Capper wrote:
> In save_elrsr(.), we use the following technique to ascertain the
> address of the vgic global state:
> (kern_hyp_va(&kvm_vgic_global_state))->nr_lr
>
> For arm, kern_hyp_va(va) == va, and this call effectively compiles out.
>
> For arm64, this call can be spurious as the address of kvm_vgic_global_state
> will usually be determined by relative page/absolute page offset relocation
> at link time. As the function is idempotent, having the call for arm64 does
> not cause any problems.
>
> Unfortunately, this is about to change for arm64 as we need to change
> the logic of kern_hyp_va to allow for kernel addresses that are outside
> the direct linear map.
>
> This patch removes the call to kern_hyp_va, and ensures that correct
> HYP addresses are computed via relative page offset addressing on arm64.
> This is achieved by a custom accessor, hyp_address(.), which on arm is a
> simple reference operator.

minor nit: I somehow feel that there word "symbol" should be part of the name of
the macro, to make it implicit that it can only be used on a symbol and not any
generic variable.

> diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
> index 08d3bb66c8b7..34a4ae906a97 100644
> --- a/arch/arm64/include/asm/kvm_hyp.h
> +++ b/arch/arm64/include/asm/kvm_hyp.h
> @@ -25,6 +25,16 @@
>
> #define __hyp_text __section(.hyp.text) notrace
>
> +#define hyp_address(symbol) \
> +({ \
> + typeof(&symbol) __ret; \
> + asm volatile( \
> + "adrp %[ptr], " #symbol "\n" \
> + "add %[ptr], %[ptr], :lo12:" #symbol "\n" \
> + : [ptr] "=r"(__ret)); \
> + __ret; \
> +})
> +

> - addr = kern_hyp_va((kern_hyp_va(&kvm_vgic_global_state))->vcpu_base_va);
> + addr = kern_hyp_va(hyp_address(kvm_vgic_global_state)->vcpu_base_va);

e.g, Like here, why do we use hyp_address only for the kvm_vgic_global_state and not
the dereferenced value. Having a name, say, hyp_symbol_address() makes it clear.

Otherwise, looks good to me.

Suzuki