On Thu, Jun 30, 2016 at 11:16:44AM +0100, Marc Zyngier wrote:
> On 30/06/16 10:22, Marc Zyngier wrote:
> > On 28/06/16 13:42, Christoffer Dall wrote:
> >> On Tue, Jun 07, 2016 at 11:58:25AM +0100, Marc Zyngier wrote:
> >>> As we move towards a selectable HYP VA range, it is obvious that
> >>> we don't want to test a variable to find out if we need to use
> >>> the bottom VA range, the top VA range, or use the address as is
> >>> (for VHE).
> >>>
> >>> Instead, we can expand our current helpers to generate the right
> >>> mask or nop with code patching. We default to using the top VA
> >>> space, with alternatives to switch to the bottom one or to nop
> >>> out the instructions.
> >>>
> >>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> >>> ---
> >>> arch/arm64/include/asm/kvm_hyp.h | 27 ++++++++++++--------------
> >>> arch/arm64/include/asm/kvm_mmu.h | 42 +++++++++++++++++++++++++++++++++++++---
> >>> 2 files changed, 51 insertions(+), 18 deletions(-)
> >>>
> >>> diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
> >>> index 61d01a9..dd4904b 100644
> >>> --- a/arch/arm64/include/asm/kvm_hyp.h
> >>> +++ b/arch/arm64/include/asm/kvm_hyp.h
> >>> @@ -25,24 +25,21 @@
> >>>
> >>> #define __hyp_text __section(.hyp.text) notrace
> >>>
> >>> -static inline unsigned long __kern_hyp_va(unsigned long v)
> >>> -{
> >>> - asm volatile(ALTERNATIVE("and %0, %0, %1",
> >>> - "nop",
> >>> - ARM64_HAS_VIRT_HOST_EXTN)
> >>> - : "+r" (v) : "i" (HYP_PAGE_OFFSET_MASK));
> >>> - return v;
> >>> -}
> >>> -
> >>> -#define kern_hyp_va(v) (typeof(v))(__kern_hyp_va((unsigned long)(v)))
> >>> -
> >>> static inline unsigned long __hyp_kern_va(unsigned long v)
> >>> {
> >>> - asm volatile(ALTERNATIVE("orr %0, %0, %1",
> >>> - "nop",
> >>> + u64 mask;
> >>> +
> >>> + asm volatile(ALTERNATIVE("mov %0, %1",
> >>> + "mov %0, %2",
> >>> + ARM64_HYP_OFFSET_LOW)
> >>> + : "=r" (mask)
> >>> + : "i" (~HYP_PAGE_OFFSET_HIGH_MASK),
> >>> + "i" (~HYP_PAGE_OFFSET_LOW_MASK));
> >>> + asm volatile(ALTERNATIVE("nop",
> >>> + "mov %0, xzr",
> >>> ARM64_HAS_VIRT_HOST_EXTN)
> >>> - : "+r" (v) : "i" (~HYP_PAGE_OFFSET_MASK));
> >>> - return v;
> >>> + : "+r" (mask));
> >>> + return v | mask;
> >>
> >> If mask is ~HYP_PAGE_OFFSET_LOW_MASK how can you be sure that setting
> >> bit (VA_BITS - 1) is always the right thing to do to generate a kernel
> >> address?
> >
> > It has taken be a while, but I think I finally see what you mean. We
> > have no idea whether that bit was set or not.
> >
> >> This is kind of what I asked before only now there's an extra bit not
> >> guaranteed by the architecture to be set for the kernel range, I
> >> think.
> >
> > Yeah, I finally connected the couple of neurons left up there (that's
> > what remains after the whole brexit braindamage). This doesn't work (or
> > rather it only works sometimes). The good new is that I also realized we
> > don't need any of that crap.
> >
> > The only case we currently use a HVA->KVA transformation is to pass the
> > panic string down to panic(), and we can perfectly prevent
> > __kvm_hyp_teardown from ever be evaluated as a HVA with a bit of
> > asm-foo. This allows us to get rid of this whole function.
>
> Here's what I meant by this:
>
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index 437cfad..c19754d 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -299,9 +299,16 @@ static const char __hyp_panic_string[] = "HYP panic:\nPS:%08llx PC:%016llx ESR:%
>
> static void __hyp_text __hyp_call_panic_nvhe(u64 spsr, u64 elr, u64 par)
> {
> - unsigned long str_va = (unsigned long)__hyp_panic_string;
> + unsigned long str_va;
>
> - __hyp_do_panic(hyp_kern_va(str_va),
> + /*
> + * Force the panic string to be loaded from the literal pool,
> + * making sure it is a kernel address and not a PC-relative
> + * reference.
> + */
> + asm volatile("ldr %0, =__hyp_panic_string" : "=r" (str_va));
> +
> + __hyp_do_panic(str_va,
> spsr, elr,
> read_sysreg(esr_el2), read_sysreg_el2(far),
> read_sysreg(hpfar_el2), par,
>
> With that in place, we can entirely get rid of hyp_kern_va().
>
Looks good to me, there's really no need to get that string pointer via
a hyp address.
Thanks,
-Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
