| Message ID | 1490865209-18283-2-git-send-email-Wei.Chen@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Wei, On 30/03/17 10:13, Wei Chen wrote: > Xen will do exception syndrome check while some types of exception > take place in EL2. The syndrome check code read the ESR_EL2 register > directly, but in some situation this register maybe overridden by > nested exception. > > For example, if we re-enable IRQ before reading ESR_EL2 which means > Xen may enter in IRQ exception mode and return the processor with > clobbered ESR_EL2 (See ARM ARM DDI 0487A.j D7.2.25) > > In this case the guest exception syndrome has been overridden, we will > check the syndrome for guest sync exception with an incorrect ESR_EL2 > value. So we want to save ESR_EL2 to cpu_user_regs as soon as the > exception takes place in EL2 to avoid using an incorrect syndrome value. > > In order to save ESR_EL2, we added a 32-bit member hsr to cpu_user_regs. > But while saving registers in trap entry, we use stp to save ELR and > CPSR at the same time through 64-bit general registers. If we keep this > code, the hsr will be overridden by upper 32-bit of CPSR. So adjust the > code to use str to save ELR in a separate instruction and use stp to > save CPSR and HSR at the same time through 32-bit general registers. > This change affects the registers restore in trap exit, we can't use the > ldp to restore ELR and CPSR from stack at the same time. We have to use > ldr to restore them separately. > > Signed-off-by: Wei Chen <Wei.Chen@arm.com> > > --- As mentioned in the internal review, it would have been helpful to mention this is a bug fix and should be backported on Xen 4.8 and Xen 4.7. > v1->v2: > 1. Use more accurate words in the commit message. > 2. Remove pointless comment message in cpu_user_regs. > 3. Explain the changes of the registers save/restore order in trap > entry/exit. > --- > xen/arch/arm/arm32/asm-offsets.c | 1 + > xen/arch/arm/arm32/entry.S | 3 +++ > xen/arch/arm/arm64/asm-offsets.c | 1 + > xen/arch/arm/arm64/entry.S | 13 +++++++++---- > xen/arch/arm/traps.c | 2 +- > xen/include/asm-arm/arm32/processor.h | 2 +- > xen/include/asm-arm/arm64/processor.h | 3 +-- A quick grep gives of ESR_EL2 in the code gives me: include/asm-arm/cpregs.h 308:#define ESR_EL2 HSR arch/arm/arm64/traps.c 35: union hsr hsr = { .bits = READ_SYSREG32(ESR_EL2) }; arch/arm/traps.c 846: printk(" ESR_EL2: %08"PRIx32"\n", READ_SYSREG32(ESR_EL2)); 2424: * (the bit ESR_EL2.S1PTW is set) 2644: const union hsr hsr = { .bits = READ_SYSREG32(ESR_EL2) }; The problem you describe also apply for the other READ_SYSREG32(ESR_EL2) and I was expecting to see them updated in this patch. Cheers,
Hi Julien On 2017/3/30 21:31, Julien Grall wrote: > Hi Wei, > > On 30/03/17 10:13, Wei Chen wrote: >> Xen will do exception syndrome check while some types of exception >> take place in EL2. The syndrome check code read the ESR_EL2 register >> directly, but in some situation this register maybe overridden by >> nested exception. >> >> For example, if we re-enable IRQ before reading ESR_EL2 which means >> Xen may enter in IRQ exception mode and return the processor with >> clobbered ESR_EL2 (See ARM ARM DDI 0487A.j D7.2.25) >> >> In this case the guest exception syndrome has been overridden, we will >> check the syndrome for guest sync exception with an incorrect ESR_EL2 >> value. So we want to save ESR_EL2 to cpu_user_regs as soon as the >> exception takes place in EL2 to avoid using an incorrect syndrome value. >> >> In order to save ESR_EL2, we added a 32-bit member hsr to cpu_user_regs. >> But while saving registers in trap entry, we use stp to save ELR and >> CPSR at the same time through 64-bit general registers. If we keep this >> code, the hsr will be overridden by upper 32-bit of CPSR. So adjust the >> code to use str to save ELR in a separate instruction and use stp to >> save CPSR and HSR at the same time through 32-bit general registers. >> This change affects the registers restore in trap exit, we can't use the >> ldp to restore ELR and CPSR from stack at the same time. We have to use >> ldr to restore them separately. >> >> Signed-off-by: Wei Chen <Wei.Chen@arm.com> >> >> --- > > As mentioned in the internal review, it would have been helpful to > mention this is a bug fix and should be backported on Xen 4.8 and Xen 4.7. > Sorry about that, I will add it to the note in next version. > >> v1->v2: >> 1. Use more accurate words in the commit message. >> 2. Remove pointless comment message in cpu_user_regs. >> 3. Explain the changes of the registers save/restore order in trap >> entry/exit. >> --- >> xen/arch/arm/arm32/asm-offsets.c | 1 + >> xen/arch/arm/arm32/entry.S | 3 +++ >> xen/arch/arm/arm64/asm-offsets.c | 1 + >> xen/arch/arm/arm64/entry.S | 13 +++++++++---- >> xen/arch/arm/traps.c | 2 +- >> xen/include/asm-arm/arm32/processor.h | 2 +- >> xen/include/asm-arm/arm64/processor.h | 3 +-- > > A quick grep gives of ESR_EL2 in the code gives me: > > include/asm-arm/cpregs.h > 308:#define ESR_EL2 HSR > > arch/arm/arm64/traps.c > 35: union hsr hsr = { .bits = READ_SYSREG32(ESR_EL2) }; > > arch/arm/traps.c > 846: printk(" ESR_EL2: %08"PRIx32"\n", READ_SYSREG32(ESR_EL2)); > 2424: * (the bit ESR_EL2.S1PTW is set) > 2644: const union hsr hsr = { .bits = READ_SYSREG32(ESR_EL2) }; > > The problem you describe also apply for the other READ_SYSREG32(ESR_EL2) > and I was expecting to see them updated in this patch. > Yes, that's what I hadn't considered. I would cover them in next version. > Cheers, >
diff --git a/xen/arch/arm/arm32/asm-offsets.c b/xen/arch/arm/arm32/asm-offsets.c index f8e6b53..5b543ab 100644 --- a/xen/arch/arm/arm32/asm-offsets.c +++ b/xen/arch/arm/arm32/asm-offsets.c @@ -26,6 +26,7 @@ void __dummy__(void) OFFSET(UREGS_lr, struct cpu_user_regs, lr); OFFSET(UREGS_pc, struct cpu_user_regs, pc); OFFSET(UREGS_cpsr, struct cpu_user_regs, cpsr); + OFFSET(UREGS_hsr, struct cpu_user_regs, hsr); OFFSET(UREGS_LR_usr, struct cpu_user_regs, lr_usr); OFFSET(UREGS_SP_usr, struct cpu_user_regs, sp_usr); diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S index 2a6f4f0..2187226 100644 --- a/xen/arch/arm/arm32/entry.S +++ b/xen/arch/arm/arm32/entry.S @@ -23,6 +23,9 @@ add r11, sp, #UREGS_kernel_sizeof+4; \ str r11, [sp, #UREGS_sp]; \ \ + mrc CP32(r11, HSR); /* Save exception syndrome */ \ + str r11, [sp, #UREGS_hsr]; \ + \ mrs r11, SPSR_hyp; \ str r11, [sp, #UREGS_cpsr]; \ and r11, #PSR_MODE_MASK; \ diff --git a/xen/arch/arm/arm64/asm-offsets.c b/xen/arch/arm/arm64/asm-offsets.c index 69ea92a..ce24e44 100644 --- a/xen/arch/arm/arm64/asm-offsets.c +++ b/xen/arch/arm/arm64/asm-offsets.c @@ -27,6 +27,7 @@ void __dummy__(void) OFFSET(UREGS_SP, struct cpu_user_regs, sp); OFFSET(UREGS_PC, struct cpu_user_regs, pc); OFFSET(UREGS_CPSR, struct cpu_user_regs, cpsr); + OFFSET(UREGS_ESR_el2, struct cpu_user_regs, hsr); OFFSET(UREGS_SPSR_el1, struct cpu_user_regs, spsr_el1); diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S index c181b5e..02802c0 100644 --- a/xen/arch/arm/arm64/entry.S +++ b/xen/arch/arm/arm64/entry.S @@ -121,9 +121,13 @@ lr .req x30 // link register stp lr, x21, [sp, #UREGS_LR] - mrs x22, elr_el2 - mrs x23, spsr_el2 - stp x22, x23, [sp, #UREGS_PC] + mrs x21, elr_el2 + str x21, [sp, #UREGS_PC] + + add x21, sp, #UREGS_CPSR + mrs x22, spsr_el2 + mrs x23, esr_el2 + stp w22, w23, [x21] .endm @@ -307,7 +311,8 @@ ENTRY(return_to_new_vcpu64) return_from_trap: msr daifset, #2 /* Mask interrupts */ - ldp x21, x22, [sp, #UREGS_PC] // load ELR, SPSR + ldr x21, [sp, #UREGS_PC] // load ELR + ldr w22, [sp, #UREGS_CPSR] // load SPSR pop x0, x1 pop x2, x3 diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 614501f..1da6d24 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -2641,7 +2641,7 @@ static void enter_hypervisor_head(struct cpu_user_regs *regs) asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs) { - const union hsr hsr = { .bits = READ_SYSREG32(ESR_EL2) }; + const union hsr hsr = { .bits = regs->hsr }; enter_hypervisor_head(regs); diff --git a/xen/include/asm-arm/arm32/processor.h b/xen/include/asm-arm/arm32/processor.h index db3b17b..f6d5df3 100644 --- a/xen/include/asm-arm/arm32/processor.h +++ b/xen/include/asm-arm/arm32/processor.h @@ -37,7 +37,7 @@ struct cpu_user_regs uint32_t pc, pc32; }; uint32_t cpsr; /* Return mode */ - uint32_t pad0; /* Doubleword-align the kernel half of the frame */ + uint32_t hsr; /* Exception Syndrome */ /* Outer guest frame only from here on... */ diff --git a/xen/include/asm-arm/arm64/processor.h b/xen/include/asm-arm/arm64/processor.h index b0726ff..24f836b 100644 --- a/xen/include/asm-arm/arm64/processor.h +++ b/xen/include/asm-arm/arm64/processor.h @@ -66,8 +66,7 @@ struct cpu_user_regs /* Return address and mode */ __DECL_REG(pc, pc32); /* ELR_EL2 */ uint32_t cpsr; /* SPSR_EL2 */ - - uint32_t pad0; /* Align end of kernel frame. */ + uint32_t hsr; /* ESR_EL2 */ /* Outer guest frame only from here on... */
Xen will do exception syndrome check while some types of exception take place in EL2. The syndrome check code read the ESR_EL2 register directly, but in some situation this register maybe overridden by nested exception. For example, if we re-enable IRQ before reading ESR_EL2 which means Xen may enter in IRQ exception mode and return the processor with clobbered ESR_EL2 (See ARM ARM DDI 0487A.j D7.2.25) In this case the guest exception syndrome has been overridden, we will check the syndrome for guest sync exception with an incorrect ESR_EL2 value. So we want to save ESR_EL2 to cpu_user_regs as soon as the exception takes place in EL2 to avoid using an incorrect syndrome value. In order to save ESR_EL2, we added a 32-bit member hsr to cpu_user_regs. But while saving registers in trap entry, we use stp to save ELR and CPSR at the same time through 64-bit general registers. If we keep this code, the hsr will be overridden by upper 32-bit of CPSR. So adjust the code to use str to save ELR in a separate instruction and use stp to save CPSR and HSR at the same time through 32-bit general registers. This change affects the registers restore in trap exit, we can't use the ldp to restore ELR and CPSR from stack at the same time. We have to use ldr to restore them separately. Signed-off-by: Wei Chen <Wei.Chen@arm.com> --- v1->v2: 1. Use more accurate words in the commit message. 2. Remove pointless comment message in cpu_user_regs. 3. Explain the changes of the registers save/restore order in trap entry/exit. --- xen/arch/arm/arm32/asm-offsets.c | 1 + xen/arch/arm/arm32/entry.S | 3 +++ xen/arch/arm/arm64/asm-offsets.c | 1 + xen/arch/arm/arm64/entry.S | 13 +++++++++---- xen/arch/arm/traps.c | 2 +- xen/include/asm-arm/arm32/processor.h | 2 +- xen/include/asm-arm/arm64/processor.h | 3 +-- 7 files changed, 17 insertions(+), 8 deletions(-)