by Ard Biesheuvel

[permalink] [raw]

Subject: Re: [RFC PATCH 6/6] kvm: arm64: Remove hyp_symbol_addr

On Thu, 19 Nov 2020 at 17:26, David Brazdil <[email protected]> wrote:
>
> The helper was used to force PC-relative addressing in hyp code because
> absolute addressing via constant-pools used to generate kernel VAs. This
> was cumbersome and required programmers to remember to use the helper
> whenever they wanted to take a pointer.
>
> Now that hyp relocations are fixed up, there is no need for the helper
> any logner. Remove it.
>
> Signed-off-by: David Brazdil <[email protected]>

Acked-by: Ard Biesheuvel <[email protected]>

> ---
> arch/arm64/include/asm/kvm_asm.h | 20 --------------------
> arch/arm64/kvm/hyp/include/hyp/switch.h | 4 ++--
> arch/arm64/kvm/hyp/nvhe/hyp-smp.c | 4 ++--
> arch/arm64/kvm/hyp/nvhe/psci-relay.c | 4 ++--
> arch/arm64/kvm/hyp/vgic-v2-cpuif-proxy.c | 2 +-
> 5 files changed, 7 insertions(+), 27 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index 1a86581e581e..1961d23c0c40 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -203,26 +203,6 @@ extern void __vgic_v3_init_lrs(void);
>
> extern u32 __kvm_get_mdcr_el2(void);
>
> -/*
> - * Obtain the PC-relative address of a kernel symbol
> - * s: symbol
> - *
> - * The goal of this macro is to return a symbol's address based on a
> - * PC-relative computation, as opposed to a loading the VA from a
> - * constant pool or something similar. This works well for HYP, as an
> - * absolute VA is guaranteed to be wrong. Only use this if trying to
> - * obtain the address of a symbol (i.e. not something you obtained by
> - * following a pointer).
> - */
> -#define hyp_symbol_addr(s) \
> - ({ \
> - typeof(s) *addr; \
> - asm("adrp %0, %1\n" \
> - "add %0, %0, :lo12:%1\n" \
> - : "=r" (addr) : "S" (&s)); \
> - addr; \
> - })
> -
> #define __KVM_EXTABLE(from, to) \
> " .pushsection __kvm_ex_table, \"a\"\n" \
> " .align 3\n" \
> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
> index 84473574c2e7..54f4860cd87c 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
> @@ -505,8 +505,8 @@ static inline void __kvm_unexpected_el2_exception(void)
> struct exception_table_entry *entry, *end;
> unsigned long elr_el2 = read_sysreg(elr_el2);
>
> - entry = hyp_symbol_addr(__start___kvm_ex_table);
> - end = hyp_symbol_addr(__stop___kvm_ex_table);
> + entry = &__start___kvm_ex_table;
> + end = &__stop___kvm_ex_table;
>
> while (entry < end) {
> addr = (unsigned long)&entry->insn + entry->insn;
> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-smp.c b/arch/arm64/kvm/hyp/nvhe/hyp-smp.c
> index ceb427aabb91..6870d9f3d4b7 100644
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-smp.c
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-smp.c
> @@ -33,8 +33,8 @@ unsigned long __hyp_per_cpu_offset(unsigned int cpu)
> if (cpu >= ARRAY_SIZE(kvm_arm_hyp_percpu_base))
> hyp_panic();
>
> - cpu_base_array = (unsigned long*)hyp_symbol_addr(kvm_arm_hyp_percpu_base);
> + cpu_base_array = (unsigned long*)(&kvm_arm_hyp_percpu_base[0]);
> this_cpu_base = kern_hyp_va(cpu_base_array[cpu]);
> - elf_base = (unsigned long)hyp_symbol_addr(__per_cpu_start);
> + elf_base = (unsigned long)&__per_cpu_start;
> return this_cpu_base - elf_base;
> }
> diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> index 313ef42f0eab..f64380a49a72 100644
> --- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> +++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> @@ -147,7 +147,7 @@ static int psci_cpu_suspend(u64 func_id, struct kvm_cpu_context *host_ctxt)
> * point if it is a deep sleep state.
> */
> ret = psci_call(func_id, power_state,
> - __hyp_pa(hyp_symbol_addr(__kvm_hyp_cpu_entry)),
> + __hyp_pa(__kvm_hyp_cpu_entry),
> __hyp_pa(cpu_params));
>
> release_reset_state(cpu_state);
> @@ -182,7 +182,7 @@ static int psci_cpu_on(u64 func_id, struct kvm_cpu_context *host_ctxt)
> return PSCI_RET_ALREADY_ON;
>
> ret = psci_call(func_id, mpidr,
> - __hyp_pa(hyp_symbol_addr(__kvm_hyp_cpu_entry)),
> + __hyp_pa(__kvm_hyp_cpu_entry),
> __hyp_pa(cpu_params));
>
> /*
> diff --git a/arch/arm64/kvm/hyp/vgic-v2-cpuif-proxy.c b/arch/arm64/kvm/hyp/vgic-v2-cpuif-proxy.c
> index 8f0585640241..87a54375bd6e 100644
> --- a/arch/arm64/kvm/hyp/vgic-v2-cpuif-proxy.c
> +++ b/arch/arm64/kvm/hyp/vgic-v2-cpuif-proxy.c
> @@ -64,7 +64,7 @@ int __vgic_v2_perform_cpuif_access(struct kvm_vcpu *vcpu)
> }
>
> rd = kvm_vcpu_dabt_get_rd(vcpu);
> - addr = hyp_symbol_addr(kvm_vgic_global_state)->vcpu_hyp_va;
> + addr = kvm_vgic_global_state.vcpu_hyp_va;
> addr += fault_ipa - vgic->vgic_cpu_base;
>
> if (kvm_vcpu_dabt_iswrite(vcpu)) {
> --
> 2.29.2.299.gdc1121823c-goog
>

2020-12-09 13:06:01

by David Brazdil

[permalink] [raw]

Subject: Re: [RFC PATCH 5/6] kvm: arm64: Fix constant-pool users in hyp

Hey, relized I never replied to this...

On Tue, Nov 24, 2020 at 03:08:20PM +0100, Ard Biesheuvel wrote:
> On Thu, 19 Nov 2020 at 17:26, David Brazdil <[email protected]> wrote:
> >
> > Hyp code used to use absolute addressing via a constant pool to obtain
> > the kernel VA of 3 symbols - panic, __hyp_panic_string and
> > __kvm_handle_stub_hvc. This used to work because the kernel would
> > relocate the addresses in the constant pool to kernel VA at boot and hyp
> > would simply load them from there.
> >
> > Now that relocations are fixed up to point to hyp VAs, this does not
> > work any longer. Rework the helpers to convert hyp VA to kernel VA / PA
> > as needed.
> >
>
> Ok, so the reason for the problem is that the literal exists inside
> the HYP text, and all literals are fixed up using the HYP mapping,
> even if they don't point to something that is mapped at HYP. Would it
> make sense to simply disregard literals that point outside of the HYP
> VA mapping?

That would be possible - I'm about to post a v1 with build-time generation of
these relocs and kernel symbols are easily recognizable as they're undefined
in that ELF object. But I do worry that that would confuse people a lot.
And if somebody referenced a kernel symbol in C (maybe not a use case, but...)
they would get different results depending on which addressing mode the
compiler picked.

>
> > Signed-off-by: David Brazdil <[email protected]>
> > ---
> > arch/arm64/include/asm/kvm_mmu.h | 29 +++++++++++++++++++----------
> > arch/arm64/kvm/hyp/nvhe/host.S | 29 +++++++++++++++--------------
> > 2 files changed, 34 insertions(+), 24 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> > index 8cb8974ec9cc..0676ff2105bb 100644
> > --- a/arch/arm64/include/asm/kvm_mmu.h
> > +++ b/arch/arm64/include/asm/kvm_mmu.h
> > @@ -72,9 +72,14 @@ alternative_cb kvm_update_va_mask
> > alternative_cb_end
> > .endm
> >
> > +.macro hyp_pa reg, tmp
> > + ldr_l \tmp, hyp_physvirt_offset
> > + add \reg, \reg, \tmp
> > +.endm
> > +
> > /*
> > - * Convert a kernel image address to a PA
> > - * reg: kernel address to be converted in place
> > + * Convert a hypervisor VA to a kernel image address
> > + * reg: hypervisor address to be converted in place
> > * tmp: temporary register
> > *
> > * The actual code generation takes place in kvm_get_kimage_voffset, and
> > @@ -82,18 +87,22 @@ alternative_cb_end
> > * perform the register allocation (kvm_get_kimage_voffset uses the
> > * specific registers encoded in the instructions).
> > */
> > -.macro kimg_pa reg, tmp
> > +.macro hyp_kimg reg, tmp
> > + /* Convert hyp VA -> PA. */
> > + hyp_pa \reg, \tmp
> > +
> > + /* Load kimage_voffset. */
> > alternative_cb kvm_get_kimage_voffset
> > - movz \tmp, #0
> > - movk \tmp, #0, lsl #16
> > - movk \tmp, #0, lsl #32
> > - movk \tmp, #0, lsl #48
> > + movz \tmp, #0
> > + movk \tmp, #0, lsl #16
> > + movk \tmp, #0, lsl #32
> > + movk \tmp, #0, lsl #48
> > alternative_cb_end
> >
> > - /* reg = __pa(reg) */
> > - sub \reg, \reg, \tmp
> > + /* Convert PA -> kimg VA. */
> > + add \reg, \reg, \tmp
> > .endm
> > -
> > +
> > #else
> >
> > #include <linux/pgtable.h>
> > diff --git a/arch/arm64/kvm/hyp/nvhe/host.S b/arch/arm64/kvm/hyp/nvhe/host.S
> > index 596dd5ae8e77..bcb80d525d8c 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/host.S
> > +++ b/arch/arm64/kvm/hyp/nvhe/host.S
> > @@ -74,27 +74,28 @@ SYM_FUNC_END(__host_enter)
> > * void __noreturn __hyp_do_panic(bool restore_host, u64 spsr, u64 elr, u64 par);
> > */
> > SYM_FUNC_START(__hyp_do_panic)
> > - /* Load the format arguments into x1-7 */
> > - mov x6, x3
> > - get_vcpu_ptr x7, x3
> > -
> > - mrs x3, esr_el2
> > - mrs x4, far_el2
> > - mrs x5, hpfar_el2
> > -
> > /* Prepare and exit to the host's panic funciton. */
> > mov lr, #(PSR_F_BIT | PSR_I_BIT | PSR_A_BIT | PSR_D_BIT |\
> > PSR_MODE_EL1h)
> > msr spsr_el2, lr
> > ldr lr, =panic
> > + hyp_kimg lr, x6
> > msr elr_el2, lr
> >
> > - /*
> > - * Set the panic format string and enter the host, conditionally
> > - * restoring the host context.
> > - */
> > + /* Set the panic format string. Use the, now free, LR as scratch. */
> > + ldr lr, =__hyp_panic_string
> > + hyp_kimg lr, x6
> > +
> > + /* Load the format arguments into x1-7. */
> > + mov x6, x3
> > + get_vcpu_ptr x7, x3
> > + mrs x3, esr_el2
> > + mrs x4, far_el2
> > + mrs x5, hpfar_el2
> > +
> > + /* Enter the host, conditionally restoring the host context. */
> > cmp x0, xzr
> > - ldr x0, =__hyp_panic_string
> > + mov x0, lr
> > b.eq __host_enter_without_restoring
> > b __host_enter_for_panic
> > SYM_FUNC_END(__hyp_do_panic)
> > @@ -124,7 +125,7 @@ SYM_FUNC_END(__hyp_do_panic)
> > * Preserve x0-x4, which may contain stub parameters.
> > */
> > ldr x5, =__kvm_handle_stub_hvc
> > - kimg_pa x5, x6
> > + hyp_pa x5, x6
> > br x5
> > .L__vect_end\@:
> > .if ((.L__vect_end\@ - .L__vect_start\@) > 0x80)
> > --
> > 2.29.2.299.gdc1121823c-goog
> >