Re: [PATCH 05/15] arm64: KVM: Refactor kern_hyp_va/hyp_kern_va to deal with multiple offsets

[Marc Zyngier <marc.zyngier@arm.com>]

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@arm.com>
>>> ---
>>>  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().

	M.
-- 
Jazz is not dead. It just smells funny...