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

[christoffer.dall@linaro.org (Christoffer Dall)]

On Thu, Jan 04, 2018 at 06:43:23PM +0000, 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 <marc.zyngier@arm.com>
> ---
>  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 <asm/alternative.h>
> @@ -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 <asm/mmu_context.h>
>  #include <asm/pgtable.h>
>  
> +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 000000000000..aee758574e61
> --- /dev/null
> +++ b/arch/arm64/kvm/va_layout.c
> @@ -0,0 +1,91 @@
> +/*
> + * Copyright (C) 2017 ARM Ltd.
> + * Author: Marc Zyngier <marc.zyngier@arm.com>
> + *
> + * 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 FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/kvm_host.h>
> +#include <asm/alternative.h>
> +#include <asm/debug-monitors.h>
> +#include <asm/insn.h>
> +#include <asm/kvm_mmu.h>
> +
> +#define HYP_PAGE_OFFSET_HIGH_MASK	((UL(1) << VA_BITS) - 1)
> +#define HYP_PAGE_OFFSET_LOW_MASK	((UL(1) << (VA_BITS - 1)) - 1)
> +
> +static u64 va_mask;
> +
> +static void compute_layout(void)
> +{
> +	phys_addr_t idmap_addr = __pa_symbol(__hyp_idmap_text_start);
> +	unsigned long mask = HYP_PAGE_OFFSET_HIGH_MASK;
> +
> +	/*
> +	 * Activate the lower HYP offset only if the idmap doesn't
> +	 * clash with it,
> +	 */

The commentary is a bit strange given the logic below...

> +	if (idmap_addr > HYP_PAGE_OFFSET_LOW_MASK)
> +		mask = HYP_PAGE_OFFSET_HIGH_MASK;

... was the initialization supposed to be LOW_MASK?

(and does this imply that this was never tested on a system that
actually used the low mask?)

> +
> +	va_mask = mask;
> +}
> +
> +static u32 compute_instruction(int n, u32 rd, u32 rn)
> +{
> +	u32 insn = AARCH64_BREAK_FAULT;
> +
> +	switch (n) {
> +	case 0:

hmmm, wonder why we need this n==0 check...

> +		insn = aarch64_insn_gen_logical_immediate(AARCH64_INSN_LOGIC_AND,
> +							  AARCH64_INSN_VARIANT_64BIT,
> +							  rn, rd, va_mask);
> +		break;
> +	}
> +
> +	return insn;
> +}
> +
> +void __init kvm_update_va_mask(struct alt_instr *alt,
> +			       __le32 *origptr, __le32 *updptr, int nr_inst)
> +{
> +	int i;
> +
> +	/* We only expect a 1 instruction sequence */

nit: wording is a bit strange, how about
"We only expect a single instruction in the alternative sequence"

> +	BUG_ON(nr_inst != 1);
> +
> +	if (!has_vhe() && !va_mask)
> +		compute_layout();
> +
> +	for (i = 0; i < nr_inst; i++) {

It's a bit funny to have a loop with the above BUG_ON.

(I'm beginning to wonder if a future patch expands on this single
instruction thing, in which case a hint in the commit message would have
been nice.)

> +		u32 rd, rn, insn, oinsn;
> +
> +		/*
> +		 * VHE doesn't need any address translation, let's NOP
> +		 * everything.
> +		 */
> +		if (has_vhe()) {
> +			updptr[i] = aarch64_insn_gen_nop();
> +			continue;
> +		}
> +
> +		oinsn = le32_to_cpu(origptr[i]);
> +		rd = aarch64_insn_decode_register(AARCH64_INSN_REGTYPE_RD, oinsn);
> +		rn = aarch64_insn_decode_register(AARCH64_INSN_REGTYPE_RN, oinsn);
> +
> +		insn = compute_instruction(i, rd, rn);
> +		BUG_ON(insn == AARCH64_BREAK_FAULT);
> +
> +		updptr[i] = cpu_to_le32(insn);
> +	}
> +}
> -- 
> 2.14.2
> 

Thanks,
-Christoffer