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

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

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


-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel