Re: [PATCH v6 04/26] arm64: KVM: Dynamically patch the kernel/hyp VA mask

[James Morse <james.morse@arm.com>]

Hi Marc,

On 14/03/18 16:50, 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
> to simplify that code by patching a single instruction with
> the correct mask (instead of the mind bending cummulative masking

(Nit: cumulative)

(so this series removes mind bending code?)

> we have at the moment) or even a single NOP on VHE. This also
> adds some initial code that will allow the patching callback
> to switch to a more complex patching.

> diff --git a/arch/arm64/kvm/va_layout.c b/arch/arm64/kvm/va_layout.c
> new file mode 100644
> index 000000000000..45e7802328d4
> --- /dev/null
> +++ b/arch/arm64/kvm/va_layout.c

> +void __init kvm_update_va_mask(struct alt_instr *alt,
> +			       __le32 *origptr, __le32 *updptr, int nr_inst)
> +{
> +	int i;
> +
> +	/* 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++) {
> +		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();

cpu_to_le32()? (I'm not going to try an boot a BE VHE model...)

aarch64_insn_gen_nop() returns:
| aarch64_insn_get_hint_value() | AARCH64_INSN_HINT_NOP;

It doesn't look like these aarch64_insn_get_XXX_value() helpers are forcing a
particular endianness. ftrace uses this, via ftrace_modify_code() ->
aarch64_insn_patch_text_nosync() -> aarch64_insn_write(), which does:
| return  __aarch64_insn_write(addr, cpu_to_le32(insn));

So it looks like the conversion is required. Patch 16 looks fine for this.

(and, I ran the teardown code on Juno big-endian...)


> +			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);
> +	}
> +}

With that,

Reviewed-by: James Morse <james.morse@arm.com>


Thanks,

James