On Thu, Jan 04, 2018 at 06:43:25PM +0000, Marc Zyngier wrote:
> kvm_vgic_global_state is part of the read-only section, and is
> usually accessed using a PC-relative address generation (adrp + add).
>
> It is thus useless to use kern_hyp_va() on it, and actively problematic
> if kern_hyp_va() becomes non-idempotent. On the other hand, there is
> no way that the compiler is going to guarantee that such access is
> always be PC relative.
nit: is always be
>
> So let's bite the bullet and provide our own accessor.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> ---
> arch/arm/include/asm/kvm_hyp.h | 6 ++++++
> arch/arm64/include/asm/kvm_hyp.h | 9 +++++++++
> virt/kvm/arm/hyp/vgic-v2-sr.c | 4 ++--
> 3 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_hyp.h b/arch/arm/include/asm/kvm_hyp.h
> index ab20ffa8b9e7..1d42d0aa2feb 100644
> --- a/arch/arm/include/asm/kvm_hyp.h
> +++ b/arch/arm/include/asm/kvm_hyp.h
> @@ -26,6 +26,12 @@
>
> #define __hyp_text __section(.hyp.text) notrace
>
> +#define hyp_symbol_addr(s) \
> + ({ \
> + typeof(s) *addr = &(s); \
> + addr; \
> + })
> +
> #define __ACCESS_VFP(CRn) \
> "mrc", "mcr", __stringify(p10, 7, %0, CRn, cr0, 0), u32
>
> diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
> index 08d3bb66c8b7..a2d98c539023 100644
> --- a/arch/arm64/include/asm/kvm_hyp.h
> +++ b/arch/arm64/include/asm/kvm_hyp.h
> @@ -25,6 +25,15 @@
>
> #define __hyp_text __section(.hyp.text) notrace
>
> +#define hyp_symbol_addr(s) \
> + ({ \
> + typeof(s) *addr; \
> + asm volatile("adrp %0, %1\n" \
> + "add %0, %0, :lo12:%1\n" \
> + : "=r" (addr) : "S" (&s)); \
Can't we use adr_l here?
> + addr; \
> + })
> +
I don't fully appreciate the semantics of this macro going by its name
only. My understanding is that if you want to resolve a symbol to an
address which is mapped in hyp, then use this. Is this correct?
If so, can we add a small comment (because I can't come up with a better
name).
> #define read_sysreg_elx(r,nvh,vh) \
> ({ \
> u64 reg; \
> diff --git a/virt/kvm/arm/hyp/vgic-v2-sr.c b/virt/kvm/arm/hyp/vgic-v2-sr.c
> index d7fd46fe9efb..4573d0552af3 100644
> --- a/virt/kvm/arm/hyp/vgic-v2-sr.c
> +++ b/virt/kvm/arm/hyp/vgic-v2-sr.c
> @@ -25,7 +25,7 @@
> static void __hyp_text save_elrsr(struct kvm_vcpu *vcpu, void __iomem *base)
> {
> struct vgic_v2_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v2;
> - int nr_lr = (kern_hyp_va(&kvm_vgic_global_state))->nr_lr;
> + int nr_lr = hyp_symbol_addr(kvm_vgic_global_state)->nr_lr;
> u32 elrsr0, elrsr1;
>
> elrsr0 = readl_relaxed(base + GICH_ELRSR0);
> @@ -139,7 +139,7 @@ int __hyp_text __vgic_v2_perform_cpuif_access(struct kvm_vcpu *vcpu)
> return -1;
>
> rd = kvm_vcpu_dabt_get_rd(vcpu);
> - addr = kern_hyp_va((kern_hyp_va(&kvm_vgic_global_state))->vcpu_base_va);
> + addr = kern_hyp_va(hyp_symbol_addr(kvm_vgic_global_state)->vcpu_base_va);
> addr += fault_ipa - vgic->vgic_cpu_base;
>
> if (kvm_vcpu_dabt_iswrite(vcpu)) {
> --
> 2.14.2
>
Otherwise looks good.
Thanks,
-Christoffer
