From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoffer Dall Subject: Re: [PATCH v4 08/19] arm64: KVM: Dynamically patch the kernel/hyp VA mask Date: Fri, 16 Feb 2018 10:02:42 +0100 Message-ID: <20180216090242.GB10440@cbox> References: <20180104184334.16571-1-marc.zyngier@arm.com> <20180104184334.16571-9-marc.zyngier@arm.com> <20180115114719.GI21403@cbox> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, Mark Rutland , Catalin Marinas , Will Deacon , James Morse , Steve Capper , Peter Maydell To: Marc Zyngier Return-path: Received: from mail-wm0-f66.google.com ([74.125.82.66]:34276 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757437AbeBPJCq (ORCPT ); Fri, 16 Feb 2018 04:02:46 -0500 Received: by mail-wm0-f66.google.com with SMTP id j21so4574836wmh.1 for ; Fri, 16 Feb 2018 01:02:46 -0800 (PST) Content-Disposition: inline In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Feb 15, 2018 at 01:11:02PM +0000, Marc Zyngier wrote: > On 15/01/18 11:47, Christoffer Dall wrote: > > 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 > >> --- > >> 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_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 000000000000..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 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 . > >> + */ > >> + > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> + > >> +#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?) > > I must has messed up with a later refactoring, as that code gets > replaced pretty quickly in the series. The full series was definitely > tested on Seattle with 39bit VAs, which is the only configuration I have > that triggers the low mask. > I realized later that this was just a temporary thing in the patch series. Still, for posterity, we should probably fix it up. > > > >> + > >> + 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... > > Makes patch splitting a bit easier. I can rework that if that helps. > No need, I get it now. > > > >> + 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" > > Sure. > > > > >> + 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.) > > That's indeed what is happening. A further patch expands the single > instruction to a 5 instruction sequence. I'll add a comment to that effect. > Thanks, -Christoffer From mboxrd@z Thu Jan 1 00:00:00 1970 From: christoffer.dall@linaro.org (Christoffer Dall) Date: Fri, 16 Feb 2018 10:02:42 +0100 Subject: [PATCH v4 08/19] arm64: KVM: Dynamically patch the kernel/hyp VA mask In-Reply-To: References: <20180104184334.16571-1-marc.zyngier@arm.com> <20180104184334.16571-9-marc.zyngier@arm.com> <20180115114719.GI21403@cbox> Message-ID: <20180216090242.GB10440@cbox> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thu, Feb 15, 2018 at 01:11:02PM +0000, Marc Zyngier wrote: > On 15/01/18 11:47, Christoffer Dall wrote: > > 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 > >> --- > >> 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_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 000000000000..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 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 . > >> + */ > >> + > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> + > >> +#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?) > > I must has messed up with a later refactoring, as that code gets > replaced pretty quickly in the series. The full series was definitely > tested on Seattle with 39bit VAs, which is the only configuration I have > that triggers the low mask. > I realized later that this was just a temporary thing in the patch series. Still, for posterity, we should probably fix it up. > > > >> + > >> + 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... > > Makes patch splitting a bit easier. I can rework that if that helps. > No need, I get it now. > > > >> + 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" > > Sure. > > > > >> + 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.) > > That's indeed what is happening. A further patch expands the single > instruction to a 5 instruction sequence. I'll add a comment to that effect. > Thanks, -Christoffer