| Message ID | 20190926183808.11630-11-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | xen/arm: XSA-201 and XSA-263 fixes | expand |
Hi Julien, Julien Grall writes: > At the moment, when a SError is received while checking for a pending > one, we will skip the handling the initial exception. > > This includes call to exit_from_guest{, _noirq} that is used to Did you mean enter_hypervisor_from_guest{, _noirq} there? Otherwise, I'm confused a lot. > synchronize part of the guest state with the internal representation. > However, we still call leave_hypervisor_tail() which is used for preempting > the guest and synchronizing back part of the guest state. > > exit_from_guest{, _noirq} works in pair with leave_hypervisor_tail(), so > skipping if former may result to 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. > > For now, calling leave_hypervisor_tail() is not necessary when injecting > a vSError to the guest. But as the path is spread accross multiple file, > it is hard to enforce that for the future (someone we may want to crash the > domain). Therefore it is best to call exit_from_guest{, _noirq} in the > vSError path as well. > > Note that the return value of check_pending_vserror is now set in x19 > instead of x0. This is because we want to keep the value across call to > C-function and x0, unlike x19, will not be saved by the callee. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > --- > > I am not aware of any issues other than with the new vGIC. But I > haven't looked hard enough so I think it would be worth to try to fix it > for Xen 4.13. > --- > xen/arch/arm/arm64/entry.S | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S > index 91cf6ee6f4..f5350247e1 100644 > --- a/xen/arch/arm/arm64/entry.S > +++ b/xen/arch/arm/arm64/entry.S > @@ -168,11 +168,13 @@ > /* > * 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 > + * skipped. > + * > + * However, we still need to call exit_from_guest{,_noirq} as the > + * return path to the guest may rely on state saved by them. > */ > alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT > bl check_pending_vserror > - cbnz x0, 1f > alternative_else_nop_endif > > mov x0, sp > @@ -180,6 +182,11 @@ > msr daifclr, \iflags > mov x0, sp > 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: > @@ -383,9 +390,9 @@ return_from_trap: > /* > * 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. > + * The register x19 will be used to indicate the results of detection. > + * x19 -- Non-zero indicates a pending virtual SError took place. > + * x19 -- Zero indicates no pending virtual SError took place. > */ > check_pending_vserror: > /* > @@ -432,9 +439,9 @@ 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
On Thu, 26 Sep 2019, Julien Grall wrote: > At the moment, when a SError is received while checking for a pending > one, we will skip the handling the initial exception. > > This includes call to exit_from_guest{, _noirq} that is used to > synchronize part of the guest state with the internal representation. > However, we still call leave_hypervisor_tail() which is used for preempting > the guest and synchronizing back part of the guest state. I second Volodymyr's comment about exit_from_guest* being the wrong name. > exit_from_guest{, _noirq} works in pair with leave_hypervisor_tail(), so > skipping if former may result to 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. > > For now, calling leave_hypervisor_tail() is not necessary when injecting > a vSError to the guest. But as the path is spread accross multiple file, > it is hard to enforce that for the future (someone we may want to crash the > domain). Therefore it is best to call exit_from_guest{, _noirq} in the > vSError path as well. > > Note that the return value of check_pending_vserror is now set in x19 > instead of x0. This is because we want to keep the value across call to > C-function and x0, unlike x19, will not be saved by the callee. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > --- > > I am not aware of any issues other than with the new vGIC. But I > haven't looked hard enough so I think it would be worth to try to fix it > for Xen 4.13. > --- > xen/arch/arm/arm64/entry.S | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S > index 91cf6ee6f4..f5350247e1 100644 > --- a/xen/arch/arm/arm64/entry.S > +++ b/xen/arch/arm/arm64/entry.S > @@ -168,11 +168,13 @@ > /* > * 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 > + * skipped. > + * > + * However, we still need to call exit_from_guest{,_noirq} as the > + * return path to the guest may rely on state saved by them. > */ > alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT > bl check_pending_vserror > - cbnz x0, 1f > alternative_else_nop_endif > > mov x0, sp > @@ -180,6 +182,11 @@ > msr daifclr, \iflags > mov x0, sp > bl enter_hypervisor_from_guest > + > + alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT > + cbnz x19, 1f > + alternative_else_nop_endif I like the idea of always calling enter_hypervisor_from_guest(_preirq) even when we are about to inject a SERROR into the guest. It is worth noting that with this patch the guest-related SERROR would still get delivered (due to the call to check_pending_vserror) before the guest state is saved by enter_hypervisor_from_guest(_preirq). So it is more like the following right? - enter hypervisor from guest - guest_vector macro - check_pending_vserror - SERROR is delivered - do_trap_hyp_serror - inject_vabt_exception - back to guest_vector macro - call enter_hypervisor_from_guest_preirq & enter_hypervisor_from_guest - leave_hypervisor_tail Which is better than what we would have without the patch, but not what one would expect. Would you be up for adding a in-code comment that describes the sequence, to clarify things? > mov x0, sp > bl do_trap_\trap > 1: > @@ -383,9 +390,9 @@ return_from_trap: > /* > * 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. > + * The register x19 will be used to indicate the results of detection. > + * x19 -- Non-zero indicates a pending virtual SError took place. > + * x19 -- Zero indicates no pending virtual SError took place. > */ > check_pending_vserror: > /* > @@ -432,9 +439,9 @@ 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 > > -- > 2.11.0 >
diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S index 91cf6ee6f4..f5350247e1 100644 --- a/xen/arch/arm/arm64/entry.S +++ b/xen/arch/arm/arm64/entry.S @@ -168,11 +168,13 @@ /* * 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 + * skipped. + * + * However, we still need to call exit_from_guest{,_noirq} as the + * return path to the guest may rely on state saved by them. */ alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT bl check_pending_vserror - cbnz x0, 1f alternative_else_nop_endif mov x0, sp @@ -180,6 +182,11 @@ msr daifclr, \iflags mov x0, sp 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: @@ -383,9 +390,9 @@ return_from_trap: /* * 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. + * The register x19 will be used to indicate the results of detection. + * x19 -- Non-zero indicates a pending virtual SError took place. + * x19 -- Zero indicates no pending virtual SError took place. */ check_pending_vserror: /* @@ -432,9 +439,9 @@ 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
At the moment, when a SError is received while checking for a pending one, we will skip the handling the initial exception. This includes call to exit_from_guest{, _noirq} that is used to synchronize part of the guest state with the internal representation. However, we still call leave_hypervisor_tail() which is used for preempting the guest and synchronizing back part of the guest state. exit_from_guest{, _noirq} works in pair with leave_hypervisor_tail(), so skipping if former may result to 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. For now, calling leave_hypervisor_tail() is not necessary when injecting a vSError to the guest. But as the path is spread accross multiple file, it is hard to enforce that for the future (someone we may want to crash the domain). Therefore it is best to call exit_from_guest{, _noirq} in the vSError path as well. Note that the return value of check_pending_vserror is now set in x19 instead of x0. This is because we want to keep the value across call to C-function and x0, unlike x19, will not be saved by the callee. Signed-off-by: Julien Grall <julien.grall@arm.com> --- I am not aware of any issues other than with the new vGIC. But I haven't looked hard enough so I think it would be worth to try to fix it for Xen 4.13. --- xen/arch/arm/arm64/entry.S | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)