Re: [PATCH v4 08/19] arm64: KVM: Dynamically patch the kernel/hyp VA mask

2018-02-16 Thread Christoffer Dall 
On Thu, Feb 15, 2018 at 01:11:02PM +, Marc Zyngier wrote:
> On 15/01/18 11:47, Christoffer Dall wrote:
> > On Thu, Jan 04, 2018 at 06:43:23PM +, Marc Zyngier wrote:
> >> So far, we're using a complicated sequence of alternatives to
> >> patch the kernel/hyp VA mask on non-VHE, and NOP out the
> >> masking altogether when on VHE.
> >>
> >> THe newly introduced dynamic patching gives us the opportunity
> > 
> > nit: s/THe/The/
> > 
> >> to simplify that code by patching a single instruction with
> >> the correct mask (instead of the mind bending cummulative masking
> >> we have at the moment) or even a single NOP on VHE.
> >>
> >> Signed-off-by: Marc Zyngier 
> >> ---
> >>  arch/arm64/include/asm/kvm_mmu.h | 45 ++--
> >>  arch/arm64/kvm/Makefile  |  2 +-
> >>  arch/arm64/kvm/va_layout.c   | 91 
> >> 
> >>  3 files changed, 104 insertions(+), 34 deletions(-)
> >>  create mode 100644 arch/arm64/kvm/va_layout.c
> >>
> >> diff --git a/arch/arm64/include/asm/kvm_mmu.h 
> >> b/arch/arm64/include/asm/kvm_mmu.h
> >> index 672c8684d5c2..b0c3cbe9b513 100644
> >> --- a/arch/arm64/include/asm/kvm_mmu.h
> >> +++ b/arch/arm64/include/asm/kvm_mmu.h
> >> @@ -69,9 +69,6 @@
> >>   * mappings, and none of this applies in that case.
> >>   */
> >>  
> >> -#define HYP_PAGE_OFFSET_HIGH_MASK ((UL(1) << VA_BITS) - 1)
> >> -#define HYP_PAGE_OFFSET_LOW_MASK  ((UL(1) << (VA_BITS - 1)) - 1)
> >> -
> >>  #ifdef __ASSEMBLY__
> >>  
> >>  #include 
> >> @@ -81,28 +78,14 @@
> >>   * Convert a kernel VA into a HYP VA.
> >>   * reg: VA to be converted.
> >>   *
> >> - * This generates the following sequences:
> >> - * - High mask:
> >> - *and x0, x0, #HYP_PAGE_OFFSET_HIGH_MASK
> >> - *nop
> >> - * - Low mask:
> >> - *and x0, x0, #HYP_PAGE_OFFSET_HIGH_MASK
> >> - *and x0, x0, #HYP_PAGE_OFFSET_LOW_MASK
> >> - * - VHE:
> >> - *nop
> >> - *nop
> >> - *
> >> - * The "low mask" version works because the mask is a strict subset of
> >> - * the "high mask", hence performing the first mask for nothing.
> >> - * Should be completely invisible on any viable CPU.
> >> + * The actual code generation takes place in kvm_update_va_mask, and
> >> + * the instructions below are only there to reserve the space and
> >> + * perform the register allocation.
> > 
> > Not just register allocation, but also to tell the generating code which
> > registers to use for its operands, right?
> 
> That's what I meant by register allocation.
> 

I suppose that's included in the term.  My confusion was that I
initially looked at this like the clobber list in inline asm, but then
realized that you really use the specific registers for each instruction
in the order listed here.

> > 
> >>   */
> >>  .macro kern_hyp_vareg
> >> -alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
> >> -  and \reg, \reg, #HYP_PAGE_OFFSET_HIGH_MASK
> >> -alternative_else_nop_endif
> >> -alternative_if ARM64_HYP_OFFSET_LOW
> >> -  and \reg, \reg, #HYP_PAGE_OFFSET_LOW_MASK
> >> -alternative_else_nop_endif
> >> +alternative_cb kvm_update_va_mask
> >> +  and \reg, \reg, #1
> >> +alternative_cb_end
> >>  .endm
> >>  
> >>  #else
> >> @@ -113,18 +96,14 @@ alternative_else_nop_endif
> >>  #include 
> >>  #include 
> >>  
> >> +void kvm_update_va_mask(struct alt_instr *alt,
> >> +  __le32 *origptr, __le32 *updptr, int nr_inst);
> >> +
> >>  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_HIGH_MASK));
> >> -  asm volatile(ALTERNATIVE("nop",
> >> -   "and %0, %0, %1",
> >> -   ARM64_HYP_OFFSET_LOW)
> >> -   : "+r" (v)
> >> -   : "i" (HYP_PAGE_OFFSET_LOW_MASK));
> >> +  asm volatile(ALTERNATIVE_CB("and %0, %0, #1\n",
> >> +  kvm_update_va_mask)
> >> +   : "+r" (v));
> >>return v;
> >>  }
> >>  
> >> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> >> index 87c4f7ae24de..93afff91cb7c 100644
> >> --- a/arch/arm64/kvm/Makefile
> >> +++ b/arch/arm64/kvm/Makefile
> >> @@ -16,7 +16,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/kvm_main.o 
> >> $(KVM)/coalesced_mmio.o $(KVM)/e
> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arm.o $(KVM)/arm/mmu.o 
> >> $(KVM)/arm/mmio.o
> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/psci.o $(KVM)/arm/perf.o
> >>  
> >> -kvm-$(CONFIG_KVM_ARM_HOST) += inject_fault.o regmap.o
> >> +kvm-$(CONFIG_KVM_ARM_HOST) += inject_fault.o regmap.o va_layout.o
> >>  kvm-$(CONFIG_KVM_ARM_HOST) += hyp.o hyp-init.o handle_exit.o
> >>  kvm-$(CONFIG_KVM_ARM_HOST) += guest.o debug.o reset.o sys_regs.o 
> >>

Re: [PATCH v4 08/19] arm64: KVM: Dynamically patch the kernel/hyp VA mask

2018-02-15 Thread Marc Zyngier 
On 15/01/18 11:47, Christoffer Dall wrote:
> On Thu, Jan 04, 2018 at 06:43:23PM +, Marc Zyngier wrote:
>> So far, we're using a complicated sequence of alternatives to
>> patch the kernel/hyp VA mask on non-VHE, and NOP out the
>> masking altogether when on VHE.
>>
>> THe newly introduced dynamic patching gives us the opportunity
> 
> nit: s/THe/The/
> 
>> to simplify that code by patching a single instruction with
>> the correct mask (instead of the mind bending cummulative masking
>> we have at the moment) or even a single NOP on VHE.
>>
>> Signed-off-by: Marc Zyngier 
>> ---
>>  arch/arm64/include/asm/kvm_mmu.h | 45 ++--
>>  arch/arm64/kvm/Makefile  |  2 +-
>>  arch/arm64/kvm/va_layout.c   | 91 
>> 
>>  3 files changed, 104 insertions(+), 34 deletions(-)
>>  create mode 100644 arch/arm64/kvm/va_layout.c
>>
>> diff --git a/arch/arm64/include/asm/kvm_mmu.h 
>> b/arch/arm64/include/asm/kvm_mmu.h
>> index 672c8684d5c2..b0c3cbe9b513 100644
>> --- a/arch/arm64/include/asm/kvm_mmu.h
>> +++ b/arch/arm64/include/asm/kvm_mmu.h
>> @@ -69,9 +69,6 @@
>>   * mappings, and none of this applies in that case.
>>   */
>>  
>> -#define HYP_PAGE_OFFSET_HIGH_MASK   ((UL(1) << VA_BITS) - 1)
>> -#define HYP_PAGE_OFFSET_LOW_MASK((UL(1) << (VA_BITS - 1)) - 1)
>> -
>>  #ifdef __ASSEMBLY__
>>  
>>  #include 
>> @@ -81,28 +78,14 @@
>>   * Convert a kernel VA into a HYP VA.
>>   * reg: VA to be converted.
>>   *
>> - * This generates the following sequences:
>> - * - High mask:
>> - *  and x0, x0, #HYP_PAGE_OFFSET_HIGH_MASK
>> - *  nop
>> - * - Low mask:
>> - *  and x0, x0, #HYP_PAGE_OFFSET_HIGH_MASK
>> - *  and x0, x0, #HYP_PAGE_OFFSET_LOW_MASK
>> - * - VHE:
>> - *  nop
>> - *  nop
>> - *
>> - * The "low mask" version works because the mask is a strict subset of
>> - * the "high mask", hence performing the first mask for nothing.
>> - * Should be completely invisible on any viable CPU.
>> + * The actual code generation takes place in kvm_update_va_mask, and
>> + * the instructions below are only there to reserve the space and
>> + * perform the register allocation.
> 
> Not just register allocation, but also to tell the generating code which
> registers to use for its operands, right?

That's what I meant by register allocation.

> 
>>   */
>>  .macro kern_hyp_va  reg
>> -alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
>> -and \reg, \reg, #HYP_PAGE_OFFSET_HIGH_MASK
>> -alternative_else_nop_endif
>> -alternative_if ARM64_HYP_OFFSET_LOW
>> -and \reg, \reg, #HYP_PAGE_OFFSET_LOW_MASK
>> -alternative_else_nop_endif
>> +alternative_cb kvm_update_va_mask
>> +and \reg, \reg, #1
>> +alternative_cb_end
>>  .endm
>>  
>>  #else
>> @@ -113,18 +96,14 @@ alternative_else_nop_endif
>>  #include 
>>  #include 
>>  
>> +void kvm_update_va_mask(struct alt_instr *alt,
>> +__le32 *origptr, __le32 *updptr, int nr_inst);
>> +
>>  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_HIGH_MASK));
>> -asm volatile(ALTERNATIVE("nop",
>> - "and %0, %0, %1",
>> - ARM64_HYP_OFFSET_LOW)
>> - : "+r" (v)
>> - : "i" (HYP_PAGE_OFFSET_LOW_MASK));
>> +asm volatile(ALTERNATIVE_CB("and %0, %0, #1\n",
>> +kvm_update_va_mask)
>> + : "+r" (v));
>>  return v;
>>  }
>>  
>> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
>> index 87c4f7ae24de..93afff91cb7c 100644
>> --- a/arch/arm64/kvm/Makefile
>> +++ b/arch/arm64/kvm/Makefile
>> @@ -16,7 +16,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/kvm_main.o 
>> $(KVM)/coalesced_mmio.o $(KVM)/e
>>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arm.o $(KVM)/arm/mmu.o 
>> $(KVM)/arm/mmio.o
>>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/psci.o $(KVM)/arm/perf.o
>>  
>> -kvm-$(CONFIG_KVM_ARM_HOST) += inject_fault.o regmap.o
>> +kvm-$(CONFIG_KVM_ARM_HOST) += inject_fault.o regmap.o va_layout.o
>>  kvm-$(CONFIG_KVM_ARM_HOST) += hyp.o hyp-init.o handle_exit.o
>>  kvm-$(CONFIG_KVM_ARM_HOST) += guest.o debug.o reset.o sys_regs.o 
>> sys_regs_generic_v8.o
>>  kvm-$(CONFIG_KVM_ARM_HOST) += vgic-sys-reg-v3.o
>> diff --git a/arch/arm64/kvm/va_layout.c b/arch/arm64/kvm/va_layout.c
>> new file mode 100644
>> index ..aee758574e61
>> --- /dev/null
>> +++ b/arch/arm64/kvm/va_layout.c
>> @@ -0,0 +1,91 @@
>> +/*
>> + * Copyright (C) 2017 ARM Ltd.
>> + * Author: Marc Zyngier 
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the

Re: [PATCH v4 08/19] arm64: KVM: Dynamically patch the kernel/hyp VA mask

2018-01-15 Thread Christoffer Dall 
On Thu, Jan 04, 2018 at 06:43:23PM +, Marc Zyngier wrote:
> So far, we're using a complicated sequence of alternatives to
> patch the kernel/hyp VA mask on non-VHE, and NOP out the
> masking altogether when on VHE.
> 
> THe newly introduced dynamic patching gives us the opportunity

nit: s/THe/The/

> to simplify that code by patching a single instruction with
> the correct mask (instead of the mind bending cummulative masking
> we have at the moment) or even a single NOP on VHE.
> 
> Signed-off-by: Marc Zyngier 
> ---
>  arch/arm64/include/asm/kvm_mmu.h | 45 ++--
>  arch/arm64/kvm/Makefile  |  2 +-
>  arch/arm64/kvm/va_layout.c   | 91 
> 
>  3 files changed, 104 insertions(+), 34 deletions(-)
>  create mode 100644 arch/arm64/kvm/va_layout.c
> 
> diff --git a/arch/arm64/include/asm/kvm_mmu.h 
> b/arch/arm64/include/asm/kvm_mmu.h
> index 672c8684d5c2..b0c3cbe9b513 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -69,9 +69,6 @@
>   * mappings, and none of this applies in that case.
>   */
>  
> -#define HYP_PAGE_OFFSET_HIGH_MASK((UL(1) << VA_BITS) - 1)
> -#define HYP_PAGE_OFFSET_LOW_MASK ((UL(1) << (VA_BITS - 1)) - 1)
> -
>  #ifdef __ASSEMBLY__
>  
>  #include 
> @@ -81,28 +78,14 @@
>   * Convert a kernel VA into a HYP VA.
>   * reg: VA to be converted.
>   *
> - * This generates the following sequences:
> - * - High mask:
> - *   and x0, x0, #HYP_PAGE_OFFSET_HIGH_MASK
> - *   nop
> - * - Low mask:
> - *   and x0, x0, #HYP_PAGE_OFFSET_HIGH_MASK
> - *   and x0, x0, #HYP_PAGE_OFFSET_LOW_MASK
> - * - VHE:
> - *   nop
> - *   nop
> - *
> - * The "low mask" version works because the mask is a strict subset of
> - * the "high mask", hence performing the first mask for nothing.
> - * Should be completely invisible on any viable CPU.
> + * The actual code generation takes place in kvm_update_va_mask, and
> + * the instructions below are only there to reserve the space and
> + * perform the register allocation.

Not just register allocation, but also to tell the generating code which
registers to use for its operands, right?

>   */
>  .macro kern_hyp_va   reg
> -alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
> - and \reg, \reg, #HYP_PAGE_OFFSET_HIGH_MASK
> -alternative_else_nop_endif
> -alternative_if ARM64_HYP_OFFSET_LOW
> - and \reg, \reg, #HYP_PAGE_OFFSET_LOW_MASK
> -alternative_else_nop_endif
> +alternative_cb kvm_update_va_mask
> + and \reg, \reg, #1
> +alternative_cb_end
>  .endm
>  
>  #else
> @@ -113,18 +96,14 @@ alternative_else_nop_endif
>  #include 
>  #include 
>  
> +void kvm_update_va_mask(struct alt_instr *alt,
> + __le32 *origptr, __le32 *updptr, int nr_inst);
> +
>  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_HIGH_MASK));
> - asm volatile(ALTERNATIVE("nop",
> -  "and %0, %0, %1",
> -  ARM64_HYP_OFFSET_LOW)
> -  : "+r" (v)
> -  : "i" (HYP_PAGE_OFFSET_LOW_MASK));
> + asm volatile(ALTERNATIVE_CB("and %0, %0, #1\n",
> + kvm_update_va_mask)
> +  : "+r" (v));
>   return v;
>  }
>  
> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> index 87c4f7ae24de..93afff91cb7c 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -16,7 +16,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/kvm_main.o 
> $(KVM)/coalesced_mmio.o $(KVM)/e
>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arm.o $(KVM)/arm/mmu.o 
> $(KVM)/arm/mmio.o
>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/psci.o $(KVM)/arm/perf.o
>  
> -kvm-$(CONFIG_KVM_ARM_HOST) += inject_fault.o regmap.o
> +kvm-$(CONFIG_KVM_ARM_HOST) += inject_fault.o regmap.o va_layout.o
>  kvm-$(CONFIG_KVM_ARM_HOST) += hyp.o hyp-init.o handle_exit.o
>  kvm-$(CONFIG_KVM_ARM_HOST) += guest.o debug.o reset.o sys_regs.o 
> sys_regs_generic_v8.o
>  kvm-$(CONFIG_KVM_ARM_HOST) += vgic-sys-reg-v3.o
> diff --git a/arch/arm64/kvm/va_layout.c b/arch/arm64/kvm/va_layout.c
> new file mode 100644
> index ..aee758574e61
> --- /dev/null
> +++ b/arch/arm64/kvm/va_layout.c
> @@ -0,0 +1,91 @@
> +/*
> + * Copyright (C) 2017 ARM Ltd.
> + * Author: Marc Zyngier 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or