Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1323103 > unrolled thread

Re: [PATCH v2 07/21] arm64: KVM: VHE: Patch out kern_hyp_va

Started byChristoffer Dall <christoffer.dall@linaro.org>
First post2016-02-01 14:30 +0100
Last post2016-02-01 16:50 +0100
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 07/21] arm64: KVM: VHE: Patch out kern_hyp_va Christoffer Dall <christoffer.dall@linaro.org> - 2016-02-01 14:30 +0100
    Re: [PATCH v2 07/21] arm64: KVM: VHE: Patch out kern_hyp_va Marc Zyngier <marc.zyngier@arm.com> - 2016-02-01 14:40 +0100
      Re: [PATCH v2 07/21] arm64: KVM: VHE: Patch out kern_hyp_va Christoffer Dall <christoffer.dall@linaro.org> - 2016-02-01 16:50 +0100

#1323103 — Re: [PATCH v2 07/21] arm64: KVM: VHE: Patch out kern_hyp_va

FromChristoffer Dall <christoffer.dall@linaro.org>
Date2016-02-01 14:30 +0100
SubjectRe: [PATCH v2 07/21] arm64: KVM: VHE: Patch out kern_hyp_va
Message-ID<qXmIi-3oo-17@gated-at.bofh.it>
On Mon, Jan 25, 2016 at 03:53:41PM +0000, Marc Zyngier wrote:
> The kern_hyp_va macro is pretty meaninless with VHE, as there is
> only one mapping - the kernel one.
> 
> In order to keep the code readable and efficient, use runtime
> patching to replace the 'and' instruction used to compute the VA
> with a 'nop'.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/include/asm/kvm_mmu.h | 11 ++++++++++-
>  arch/arm64/kvm/hyp/hyp.h         | 25 ++++++++++++++++++++++---
>  2 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> index d3e6d7b..62f0d14 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -23,13 +23,16 @@
>  #include <asm/cpufeature.h>
>  
>  /*
> - * As we only have the TTBR0_EL2 register, we cannot express
> + * As ARMv8.0 only has the TTBR0_EL2 register, we cannot express
>   * "negative" addresses. This makes it impossible to directly share
>   * mappings with the kernel.
>   *
>   * Instead, give the HYP mode its own VA region at a fixed offset from
>   * the kernel by just masking the top bits (which are all ones for a
>   * kernel address).
> + *
> + * ARMv8.1 (using VHE) does have a TTBR1_EL2, and doesn't use these
> + * macros (the entire kernel runs at EL2).
>   */
>  #define HYP_PAGE_OFFSET_SHIFT	VA_BITS
>  #define HYP_PAGE_OFFSET_MASK	((UL(1) << HYP_PAGE_OFFSET_SHIFT) - 1)
> @@ -56,6 +59,8 @@
>  
>  #ifdef __ASSEMBLY__
>  
> +#include <asm/alternative.h>
> +#include <asm/cpufeature.h>
>  #include <asm/kvm_arm.h>
>  
>  .macro setup_vtcr tmp1, tmp2
> @@ -84,7 +89,11 @@
>   * reg: VA to be converted.
>   */
>  .macro kern_hyp_va	reg
> +alternative_if_not ARM64_HAS_VIRT_HOST_EXTN	
>  	and	\reg, \reg, #HYP_PAGE_OFFSET_MASK
> +alternative_else
> +	nop
> +alternative_endif
>  .endm
>  
>  #else
> diff --git a/arch/arm64/kvm/hyp/hyp.h b/arch/arm64/kvm/hyp/hyp.h
> index fb27517..fc502f3 100644
> --- a/arch/arm64/kvm/hyp/hyp.h
> +++ b/arch/arm64/kvm/hyp/hyp.h
> @@ -25,9 +25,28 @@
>  
>  #define __hyp_text __section(.hyp.text) notrace
>  
> -#define kern_hyp_va(v) (typeof(v))((unsigned long)(v) & HYP_PAGE_OFFSET_MASK)
> -#define hyp_kern_va(v) (typeof(v))((unsigned long)(v) - HYP_PAGE_OFFSET \
> -						      + PAGE_OFFSET)
> +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)
> +{
> +	u64 offset = PAGE_OFFSET - HYP_PAGE_OFFSET;
> +	asm volatile(ALTERNATIVE("add %0, %0, %1",
> +				 "nop",
> +				 ARM64_HAS_VIRT_HOST_EXTN)
> +		     : "+r" (v) : "r" (offset));
> +	return v;
> +}
> +
> +#define hyp_kern_va(v) (typeof(v))(__hyp_kern_va((unsigned long)(v)))

why do we need this casting of values instead of just defining these
inlines and calling them directly with proper typing?

-Christoffer

>  
>  /**
>   * hyp_alternate_select - Generates patchable code sequences that are
> -- 
> 2.1.4
>

[toc] | [next] | [standalone]

#1323115

FromMarc Zyngier <marc.zyngier@arm.com>
Date2016-02-01 14:40 +0100
Message-ID<qXmRY-3vq-25@gated-at.bofh.it>
In reply to#1323103
On 01/02/16 13:20, Christoffer Dall wrote:
> On Mon, Jan 25, 2016 at 03:53:41PM +0000, Marc Zyngier wrote:
>> The kern_hyp_va macro is pretty meaninless with VHE, as there is
>> only one mapping - the kernel one.
>>
>> In order to keep the code readable and efficient, use runtime
>> patching to replace the 'and' instruction used to compute the VA
>> with a 'nop'.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm64/include/asm/kvm_mmu.h | 11 ++++++++++-
>>  arch/arm64/kvm/hyp/hyp.h         | 25 ++++++++++++++++++++++---
>>  2 files changed, 32 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
>> index d3e6d7b..62f0d14 100644
>> --- a/arch/arm64/include/asm/kvm_mmu.h
>> +++ b/arch/arm64/include/asm/kvm_mmu.h
>> @@ -23,13 +23,16 @@
>>  #include <asm/cpufeature.h>
>>  
>>  /*
>> - * As we only have the TTBR0_EL2 register, we cannot express
>> + * As ARMv8.0 only has the TTBR0_EL2 register, we cannot express
>>   * "negative" addresses. This makes it impossible to directly share
>>   * mappings with the kernel.
>>   *
>>   * Instead, give the HYP mode its own VA region at a fixed offset from
>>   * the kernel by just masking the top bits (which are all ones for a
>>   * kernel address).
>> + *
>> + * ARMv8.1 (using VHE) does have a TTBR1_EL2, and doesn't use these
>> + * macros (the entire kernel runs at EL2).
>>   */
>>  #define HYP_PAGE_OFFSET_SHIFT	VA_BITS
>>  #define HYP_PAGE_OFFSET_MASK	((UL(1) << HYP_PAGE_OFFSET_SHIFT) - 1)
>> @@ -56,6 +59,8 @@
>>  
>>  #ifdef __ASSEMBLY__
>>  
>> +#include <asm/alternative.h>
>> +#include <asm/cpufeature.h>
>>  #include <asm/kvm_arm.h>
>>  
>>  .macro setup_vtcr tmp1, tmp2
>> @@ -84,7 +89,11 @@
>>   * reg: VA to be converted.
>>   */
>>  .macro kern_hyp_va	reg
>> +alternative_if_not ARM64_HAS_VIRT_HOST_EXTN	
>>  	and	\reg, \reg, #HYP_PAGE_OFFSET_MASK
>> +alternative_else
>> +	nop
>> +alternative_endif
>>  .endm
>>  
>>  #else
>> diff --git a/arch/arm64/kvm/hyp/hyp.h b/arch/arm64/kvm/hyp/hyp.h
>> index fb27517..fc502f3 100644
>> --- a/arch/arm64/kvm/hyp/hyp.h
>> +++ b/arch/arm64/kvm/hyp/hyp.h
>> @@ -25,9 +25,28 @@
>>  
>>  #define __hyp_text __section(.hyp.text) notrace
>>  
>> -#define kern_hyp_va(v) (typeof(v))((unsigned long)(v) & HYP_PAGE_OFFSET_MASK)
>> -#define hyp_kern_va(v) (typeof(v))((unsigned long)(v) - HYP_PAGE_OFFSET \
>> -						      + PAGE_OFFSET)
>> +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)
>> +{
>> +	u64 offset = PAGE_OFFSET - HYP_PAGE_OFFSET;
>> +	asm volatile(ALTERNATIVE("add %0, %0, %1",
>> +				 "nop",
>> +				 ARM64_HAS_VIRT_HOST_EXTN)
>> +		     : "+r" (v) : "r" (offset));
>> +	return v;
>> +}
>> +
>> +#define hyp_kern_va(v) (typeof(v))(__hyp_kern_va((unsigned long)(v)))
> 
> why do we need this casting of values instead of just defining these
> inlines and calling them directly with proper typing?

We commonly pass both pointers and unsigned long to this helper. Do you
really want a separate helper for each type instead of one that does it all?

Thanks,

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

[toc] | [prev] | [next] | [standalone]

#1323253

FromChristoffer Dall <christoffer.dall@linaro.org>
Date2016-02-01 16:50 +0100
Message-ID<qXoTO-4WL-37@gated-at.bofh.it>
In reply to#1323115
On Mon, Feb 01, 2016 at 01:38:57PM +0000, Marc Zyngier wrote:
> On 01/02/16 13:20, Christoffer Dall wrote:
> > On Mon, Jan 25, 2016 at 03:53:41PM +0000, Marc Zyngier wrote:
> >> The kern_hyp_va macro is pretty meaninless with VHE, as there is
> >> only one mapping - the kernel one.
> >>
> >> In order to keep the code readable and efficient, use runtime
> >> patching to replace the 'and' instruction used to compute the VA
> >> with a 'nop'.
> >>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> >>  arch/arm64/include/asm/kvm_mmu.h | 11 ++++++++++-
> >>  arch/arm64/kvm/hyp/hyp.h         | 25 ++++++++++++++++++++++---
> >>  2 files changed, 32 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> >> index d3e6d7b..62f0d14 100644
> >> --- a/arch/arm64/include/asm/kvm_mmu.h
> >> +++ b/arch/arm64/include/asm/kvm_mmu.h
> >> @@ -23,13 +23,16 @@
> >>  #include <asm/cpufeature.h>
> >>  
> >>  /*
> >> - * As we only have the TTBR0_EL2 register, we cannot express
> >> + * As ARMv8.0 only has the TTBR0_EL2 register, we cannot express
> >>   * "negative" addresses. This makes it impossible to directly share
> >>   * mappings with the kernel.
> >>   *
> >>   * Instead, give the HYP mode its own VA region at a fixed offset from
> >>   * the kernel by just masking the top bits (which are all ones for a
> >>   * kernel address).
> >> + *
> >> + * ARMv8.1 (using VHE) does have a TTBR1_EL2, and doesn't use these
> >> + * macros (the entire kernel runs at EL2).
> >>   */
> >>  #define HYP_PAGE_OFFSET_SHIFT	VA_BITS
> >>  #define HYP_PAGE_OFFSET_MASK	((UL(1) << HYP_PAGE_OFFSET_SHIFT) - 1)
> >> @@ -56,6 +59,8 @@
> >>  
> >>  #ifdef __ASSEMBLY__
> >>  
> >> +#include <asm/alternative.h>
> >> +#include <asm/cpufeature.h>
> >>  #include <asm/kvm_arm.h>
> >>  
> >>  .macro setup_vtcr tmp1, tmp2
> >> @@ -84,7 +89,11 @@
> >>   * reg: VA to be converted.
> >>   */
> >>  .macro kern_hyp_va	reg
> >> +alternative_if_not ARM64_HAS_VIRT_HOST_EXTN	
> >>  	and	\reg, \reg, #HYP_PAGE_OFFSET_MASK
> >> +alternative_else
> >> +	nop
> >> +alternative_endif
> >>  .endm
> >>  
> >>  #else
> >> diff --git a/arch/arm64/kvm/hyp/hyp.h b/arch/arm64/kvm/hyp/hyp.h
> >> index fb27517..fc502f3 100644
> >> --- a/arch/arm64/kvm/hyp/hyp.h
> >> +++ b/arch/arm64/kvm/hyp/hyp.h
> >> @@ -25,9 +25,28 @@
> >>  
> >>  #define __hyp_text __section(.hyp.text) notrace
> >>  
> >> -#define kern_hyp_va(v) (typeof(v))((unsigned long)(v) & HYP_PAGE_OFFSET_MASK)
> >> -#define hyp_kern_va(v) (typeof(v))((unsigned long)(v) - HYP_PAGE_OFFSET \
> >> -						      + PAGE_OFFSET)
> >> +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)
> >> +{
> >> +	u64 offset = PAGE_OFFSET - HYP_PAGE_OFFSET;
> >> +	asm volatile(ALTERNATIVE("add %0, %0, %1",
> >> +				 "nop",
> >> +				 ARM64_HAS_VIRT_HOST_EXTN)
> >> +		     : "+r" (v) : "r" (offset));
> >> +	return v;
> >> +}
> >> +
> >> +#define hyp_kern_va(v) (typeof(v))(__hyp_kern_va((unsigned long)(v)))
> > 
> > why do we need this casting of values instead of just defining these
> > inlines and calling them directly with proper typing?
> 
> We commonly pass both pointers and unsigned long to this helper. Do you
> really want a separate helper for each type instead of one that does it all?
> 

no, what I was suggesting was to always take a specific type and change
the callers that don't match to use a specific cast so you give the
compiler a chance to scream at you when writing new code.

But I don't feel strongly about it.

Thanks,
-Christoffer

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web