[Xen-devel,RFC,for-4.13,10/10] xen/arm64: entry: Ensure the guest state is synced when receiving a vSError

Message ID 20190926183808.11630-11-julien.grall@arm.com
State New
Headers show
Series
  • xen/arm: XSA-201 and XSA-263 fixes
Related show

Commit Message

Julien Grall Sept. 26, 2019, 6:38 p.m.
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(-)

Comments

Volodymyr Babchuk Sept. 27, 2019, 3:30 p.m. | #1
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
Stefano Stabellini Oct. 2, 2019, 12:50 a.m. | #2
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
>

Patch

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