From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ard Biesheuvel Subject: Re: [PATCH 05/15] arm64: KVM: Refactor kern_hyp_va/hyp_kern_va to deal with multiple offsets Date: Thu, 30 Jun 2016 12:42:09 +0200 Message-ID: References: <1465297115-13091-1-git-send-email-marc.zyngier@arm.com> <1465297115-13091-6-git-send-email-marc.zyngier@arm.com> <20160628124210.GP26498@cbox> <5774E4CE.8020200@arm.com> <5774F18C.7030603@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Cc: Christoffer Dall , "linux-arm-kernel@lists.infradead.org" , KVM devel mailing list , "kvmarm@lists.cs.columbia.edu" To: Marc Zyngier Return-path: Received: from mail-it0-f53.google.com ([209.85.214.53]:38891 "EHLO mail-it0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751601AbcF3KmL (ORCPT ); Thu, 30 Jun 2016 06:42:11 -0400 Received: by mail-it0-f53.google.com with SMTP id h190so67464265ith.1 for ; Thu, 30 Jun 2016 03:42:10 -0700 (PDT) In-Reply-To: <5774F18C.7030603@arm.com> Sender: kvm-owner@vger.kernel.org List-ID: On 30 June 2016 at 12:16, 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 >>>> --- >>>> 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)); > + Wouldn't it suffice to make __hyp_panic_string a non-static pointer to const char? That way, it will be statically initialized with a kernel VA, and the external linkage forces the compiler to evaluate its value at runtime. > + __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(). > > M. > -- > Jazz is not dead. It just smells funny... > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel From mboxrd@z Thu Jan 1 00:00:00 1970 From: ard.biesheuvel@linaro.org (Ard Biesheuvel) Date: Thu, 30 Jun 2016 12:42:09 +0200 Subject: [PATCH 05/15] arm64: KVM: Refactor kern_hyp_va/hyp_kern_va to deal with multiple offsets In-Reply-To: <5774F18C.7030603@arm.com> References: <1465297115-13091-1-git-send-email-marc.zyngier@arm.com> <1465297115-13091-6-git-send-email-marc.zyngier@arm.com> <20160628124210.GP26498@cbox> <5774E4CE.8020200@arm.com> <5774F18C.7030603@arm.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 30 June 2016 at 12:16, 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 >>>> --- >>>> 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)); > + Wouldn't it suffice to make __hyp_panic_string a non-static pointer to const char? That way, it will be statically initialized with a kernel VA, and the external linkage forces the compiler to evaluate its value at runtime. > + __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(). > > M. > -- > Jazz is not dead. It just smells funny... > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel