| Message ID | 20191031150922.22938-20-julien.grall@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: XSA-201 and XSA-263 fixes | expand |
On Thu, 31 Oct 2019, Julien Grall wrote: > When a SError/Asynchronous Abort generated by the guest has been > consumed, we will skip the handling of the initial exception. > > This includes the calls to enter_hypervisor_from_guest{, _noirq} that > is used to synchronize part of the guest state with the internal > representation and re-enable workarounds (e.g. SSBD). However, we still > call leave_hypervisor_to_guest() which is used for preempting the guest > and synchronizing back part of the guest state. > > enter_hypervisor_from_guest{, _noirq} works in pair with > leave_hypervisor_to_guest(), so skipping the first two may result > in a loss of some part of guest state. > > An example is the new vGIC which will save the state of the LRs on exit > from the guest and rewrite all of them on entry to the guest. > > A more worrying example is SSBD workaround may not be re-enabled. If > leave_hypervisor_to_guest() is rescheduling the vCPU, then we may end to > run a lot of code with SSBD workaroud disabled. > > For now, calling leave_hypervisor_to_guest() is not necessary when > injecting a vSError to the guest. But it would still be good to give an > opportunity to reschedule. So both enter_hypervisor_from_guest() and > leave_hypervisor_to_guest() are called. > > Note that on arm64, the return value for check_pending_vserror is now > stored in x19 instead of x0. This is because we want to keep the value > across call to C-functions (x0, unlike x19, will not be saved by the > callee). > > Take the opportunity to rename check_pending_vserror() to > check_pending_guest_serror() as the function is dealing with host SError > and *not* virtual SError. The documentation is also updated accross > Arm32 and Arm64 to clarify how Xen is dealing with SError generated by > the guest. > > Signed-off-by: Julien Grall <julien.grall@arm.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > > Changes in v4: > - Rewording + typo > > Changes in v3: > - Update comments in the code. > - Update commit message > - Add arm32 support > > There are two known issues without this patch applied: > * The state of the vGIC when using the new version may be lost. > * SSBD workaround may be kept disabled while rescheduling the guest. > --- > xen/arch/arm/arm32/entry.S | 57 ++++++++++++++++++++++++++++++++++++++-------- > xen/arch/arm/arm64/entry.S | 54 ++++++++++++++++++++++++++++++++----------- > 2 files changed, 88 insertions(+), 23 deletions(-) > > diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S > index 34156c4404..b31056a616 100644 > --- a/xen/arch/arm/arm32/entry.S > +++ b/xen/arch/arm/arm32/entry.S > @@ -27,6 +27,10 @@ > /* > * Actions that needs to be done after entering the hypervisor from the > * guest and before the interrupts are unmasked. > + * > + * @return: > + * r4: Set to a non-zero value if a pending Abort exception took place. > + * Otherwise, it will be set to zero. > */ > arch_enter_hypervisor_from_guest_preirq: > #ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR > @@ -56,18 +60,35 @@ arch_enter_hypervisor_from_guest_preirq: > SAVE_ONE_BANKED(R11_fiq); SAVE_ONE_BANKED(R12_fiq); > > /* > - * If the SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT has been set in the cpu > - * feature, the checking of pending SErrors will be skipped. > + * We may have entered the hypervisor with pending asynchronous Abort > + * generated by the guest. If we need to categorize them, then > + * we need to consume any outstanding asynchronous Abort. > + * Otherwise, they can be consumed later on. > */ > alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT > + mov r4, #0 /* r4 := No Abort was consumed */ > b skip_check > alternative_else_nop_endif > > /* > - * Start to check pending virtual abort in the gap of Guest -> HYP > - * world switch. > + * Consume pending asynchronous Abort generated by the guest if any. > + * > + * The only way to consume an Abort interrupt is to unmask it. So > + * Abort exception will be unmaked for a small window and then masked > + * it again. > + * > + * It is fine to unmask asynchronous Abort exception as we fully > + * control the state of the processor and only limited code will > + * be executed if the exception returns (see do_trap_data_abort()). > * > - * Save ELR_hyp to check whether the pending virtual abort exception > + * TODO: The asynchronous abort path should be reworked to > + * inject the virtual asynchronous Abort in enter_hypervisor_* > + * rather than do_trap_data_abort(). This should make easier to > + * understand the path. > + */ > + > + /* > + * save elr_hyp to check whether the pending virtual abort exception > * takes place while we are doing this trap exception. > */ > mrs r1, ELR_hyp > @@ -112,11 +133,11 @@ abort_guest_exit_end: > cmp r1, r2 > > /* > - * Not equal, the pending virtual abort exception took place, the > - * initial exception does not have any significance to be handled. > - * Exit ASAP. > + * Set r4 depending on whether an asynchronous abort were > + * consumed. > */ > - bne return_from_trap > + movne r4, #1 > + moveq r4, #0 > > skip_check: > b enter_hypervisor_from_guest_preirq > @@ -179,12 +200,28 @@ ENDPROC(arch_enter_hypervisor_from_guest_preirq) > > 1: > /* Trap from the guest */ > + /* > + * arch_enter_hypervisor_from_guest_preirq will return with r4 set to > + * a non-zero value if an asynchronous Abort was consumed. > + * > + * When an asynchronous Abort has been consumed (r4 != 0), we may have > + * injected a virtual asynchronous Abort to the guest. > + * > + * In this case, the initial exception will be discarded (PC has > + * been adjusted by inject_vabt_exception()). However, we still > + * want to give an opportunity to reschedule the vCPU. So we > + * only want to skip the handling of the initial exception (i.e. > + * do_trap_*()). > + */ > bl arch_enter_hypervisor_from_guest_preirq > .if \guest_iflags != n > cpsie \guest_iflags > .endif > > - bl enter_hypervisor_from_guest > + adr lr, 2f > + cmp r4, #0 > + adrne lr, return_from_trap > + b enter_hypervisor_from_guest > > 2: > /* We are ready to handle the trap, setup the registers and jump. */ > diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S > index a8ba7ab961..d35855af96 100644 > --- a/xen/arch/arm/arm64/entry.S > +++ b/xen/arch/arm/arm64/entry.S > @@ -184,18 +184,41 @@ > .macro guest_vector compat, iflags, trap, save_x0_x1=1 > entry hyp=0, compat=\compat, save_x0_x1=\save_x0_x1 > /* > - * The vSError will be checked while SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT > - * is not set. If a vSError took place, the initial exception will be > - * skipped. Exit ASAP > + * We may have entered the hypervisor with pending SErrors > + * generated by the guest. If we need to categorize them, then > + * we need to check any outstanding SErrors will be consumed. > + * > + * The function check_pending_guest_serror() will unmask SError > + * exception temporarily. This is fine to do before enter_* > + * helpers are called because we fully control the state of the > + * processor and only limited code willl be executed (see > + * do_trap_hyp_serror()). > + * > + * When a SError has been consumed (x19 != 0), we may have injected a > + * virtual SError to the guest. > + * > + * In this case, the initial exception will be discarded (PC has > + * been adjusted by inject_vabt_exception()). However, we still > + * want to give an opportunity to reschedule the vCPU. So we > + * only want to skip the handling of the initial exception (i.e. > + * do_trap_*()). > + * > + * TODO: The SErrors path should be reworked to inject the vSError in > + * enter_hypervisor_* rather than do_trap_hyp_serror. This should make > + * easier to understand the path. > */ > alternative_if_not SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT > - bl check_pending_vserror > - cbnz x0, 1f > + bl check_pending_guest_serror > alternative_else_nop_endif > > bl enter_hypervisor_from_guest_preirq > msr daifclr, \iflags > bl enter_hypervisor_from_guest > + > + alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT > + cbnz x19, 1f > + alternative_else_nop_endif > + > mov x0, sp > bl do_trap_\trap > 1: > @@ -436,13 +459,17 @@ return_from_trap: > eret > > /* > - * This function is used to check pending virtual SError in the gap of > - * EL1 -> EL2 world switch. > - * The x0 register will be used to indicate the results of detection. > - * x0 -- Non-zero indicates a pending virtual SError took place. > - * x0 -- Zero indicates no pending virtual SError took place. > + * Consume pending SError generated by the guest if any. > + * > + * @return: > + * x19: Set to a non-zero value if a pending Abort exception took place. > + * Otherwise, it will be set to zero. > + * > + * Without RAS extension, the only way to consume a SError is to unmask > + * it. So the function will unmask SError exception for a small window and > + * then mask it again. > */ > -check_pending_vserror: > +check_pending_guest_serror: > /* > * Save elr_el2 to check whether the pending SError exception takes > * place while we are doing this sync exception. > @@ -487,11 +514,12 @@ abort_guest_exit_end: > > /* > * Not equal, the pending SError exception took place, set > - * x0 to non-zero. > + * x19 to non-zero. > */ > - cset x0, ne > + cset x19, ne > > ret > +ENDPROC(check_pending_guest_serror) > > /* > * Exception vectors. > -- > 2.11.0 >
diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S index 34156c4404..b31056a616 100644 --- a/xen/arch/arm/arm32/entry.S +++ b/xen/arch/arm/arm32/entry.S @@ -27,6 +27,10 @@ /* * Actions that needs to be done after entering the hypervisor from the * guest and before the interrupts are unmasked. + * + * @return: + * r4: Set to a non-zero value if a pending Abort exception took place. + * Otherwise, it will be set to zero. */ arch_enter_hypervisor_from_guest_preirq: #ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR @@ -56,18 +60,35 @@ arch_enter_hypervisor_from_guest_preirq: SAVE_ONE_BANKED(R11_fiq); SAVE_ONE_BANKED(R12_fiq); /* - * If the SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT has been set in the cpu - * feature, the checking of pending SErrors will be skipped. + * We may have entered the hypervisor with pending asynchronous Abort + * generated by the guest. If we need to categorize them, then + * we need to consume any outstanding asynchronous Abort. + * Otherwise, they can be consumed later on. */ alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT + mov r4, #0 /* r4 := No Abort was consumed */ b skip_check alternative_else_nop_endif /* - * Start to check pending virtual abort in the gap of Guest -> HYP - * world switch. + * Consume pending asynchronous Abort generated by the guest if any. + * + * The only way to consume an Abort interrupt is to unmask it. So + * Abort exception will be unmaked for a small window and then masked + * it again. + * + * It is fine to unmask asynchronous Abort exception as we fully + * control the state of the processor and only limited code will + * be executed if the exception returns (see do_trap_data_abort()). * - * Save ELR_hyp to check whether the pending virtual abort exception + * TODO: The asynchronous abort path should be reworked to + * inject the virtual asynchronous Abort in enter_hypervisor_* + * rather than do_trap_data_abort(). This should make easier to + * understand the path. + */ + + /* + * save elr_hyp to check whether the pending virtual abort exception * takes place while we are doing this trap exception. */ mrs r1, ELR_hyp @@ -112,11 +133,11 @@ abort_guest_exit_end: cmp r1, r2 /* - * Not equal, the pending virtual abort exception took place, the - * initial exception does not have any significance to be handled. - * Exit ASAP. + * Set r4 depending on whether an asynchronous abort were + * consumed. */ - bne return_from_trap + movne r4, #1 + moveq r4, #0 skip_check: b enter_hypervisor_from_guest_preirq @@ -179,12 +200,28 @@ ENDPROC(arch_enter_hypervisor_from_guest_preirq) 1: /* Trap from the guest */ + /* + * arch_enter_hypervisor_from_guest_preirq will return with r4 set to + * a non-zero value if an asynchronous Abort was consumed. + * + * When an asynchronous Abort has been consumed (r4 != 0), we may have + * injected a virtual asynchronous Abort to the guest. + * + * In this case, the initial exception will be discarded (PC has + * been adjusted by inject_vabt_exception()). However, we still + * want to give an opportunity to reschedule the vCPU. So we + * only want to skip the handling of the initial exception (i.e. + * do_trap_*()). + */ bl arch_enter_hypervisor_from_guest_preirq .if \guest_iflags != n cpsie \guest_iflags .endif - bl enter_hypervisor_from_guest + adr lr, 2f + cmp r4, #0 + adrne lr, return_from_trap + b enter_hypervisor_from_guest 2: /* We are ready to handle the trap, setup the registers and jump. */ diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S index a8ba7ab961..d35855af96 100644 --- a/xen/arch/arm/arm64/entry.S +++ b/xen/arch/arm/arm64/entry.S @@ -184,18 +184,41 @@ .macro guest_vector compat, iflags, trap, save_x0_x1=1 entry hyp=0, compat=\compat, save_x0_x1=\save_x0_x1 /* - * The vSError will be checked while SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT - * is not set. If a vSError took place, the initial exception will be - * skipped. Exit ASAP + * We may have entered the hypervisor with pending SErrors + * generated by the guest. If we need to categorize them, then + * we need to check any outstanding SErrors will be consumed. + * + * The function check_pending_guest_serror() will unmask SError + * exception temporarily. This is fine to do before enter_* + * helpers are called because we fully control the state of the + * processor and only limited code willl be executed (see + * do_trap_hyp_serror()). + * + * When a SError has been consumed (x19 != 0), we may have injected a + * virtual SError to the guest. + * + * In this case, the initial exception will be discarded (PC has + * been adjusted by inject_vabt_exception()). However, we still + * want to give an opportunity to reschedule the vCPU. So we + * only want to skip the handling of the initial exception (i.e. + * do_trap_*()). + * + * TODO: The SErrors path should be reworked to inject the vSError in + * enter_hypervisor_* rather than do_trap_hyp_serror. This should make + * easier to understand the path. */ alternative_if_not SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT - bl check_pending_vserror - cbnz x0, 1f + bl check_pending_guest_serror alternative_else_nop_endif bl enter_hypervisor_from_guest_preirq msr daifclr, \iflags bl enter_hypervisor_from_guest + + alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT + cbnz x19, 1f + alternative_else_nop_endif + mov x0, sp bl do_trap_\trap 1: @@ -436,13 +459,17 @@ return_from_trap: eret /* - * This function is used to check pending virtual SError in the gap of - * EL1 -> EL2 world switch. - * The x0 register will be used to indicate the results of detection. - * x0 -- Non-zero indicates a pending virtual SError took place. - * x0 -- Zero indicates no pending virtual SError took place. + * Consume pending SError generated by the guest if any. + * + * @return: + * x19: Set to a non-zero value if a pending Abort exception took place. + * Otherwise, it will be set to zero. + * + * Without RAS extension, the only way to consume a SError is to unmask + * it. So the function will unmask SError exception for a small window and + * then mask it again. */ -check_pending_vserror: +check_pending_guest_serror: /* * Save elr_el2 to check whether the pending SError exception takes * place while we are doing this sync exception. @@ -487,11 +514,12 @@ abort_guest_exit_end: /* * Not equal, the pending SError exception took place, set - * x0 to non-zero. + * x19 to non-zero. */ - cset x0, ne + cset x19, ne ret +ENDPROC(check_pending_guest_serror) /* * Exception vectors.
When a SError/Asynchronous Abort generated by the guest has been consumed, we will skip the handling of the initial exception. This includes the calls to enter_hypervisor_from_guest{, _noirq} that is used to synchronize part of the guest state with the internal representation and re-enable workarounds (e.g. SSBD). However, we still call leave_hypervisor_to_guest() which is used for preempting the guest and synchronizing back part of the guest state. enter_hypervisor_from_guest{, _noirq} works in pair with leave_hypervisor_to_guest(), so skipping the first two may result in a loss of some part of guest state. An example is the new vGIC which will save the state of the LRs on exit from the guest and rewrite all of them on entry to the guest. A more worrying example is SSBD workaround may not be re-enabled. If leave_hypervisor_to_guest() is rescheduling the vCPU, then we may end to run a lot of code with SSBD workaroud disabled. For now, calling leave_hypervisor_to_guest() is not necessary when injecting a vSError to the guest. But it would still be good to give an opportunity to reschedule. So both enter_hypervisor_from_guest() and leave_hypervisor_to_guest() are called. Note that on arm64, the return value for check_pending_vserror is now stored in x19 instead of x0. This is because we want to keep the value across call to C-functions (x0, unlike x19, will not be saved by the callee). Take the opportunity to rename check_pending_vserror() to check_pending_guest_serror() as the function is dealing with host SError and *not* virtual SError. The documentation is also updated accross Arm32 and Arm64 to clarify how Xen is dealing with SError generated by the guest. Signed-off-by: Julien Grall <julien.grall@arm.com> --- Changes in v4: - Rewording + typo Changes in v3: - Update comments in the code. - Update commit message - Add arm32 support There are two known issues without this patch applied: * The state of the vGIC when using the new version may be lost. * SSBD workaround may be kept disabled while rescheduling the guest. --- xen/arch/arm/arm32/entry.S | 57 ++++++++++++++++++++++++++++++++++++++-------- xen/arch/arm/arm64/entry.S | 54 ++++++++++++++++++++++++++++++++----------- 2 files changed, 88 insertions(+), 23 deletions(-)