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 <[email protected]> > --- > 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 <[email protected]> > + * > + * 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 _______________________________________________ kvmarm mailing list [email protected] https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
