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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxx>
> 
> ---
> 
> 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
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel