| Message ID | 1447669698-15939-2-git-send-email-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Marc, On 16/11/15 10:28, Marc Zyngier wrote: > When running a 32bit guest under a 64bit hypervisor, the ARMv8 > architecture defines a mapping of the 32bit registers in the 64bit > space. This includes banked registers that are being demultiplexed > over the 64bit ones. > > On exception caused by an operation involving a 32bit register, the > HW exposes the register number in the ESR_EL2 register. It was so > far understood that SW had to compute which register was AArch64 > register was used (based on the current AArch32 mode and register > number). > > It turns out that I misinterpreted the ARM ARM, and the clue is in > D1.20.1: "For some exceptions, the exception syndrome given in the > ESR_ELx identifies one or more register numbers from the issued > instruction that generated the exception. Where the exception is > taken from an Exception level using AArch32 these register numbers > give the AArch64 view of the register." > > Which means that the HW is already giving us the translated version, > and that we shouldn't try to interpret it at all (for example, doing > an MMIO operation from the IRQ mode using the LR register leads to > very unexpected behaviours). > > The fix is thus not to perform a call to vcpu_reg32() at all from > vcpu_reg(), and use whatever register number is supplied directly. > The only case we need to find out about the mapping is when we > actively generate a register access, which only occurs when injecting > a fault in a guest. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > arch/arm64/include/asm/kvm_emulate.h | 8 +++++--- > arch/arm64/kvm/inject_fault.c | 2 +- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h > index 17e92f0..3ca894e 100644 > --- a/arch/arm64/include/asm/kvm_emulate.h > +++ b/arch/arm64/include/asm/kvm_emulate.h > @@ -99,11 +99,13 @@ static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu) > *vcpu_cpsr(vcpu) |= COMPAT_PSR_T_BIT; > } > > +/* > + * vcpu_reg should always be passed a register number coming from a > + * read of ESR_EL2. Otherwise, it may give the wrong result on AArch32 > + * with banked registers. > + */ > static inline unsigned long *vcpu_reg(const struct kvm_vcpu *vcpu, u8 reg_num) > { > - if (vcpu_mode_is_32bit(vcpu)) > - return vcpu_reg32(vcpu, reg_num); > - > return (unsigned long *)&vcpu_gp_regs(vcpu)->regs.regs[reg_num]; > } > > diff --git a/arch/arm64/kvm/inject_fault.c b/arch/arm64/kvm/inject_fault.c > index 85c5715..648112e 100644 > --- a/arch/arm64/kvm/inject_fault.c > +++ b/arch/arm64/kvm/inject_fault.c > @@ -48,7 +48,7 @@ static void prepare_fault32(struct kvm_vcpu *vcpu, u32 mode, u32 vect_offset) > > /* Note: These now point to the banked copies */ > *vcpu_spsr(vcpu) = new_spsr_value; > - *vcpu_reg(vcpu, 14) = *vcpu_pc(vcpu) + return_offset; > + *vcpu_reg32(vcpu, 14) = *vcpu_pc(vcpu) + return_offset; To the best of my knowledge after picking through all the uses of vcpu_reg, particularly in the shared 32-bit code, this does seem to be the only one which involves a potentially-banked register number that didn't originally come from an ESR read, and thus needs translation. Reviewed-by: Robin Murphy <robin.murphy@arm.com> (unfortunately I don't have an actual test-case as it was already a third-hand report when I started trying to look into it). Thanks for picking this up, Robin. > > /* Branch to exception vector */ > if (sctlr & (1 << 13)) > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h index 17e92f0..3ca894e 100644 --- a/arch/arm64/include/asm/kvm_emulate.h +++ b/arch/arm64/include/asm/kvm_emulate.h @@ -99,11 +99,13 @@ static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu) *vcpu_cpsr(vcpu) |= COMPAT_PSR_T_BIT; } +/* + * vcpu_reg should always be passed a register number coming from a + * read of ESR_EL2. Otherwise, it may give the wrong result on AArch32 + * with banked registers. + */ static inline unsigned long *vcpu_reg(const struct kvm_vcpu *vcpu, u8 reg_num) { - if (vcpu_mode_is_32bit(vcpu)) - return vcpu_reg32(vcpu, reg_num); - return (unsigned long *)&vcpu_gp_regs(vcpu)->regs.regs[reg_num]; } diff --git a/arch/arm64/kvm/inject_fault.c b/arch/arm64/kvm/inject_fault.c index 85c5715..648112e 100644 --- a/arch/arm64/kvm/inject_fault.c +++ b/arch/arm64/kvm/inject_fault.c @@ -48,7 +48,7 @@ static void prepare_fault32(struct kvm_vcpu *vcpu, u32 mode, u32 vect_offset) /* Note: These now point to the banked copies */ *vcpu_spsr(vcpu) = new_spsr_value; - *vcpu_reg(vcpu, 14) = *vcpu_pc(vcpu) + return_offset; + *vcpu_reg32(vcpu, 14) = *vcpu_pc(vcpu) + return_offset; /* Branch to exception vector */ if (sctlr & (1 << 13))
When running a 32bit guest under a 64bit hypervisor, the ARMv8 architecture defines a mapping of the 32bit registers in the 64bit space. This includes banked registers that are being demultiplexed over the 64bit ones. On exception caused by an operation involving a 32bit register, the HW exposes the register number in the ESR_EL2 register. It was so far understood that SW had to compute which register was AArch64 register was used (based on the current AArch32 mode and register number). It turns out that I misinterpreted the ARM ARM, and the clue is in D1.20.1: "For some exceptions, the exception syndrome given in the ESR_ELx identifies one or more register numbers from the issued instruction that generated the exception. Where the exception is taken from an Exception level using AArch32 these register numbers give the AArch64 view of the register." Which means that the HW is already giving us the translated version, and that we shouldn't try to interpret it at all (for example, doing an MMIO operation from the IRQ mode using the LR register leads to very unexpected behaviours). The fix is thus not to perform a call to vcpu_reg32() at all from vcpu_reg(), and use whatever register number is supplied directly. The only case we need to find out about the mapping is when we actively generate a register access, which only occurs when injecting a fault in a guest. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- arch/arm64/include/asm/kvm_emulate.h | 8 +++++--- arch/arm64/kvm/inject_fault.c | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-)