Hi,
On 12/20/19 3:05 PM, Mark Rutland wrote:
> When KVM injects an exception into a guest, it generates the CPSR value
> from scratch, configuring CPSR.{M,A,I,T,E}, and setting all other
> bits to zero.
>
> This isn't correct, as the architecture specifies that some CPSR 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
This patch applies equally well to 32bit KVM, where we have a SPSR register.
> layout can be seen in the diagram on ARM DDI 0487E.a page C5-426.
>
> 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/arm/include/asm/kvm_emulate.h | 12 ++++
> arch/arm64/include/asm/ptrace.h | 1 +
> virt/kvm/arm/aarch32.c | 110 +++++++++++++++++++++++++++++++++----
> 3 files changed, 113 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> index 40002416efec..dee2567661ed 100644
> --- a/arch/arm/include/asm/kvm_emulate.h
> +++ b/arch/arm/include/asm/kvm_emulate.h
> @@ -14,13 +14,25 @@
> #include <asm/cputype.h>
>
> /* arm64 compatibility macros */
> +#define PSR_AA32_MODE_FIQ FIQ_MODE
> +#define PSR_AA32_MODE_SVC SVC_MODE
> #define PSR_AA32_MODE_ABT ABT_MODE
> #define PSR_AA32_MODE_UND UND_MODE
> #define PSR_AA32_T_BIT PSR_T_BIT
> +#define PSR_AA32_F_BIT PSR_F_BIT
> #define PSR_AA32_I_BIT PSR_I_BIT
> #define PSR_AA32_A_BIT PSR_A_BIT
> #define PSR_AA32_E_BIT PSR_E_BIT
> #define PSR_AA32_IT_MASK PSR_IT_MASK
> +#define PSR_AA32_GE_MASK 0x000f0000
> +#define PSR_AA32_PAN_BIT 0x00400000
> +#define PSR_AA32_SSBS_BIT 0x00800000
> +#define PSR_AA32_DIT_BIT 0x01000000
This is actually PSR_J_BIT on arm. Maybe it's worth defining it like that to
make the overlap obvious.
> +#define PSR_AA32_Q_BIT PSR_Q_BIT
> +#define PSR_AA32_V_BIT PSR_V_BIT
> +#define PSR_AA32_C_BIT PSR_C_BIT
> +#define PSR_AA32_Z_BIT PSR_Z_BIT
> +#define PSR_AA32_N_BIT PSR_N_BIT
>
> unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>
> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
> index fbebb411ae20..bf57308fcd63 100644
> --- a/arch/arm64/include/asm/ptrace.h
> +++ b/arch/arm64/include/asm/ptrace.h
> @@ -62,6 +62,7 @@
> #define PSR_AA32_I_BIT 0x00000080
> #define PSR_AA32_A_BIT 0x00000100
> #define PSR_AA32_E_BIT 0x00000200
> +#define PSR_AA32_PAN_BIT 0x00400000
> #define PSR_AA32_SSBS_BIT 0x00800000
> #define PSR_AA32_DIT_BIT 0x01000000
> #define PSR_AA32_Q_BIT 0x08000000
> diff --git a/virt/kvm/arm/aarch32.c b/virt/kvm/arm/aarch32.c
> index c4c57ba99e90..17bcde5c2451 100644
> --- a/virt/kvm/arm/aarch32.c
> +++ b/virt/kvm/arm/aarch32.c
> @@ -10,6 +10,7 @@
> * Author: Christoffer Dall <c.dall@xxxxxxxxxxxxxxxxxxxxxx>
> */
>
> +#include <linux/bits.h>
> #include <linux/kvm_host.h>
> #include <asm/kvm_emulate.h>
> #include <asm/kvm_hyp.h>
> @@ -28,22 +29,111 @@ static const u8 return_offsets[8][2] = {
> [7] = { 4, 4 }, /* FIQ, unused */
> };
>
> +/*
> + * When an exception is taken, most CPSR fields are left unchanged in the
> + * handler. However, some are explicitly overridden (e.g. M[4:0]).
> + *
> + * The SPSR/SPSR_ELx layouts differ, and the below is intended to work with
> + * either format. Note: SPSR.J bit doesn't exist in SPSR_ELx, but this bit was
> + * obsoleted by the ARMv7 virtualization extensions and is RES0.
> + *
> + * For the SPSR layout seen from AArch32, see:
> + * - ARM DDI 0406C.d, page B1-1148
> + * - ARM DDI 0487E.a, page G8-6264
> + *
> + * For the SPSR_ELx layout for AArch32 seen from AArch64, see:
> + * - ARM DDI 0487E.a, page C5-426
> + *
> + * Here we manipulate the fields in order of the AArch32 SPSR_ELx layout, from
> + * MSB to LSB.
> + */
> +static unsigned long get_except32_cpsr(struct kvm_vcpu *vcpu, u32 mode)
> +{
> + u32 sctlr = vcpu_cp15(vcpu, c1_SCTLR);
> + unsigned long old, new;
> +
> + old = *vcpu_cpsr(vcpu);
> + new = 0;
> +
> + new |= (old & PSR_AA32_N_BIT);
> + new |= (old & PSR_AA32_Z_BIT);
> + new |= (old & PSR_AA32_C_BIT);
> + new |= (old & PSR_AA32_V_BIT);
> + new |= (old & PSR_AA32_Q_BIT);
> +
> + // CPSR.IT[7:0] are set to zero upon any exception
> + // See ARM DDI 0487E.a, section G1.12.3
> + // See ARM DDI 0406C.d, section B1.8.3
> +
> + new |= (old & PSR_AA32_DIT_BIT);
> +
> + // CPSR.SSBS is set to SCTLR.DSSBS upon any exception
> + // See ARM DDI 0487E.a, page G8-6244
> + if (sctlr & BIT(31))
> + new |= PSR_AA32_SSBS_BIT;
> +
> + // CPSR.PAN is unchanged unless overridden by SCTLR.SPAN
> + // See ARM DDI 0487E.a, page G8-6246
> + new |= (old & PSR_AA32_PAN_BIT);
> + if (sctlr & BIT(23))
> + new |= PSR_AA32_PAN_BIT;
PSTATE.PAN is unconditionally set when SCTLR.SPAN is clear.
> +
> + // SS does not exist in AArch32, so ignore
> +
> + // CPSR.IL is set to zero upon any exception
> + // See ARM DDI 0487E.a, page G1-5527
> +
> + new |= (old & PSR_AA32_GE_MASK);
> +
> + // CPSR.IT[7:0] are set to zero upon any exception
> + // See prior comment above
> +
> + // CPSR.E is set to SCTLR.EE upon any exception
> + // See ARM DDI 0487E.a, page G8-6245
> + // See ARM DDI 0406C.d, page B4-1701
> + if (sctlr & BIT(25))
> + new |= PSR_AA32_E_BIT;
> +
> + // CPSR.A is unchanged upon an exception to Undefined, Supervisor
> + // CPSR.A is set upon an exception to other modes
> + // See ARM DDI 0487E.a, pages G1-5515 to G1-5516
> + // See ARM DDI 0406C.d, page B1-1182
> + new |= (old & PSR_AA32_A_BIT);
> + if (mode != PSR_AA32_MODE_UND && mode != PSR_AA32_MODE_SVC)
> + new |= PSR_AA32_A_BIT;
> +
> + // CPSR.I is set upon any exception
> + // See ARM DDI 0487E.a, pages G1-5515 to G1-5516
> + // See ARM DDI 0406C.d, page B1-1182
> + new |= PSR_AA32_I_BIT;
> +
> + // CPSR.F is set upon an exception to FIQ
> + // CPSR.F is unchanged upon an exception to other modes
> + // See ARM DDI 0487E.a, pages G1-5515 to G1-5516
> + // See ARM DDI 0406C.d, page B1-1182
> + new |= (old & PSR_AA32_F_BIT);
> + if (mode == PSR_AA32_MODE_FIQ)
> + new |= PSR_AA32_F_BIT;
> +
> + // CPSR.T is set to SCTLR.TE upon any exception
> + // See ARM DDI 0487E.a, page G8-5514
> + // See ARM DDI 0406C.d, page B1-1181
> + if (sctlr & BIT(30))
> + new |= PSR_AA32_T_BIT;
> +
> + new |= mode;
> +
> + return new;
> +}
> +
> static void prepare_fault32(struct kvm_vcpu *vcpu, u32 mode, u32 vect_offset)
> {
> - unsigned long cpsr;
> unsigned long new_spsr_value = *vcpu_cpsr(vcpu);
> bool is_thumb = (new_spsr_value & PSR_AA32_T_BIT);
> u32 return_offset = return_offsets[vect_offset >> 2][is_thumb];
> u32 sctlr = vcpu_cp15(vcpu, c1_SCTLR);
>
> - cpsr = mode | PSR_AA32_I_BIT;
> -
> - if (sctlr & (1 << 30))
> - cpsr |= PSR_AA32_T_BIT;
> - if (sctlr & (1 << 25))
> - cpsr |= PSR_AA32_E_BIT;
> -
> - *vcpu_cpsr(vcpu) = cpsr;
> + *vcpu_cpsr(vcpu) = get_except32_cpsr(vcpu, mode);
>
> /* Note: These now point to the banked copies */
> vcpu_write_spsr(vcpu, new_spsr_value);
> @@ -84,7 +174,7 @@ static void inject_abt32(struct kvm_vcpu *vcpu, bool is_pabt,
> fsr = &vcpu_cp15(vcpu, c5_DFSR);
> }
>
> - prepare_fault32(vcpu, PSR_AA32_MODE_ABT | PSR_AA32_A_BIT, vect_offset);
> + prepare_fault32(vcpu, PSR_AA32_MODE_ABT, vect_offset);
>
> *far = addr;
>
I've also checked that the ARM documentation references match the code.
Thanks,
Alex
