Re: [PATCH 1/3] KVM: arm64: correct PSTATE on exception entry

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 12/20/19 3:05 PM, Mark Rutland wrote:
> When KVM injects an exception into a guest, it generates the PSTATE
> value from scratch, configuring PSTATE.{M[4:0],DAIF}, and setting all
> other bits to zero.
>
> This isn't correct, as the architecture specifies that some PSTATE bits
> are (conditionally) cleared or set upon an exception, and others are
> unchanged from the original context.
>
> This patch adds logic to match the architectural behaviour. To make this
> simple to follow/audit/extend, documentation references are provided,
> and bits are configured in order of their layout in SPSR_EL2. This
> layout can be seen in the diagram on ARM DDI 0487E.a page C5-429.
>
> Signed-off-by: Mark Rutland <mark.rutland@xxxxxxx>
> Cc: Alexandru Elisei <alexandru.elisei@xxxxxxx>
> Cc: Drew Jones <drjones@xxxxxxxxxx>
> Cc: James Morse <james.morse@xxxxxxx>
> Cc: Julien Thierry <julien.thierry.kdev@xxxxxxxxx>
> Cc: Marc Zyngier <maz@xxxxxxxxxx>
> Cc: Peter Maydell <peter.maydell@xxxxxxxxxx>
> Cc: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> Cc: Will Deacon <will@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> ---
>  arch/arm64/include/uapi/asm/ptrace.h |  1 +
>  arch/arm64/kvm/inject_fault.c        | 69 +++++++++++++++++++++++++++++++++---
>  2 files changed, 65 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm64/include/uapi/asm/ptrace.h b/arch/arm64/include/uapi/asm/ptrace.h
> index 7ed9294e2004..d1bb5b69f1ce 100644
> --- a/arch/arm64/include/uapi/asm/ptrace.h
> +++ b/arch/arm64/include/uapi/asm/ptrace.h
> @@ -49,6 +49,7 @@
>  #define PSR_SSBS_BIT	0x00001000
>  #define PSR_PAN_BIT	0x00400000
>  #define PSR_UAO_BIT	0x00800000
> +#define PSR_DIT_BIT	0x01000000
>  #define PSR_V_BIT	0x10000000
>  #define PSR_C_BIT	0x20000000
>  #define PSR_Z_BIT	0x40000000
> diff --git a/arch/arm64/kvm/inject_fault.c b/arch/arm64/kvm/inject_fault.c
> index a9d25a305af5..270d91c05246 100644
> --- a/arch/arm64/kvm/inject_fault.c
> +++ b/arch/arm64/kvm/inject_fault.c
> @@ -14,9 +14,6 @@
>  #include <asm/kvm_emulate.h>
>  #include <asm/esr.h>
>  
> -#define PSTATE_FAULT_BITS_64 	(PSR_MODE_EL1h | PSR_A_BIT | PSR_F_BIT | \
> -				 PSR_I_BIT | PSR_D_BIT)
> -
>  #define CURRENT_EL_SP_EL0_VECTOR	0x0
>  #define CURRENT_EL_SP_ELx_VECTOR	0x200
>  #define LOWER_EL_AArch64_VECTOR		0x400
> @@ -50,6 +47,68 @@ static u64 get_except_vector(struct kvm_vcpu *vcpu, enum exception_type type)
>  	return vcpu_read_sys_reg(vcpu, VBAR_EL1) + exc_offset + type;
>  }
>  
> +/*
> + * When an exception is taken, most PSTATE fields are left unchanged in the
> + * handler. However, some are explicitly overridden (e.g. M[4:0]). Luckily all
> + * of the inherited bits have the same position in the AArch64/AArch32 SPSR_ELx
> + * layouts, so we don't need to shuffle these for exceptions from AArch32 EL0.
> + *
> + * For the SPSR_ELx layout for AArch64, see ARM DDI 0487E.a page C5-429.
> + * For the SPSR_ELx layout for AArch32, see ARM DDI 0487E.a page C5-426.

The commit message mentions only the SPSR_ELx layout for AArch64.

> + *
> + * Here we manipulate the fields in order of the AArch64 SPSR_ELx layout, from
> + * MSB to LSB.
> + */
> +static unsigned long get_except64_pstate(struct kvm_vcpu *vcpu)
> +{
> +	unsigned long sctlr = vcpu_read_sys_reg(vcpu, SCTLR_EL1);
> +	unsigned long old, new;
> +
> +	old = *vcpu_cpsr(vcpu);
> +	new = 0;
> +
> +	new |= (old & PSR_N_BIT);
> +	new |= (old & PSR_Z_BIT);
> +	new |= (old & PSR_C_BIT);
> +	new |= (old & PSR_V_BIT);
> +
> +	// TODO: TCO (if/when ARMv8.5-MemTag is exposed to guests)
> +
> +	new |= (old & PSR_DIT_BIT);
> +
> +	// PSTATE.UAO is set to zero upon any exception to AArch64
> +	// See ARM DDI 0487E.a, page D5-2579.
> +
> +	// PSTATE.PAN is unchanged unless overridden by SCTLR_ELx.SPAN
> +	// See ARM DDI 0487E.a, page D5-2578.
> +	new |= (old & PSR_PAN_BIT);
> +	if (sctlr & SCTLR_EL1_SPAN)
> +		new |= PSR_PAN_BIT;

On page D13-3264, it is stated that the PAN bit is set unconditionally if
SCTLR_EL1.SPAN is clear, not set.

> +
> +	// PSTATE.SS is set to zero upon any exception to AArch64
> +	// See ARM DDI 0487E.a, page D2-2452.
> +
> +	// PSTATE.IL is set to zero upon any exception to AArch64
> +	// See ARM DDI 0487E.a, page D1-2306.
> +
> +	// PSTATE.SSBS is set to SCTLR_ELx.DSSBS upon any exception to AArch64
> +	// See ARM DDI 0487E.a, page D13-3258
> +	if (sctlr & SCTLR_ELx_DSSBS)
> +		new |= PSR_SSBS_BIT;
> +
> +	// PSTATE.BTYPE is set to zero upon any exception to AArch64
> +	// See ARM DDI 0487E.a, pages D1-2293 to D1-2294.
> +
> +	new |= PSR_D_BIT;
> +	new |= PSR_A_BIT;
> +	new |= PSR_I_BIT;
> +	new |= PSR_F_BIT;
> +
> +	new |= PSR_MODE_EL1h;
> +
> +	return new;
> +}
> +
>  static void inject_abt64(struct kvm_vcpu *vcpu, bool is_iabt, unsigned long addr)
>  {
>  	unsigned long cpsr = *vcpu_cpsr(vcpu);
> @@ -59,7 +118,7 @@ static void inject_abt64(struct kvm_vcpu *vcpu, bool is_iabt, unsigned long addr
>  	vcpu_write_elr_el1(vcpu, *vcpu_pc(vcpu));
>  	*vcpu_pc(vcpu) = get_except_vector(vcpu, except_type_sync);
>  
> -	*vcpu_cpsr(vcpu) = PSTATE_FAULT_BITS_64;
> +	*vcpu_cpsr(vcpu) = get_except64_pstate(vcpu);
>  	vcpu_write_spsr(vcpu, cpsr);
>  
>  	vcpu_write_sys_reg(vcpu, addr, FAR_EL1);
> @@ -94,7 +153,7 @@ static void inject_undef64(struct kvm_vcpu *vcpu)
>  	vcpu_write_elr_el1(vcpu, *vcpu_pc(vcpu));
>  	*vcpu_pc(vcpu) = get_except_vector(vcpu, except_type_sync);
>  
> -	*vcpu_cpsr(vcpu) = PSTATE_FAULT_BITS_64;
> +	*vcpu_cpsr(vcpu) = get_except64_pstate(vcpu);
>  	vcpu_write_spsr(vcpu, cpsr);
>  
>  	/*

I've also checked the ARM ARM pages mentioned in the comments, and the
references are correct. The SPSR_EL2 layouts for exceptions taken from AArch64,
respectively AArch32, states are compatible with the way we create the SPSR_EL2
that will be used for eret'ing to the guest, just like the comment says.

I have a suggestion. I think that in ARM ARM, shuffling things between sections
happens a lot less often than adding/removing things from one particular
section, so the pages referenced are more likely to change in later versions.
How about referencing the section instead of the exact page? Something like:
"This layout can be seen in the diagram on ARM DDI 0487E.a, section C5.2.18,
when an exception is taken from AArch64 state"?

Thanks,
Alex



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux