[PATCH v2 22/36] KVM: arm64: Prepare to handle traps on deferred VM sysregs

[Marc Zyngier]

On 07/12/17 17:06, Christoffer Dall wrote:
> When we defer the save/restore of system registers to vcpu_load and
> vcpu_put, we need to take care of the emulation code that handles traps
> to these registers, since simply reading the memory array will return
> stale data.
> 
> Therefore, introduce two functions to directly read/write the registers
> from the physical CPU when we're on a VHE system that has loaded the
> system registers onto the physical CPU.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall at linaro.org>
> ---
> 
> Notes:
>     Changes since v1:
>      - Removed spurious white space
> 
>  arch/arm64/include/asm/kvm_host.h |  4 +++
>  arch/arm64/kvm/sys_regs.c         | 53 +++++++++++++++++++++++++++++++++++++--
>  2 files changed, 55 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index de0d55b30b61..f6afe685a280 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -279,6 +279,10 @@ struct kvm_vcpu_arch {
>  
>  	/* Detect first run of a vcpu */
>  	bool has_run_once;
> +
> +	/* True when deferrable sysregs are loaded on the physical CPU,
> +	 * see kvm_vcpu_load_sysregs and kvm_vcpu_put_sysregs. */
> +	bool sysregs_loaded_on_cpu;
>  };
>  
>  #define vcpu_gp_regs(v)		(&(v)->arch.ctxt.gp_regs)
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 62c12ab9e6c4..80adbec933de 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -35,6 +35,7 @@
>  #include <asm/kvm_coproc.h>
>  #include <asm/kvm_emulate.h>
>  #include <asm/kvm_host.h>
> +#include <asm/kvm_hyp.h>
>  #include <asm/kvm_mmu.h>
>  #include <asm/perf_event.h>
>  #include <asm/sysreg.h>
> @@ -111,6 +112,54 @@ static bool access_dcsw(struct kvm_vcpu *vcpu,
>  	return true;
>  }
>  
> +static u64 read_deferrable_vm_reg(struct kvm_vcpu *vcpu, int reg)
> +{
> +	if (vcpu->arch.sysregs_loaded_on_cpu) {
> +		switch (reg) {
> +		case SCTLR_EL1:		return read_sysreg_el1(sctlr);
> +		case TTBR0_EL1:		return read_sysreg_el1(ttbr0);
> +		case TTBR1_EL1:		return read_sysreg_el1(ttbr1);
> +		case TCR_EL1:		return read_sysreg_el1(tcr);
> +		case ESR_EL1:		return read_sysreg_el1(esr);
> +		case FAR_EL1:		return read_sysreg_el1(far);
> +		case AFSR0_EL1:		return read_sysreg_el1(afsr0);
> +		case AFSR1_EL1:		return read_sysreg_el1(afsr1);
> +		case MAIR_EL1:		return read_sysreg_el1(mair);
> +		case AMAIR_EL1:		return read_sysreg_el1(amair);
> +		case CONTEXTIDR_EL1:	return read_sysreg_el1(contextidr);
> +		case DACR32_EL2:	return read_sysreg(dacr32_el2);
> +		case IFSR32_EL2:	return read_sysreg(ifsr32_el2);
> +		default:		BUG();
> +		}
> +	}
> +
> +	return vcpu_sys_reg(vcpu, reg);
> +}
> +
> +static void write_deferrable_vm_reg(struct kvm_vcpu *vcpu, int reg, u64 val)
> +{
> +	if (vcpu->arch.sysregs_loaded_on_cpu) {
> +		switch (reg) {
> +		case SCTLR_EL1:		write_sysreg_el1(val, sctlr);	return;
> +		case TTBR0_EL1:		write_sysreg_el1(val, ttbr0);	return;
> +		case TTBR1_EL1:		write_sysreg_el1(val, ttbr1);	return;
> +		case TCR_EL1:		write_sysreg_el1(val, tcr);	return;
> +		case ESR_EL1:		write_sysreg_el1(val, esr);	return;
> +		case FAR_EL1:		write_sysreg_el1(val, far);	return;
> +		case AFSR0_EL1:		write_sysreg_el1(val, afsr0);	return;
> +		case AFSR1_EL1:		write_sysreg_el1(val, afsr1);	return;
> +		case MAIR_EL1:		write_sysreg_el1(val, mair);	return;
> +		case AMAIR_EL1:		write_sysreg_el1(val, amair);	return;
> +		case CONTEXTIDR_EL1:	write_sysreg_el1(val, contextidr); return;
> +		case DACR32_EL2:	write_sysreg(val, dacr32_el2); return;
> +		case IFSR32_EL2:	write_sysreg(val, ifsr32_el2); return;
> +		default:		BUG();
> +		}
> +	}
> +
> +	vcpu_sys_reg(vcpu, reg) = val;
> +}
> +
>  /*
>   * Generic accessor for VM registers. Only called as long as HCR_TVM
>   * is set. If the guest enables the MMU, we stop trapping the VM
> @@ -133,14 +182,14 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu,
>  	if (!p->is_aarch32 || !p->is_32bit) {
>  		val = p->regval;
>  	} else {
> -		val = vcpu_sys_reg(vcpu, reg);
> +		val = read_deferrable_vm_reg(vcpu, reg);
>  		if (r->reg % 2)
>  			val = (p->regval << 32) | (u64)lower_32_bits(val);
>  		else
>  			val = ((u64)upper_32_bits(val) << 32) |
>  				(u64)lower_32_bits(p->regval);
>  	}
> -	vcpu_sys_reg(vcpu, reg) = val;
> +	write_deferrable_vm_reg(vcpu, reg, val);
>  
>  	kvm_toggle_cache(vcpu, was_enabled);
>  	return true;
> 

I'm slightly uneasy with this. It means that the rest of the KVM code
has to know whether a given register is deferrable or not (or face the
wrath of the BUG). I'd be more inclined to hide the "loaded on cpu"
magic in the vcpu_sys_reg() accessors.

Thoughts?

	M.
-- 
Jazz is not dead. It just smells funny...