On Mon, 2020-11-02 at 16:40 +0000, Marc Zyngier wrote:
[...]
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index dfb5218137ca..3f23f7478d2a 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -252,10 +252,32 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> memcpy(addr, valp, KVM_REG_SIZE(reg->id));
I was looking at KVM_(G|S)ET_ONE_REG implementations and something looks off to me here:
...
if (off == KVM_REG_ARM_CORE_REG(regs.pstate)) {
u64 mode = (*(u64 *)valp) & PSR_AA32_MODE_MASK;
switch (mode) {
Masking and switch over mode here...
case PSR_AA32_MODE_USR:
if (!kvm_supports_32bit_el0())
return -EINVAL;
break;
case PSR_AA32_MODE_FIQ:
case PSR_AA32_MODE_IRQ:
...
>
> if (*vcpu_cpsr(vcpu) & PSR_MODE32_BIT) {
> - int i;
> + int i, nr_reg;
> +
> + switch (*vcpu_cpsr(vcpu)) {
...but switching over mode without masking here.
I don't know if this is as intended, but I thought I'd mention it.
> + /*
> + * Either we are dealing with user mode, and only the
> + * first 15 registers (+ PC) must be narrowed to 32bit.
> + * AArch32 r0-r14 conveniently map to AArch64 x0-x14.
> + */
> + case PSR_AA32_MODE_USR:
> + case PSR_AA32_MODE_SYS:
> + nr_reg = 15;
> + break;
> +
> + /*
> + * Otherwide, this is a priviledged mode, and *all* the
> + * registers must be narrowed to 32bit.
> + */
> + default:
> + nr_reg = 31;
> + break;
> + }
> +
> + for (i = 0; i < nr_reg; i++)
> + vcpu_set_reg(vcpu, i, (u32)vcpu_get_reg(vcpu, i));
>
> - for (i = 0; i < 16; i++)
> - *vcpu_reg32(vcpu, i) = (u32)*vcpu_reg32(vcpu, i);
> + *vcpu_pc(vcpu) = (u32)*vcpu_pc(vcpu);
> }
> out:
> return err;
> diff --git a/arch/arm64/kvm/regmap.c b/arch/arm64/kvm/regmap.c
> deleted file mode 100644
> index ae7e290bb017..000000000000
> --- a/arch/arm64/kvm/regmap.c
> +++ /dev/null
> @@ -1,128 +0,0 @@
[...]
> -unsigned long *vcpu_reg32(const struct kvm_vcpu *vcpu, u8 reg_num)
> -{
> - unsigned long *reg_array = (unsigned long *)&vcpu->arch.ctxt.regs;
> - unsigned long mode = *vcpu_cpsr(vcpu) & PSR_AA32_MODE_MASK;
There used to be masking here at least.
> -
> - switch (mode) {
> - case PSR_AA32_MODE_USR ... PSR_AA32_MODE_SVC:
> - mode &= ~PSR_MODE32_BIT; /* 0 ... 3 */
> - break;
> -
> - case PSR_AA32_MODE_ABT:
> - mode = 4;
> - break;
> -
> - case PSR_AA32_MODE_UND:
> - mode = 5;
> - break;
> -
> - case PSR_AA32_MODE_SYS:
> - mode = 0; /* SYS maps to USR */
> - break;
> -
> - default:
> - BUG();
> - }
> -
> - return reg_array + vcpu_reg_offsets[mode][reg_num];
> -}
