Re: [Xen-devel] [PATCH v3 16/19] xen/arm: Introduce a macro to synchronize SError

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 31 Mar 2017, Wei Chen wrote:
> In previous patches, we have provided the ability to synchronize
> SErrors in exception entries. But we haven't synchronized SErrors
> while returning to guest and doing context switch.
> 
> So we still have two risks:
> 1. Slipping hypervisor SErrors to guest. For example, hypervisor
>    triggers a SError while returning to guest, but this SError may be
>    delivered after entering guest. In "DIVERSE" option, this SError
>    would be routed back to guest and panic the guest. But actually,
>    we should crash the whole system due to this hypervisor SError.
> 2. Slipping previous guest SErrors to the next guest. In "FORWARD"
>    option, if hypervisor triggers a SError while context switching.
>    This SError may be delivered after switching to next vCPU. In this
>    case, this SError will be forwarded to next vCPU and may panic
>    an incorrect guest.
> 
> So we have have to introduce this macro to synchronize SErrors while
> returning to guest and doing context switch.
> 
> We also added a barrier to this macro to prevent compiler reorder our
> asm volatile code.
> 
> Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


> ---
> v2->v3:
> 1. Use a macro to replace function to synchronize SErrors.
> 2. Add barrier to avoid compiler reorder our code.
> 
> Note:
> I had followed Julien's suggestion to add the Assert in to macro,
> But I found I always hit the Assert. Because when option == DIVERSE,
> the SKIP_CHECK_PENDING_VSERROR feature would not be set into cpu_hwcaps.
> And in the later patch, I will use this feature to skip synchronize
> SErrors before returning to guest.
> The cpus_has_cap(SKIP_CHECK_PENDING_VSERROR) will always false.
> And hit the ASSERT.
> 
> And about the local_abort enable check, should we disable the abort
> before synchronizing SErrors while returning to guest or doing context
> switch? Just like in these two places we have disable the IRQ.
> 
> For this testing, I have apply this series to latest staging tree.
> 
> ...
> (XEN) Command line: console=dtuart dtuart=serial0 conswitch=x loglvl=all
> dom0_mem=8G dom0_max_vcpus=8 serrors=diverse
> (XEN) Placing Xen at 0x00000083fee00000-0x00000083ff000000
> ...
> (XEN) ----SYNCHRONIZE_SERROR ASSERT 0 1
> (XEN) Assertion 'cpus_have_cap(5) && local_abort_is_enabled()' failed at
> traps.c:2954
> (XEN) ----[ Xen-4.9-unstable  arm64  debug=y   Not tainted ]----
> (XEN) CPU:    0
> (XEN) PC:     000000000025c09c leave_hypervisor_tail+0xa8/0x100
> (XEN) LR:     000000000025c078
> (XEN) SP:     00008003fac07e80
> (XEN) CPSR:   800002c9 MODE:64-bit EL2h (Hypervisor, handler)
> (XEN)      X0: 000000000000005a  X1: 00000000ffffffff  X2:
> 0000000000000000
> (XEN)      X3: 0000000000000000  X4: 0000000000000010  X5:
> 0000000000000000
> (XEN)      X6: 00008003ffe50000  X7: 0000000000000001  X8:
> 00000000fffffffd
> (XEN)      X9: 000000000000000a X10: 0000000000000031 X11:
> 00008003fac07bf8
> (XEN)     X12: 0000000000000001 X13: 000000000026c370 X14:
> 0000000000000020
> (XEN)     X15: 0000000000000000 X16: 00000083fff42fc0 X17:
> 00000000fffffffe
> (XEN)     X18: 0000000000000000 X19: 0000000000292c58 X20:
> 0000000000290028
> (XEN)     X21: 00000000002ea000 X22: 0000000000000000 X23:
> 0000000000000000
> (XEN)     X24: 0000000000000000 X25: 0000000000000000 X26:
> 0000000000000000
> (XEN)     X27: 0000000000000000 X28: 0000000000000000  FP:
> 00008003fac07e80
> (XEN)
> (XEN)   VTCR_EL2: 80043594
> (XEN)  VTTBR_EL2: 00010083fd036000
> (XEN)
> (XEN)  SCTLR_EL2: 30cd183d
> (XEN)    HCR_EL2: 000000008038663f
> (XEN)  TTBR0_EL2: 00000083fef0e000
> (XEN)
> (XEN)    ESR_EL2: f2000001
> (XEN)  HPFAR_EL2: 0000000000000000
> (XEN)    FAR_EL2: 0000000000000000
> (XEN)
> (XEN) Xen stack trace from sp=00008003fac07e80:
> (XEN)    00008003fac07ea0 0000000000262934 0000000000000000
> 0000000000000000
> (XEN)    0000000000000000 0000000000249cac 0000008048000000
> 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000
> 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000
> 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000
> 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000
> 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000
> 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000
> 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000
> 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000008040080000
> 00000000000001c5
> (XEN)    0000000000000000 0000000000000000 0000000000000000
> 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000
> 0000000000000000
> (XEN) Xen call trace:
> (XEN)    [<000000000025c09c>] leave_hypervisor_tail+0xa8/0x100 (PC)
> (XEN)    [<000000000025c078>] leave_hypervisor_tail+0x84/0x100 (LR)
> (XEN)    [<0000000000262934>] return_to_new_vcpu64+0x4/0x30
> (XEN)    [<0000000000249cac>] domain.c#continue_new_vcpu+0/0xa4
> (XEN)
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) Assertion 'cpus_have_cap(5) && local_abort_is_enabled()' failed at
> traps.c:2954
> (XEN) ****************************************
> (XEN)
> (XEN) Reboot in five seconds...
> ---
>  xen/include/asm-arm/processor.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index bb24bee..a787d1b 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -723,6 +723,17 @@ void abort_guest_exit_end(void);
>      ( (unsigned long)abort_guest_exit_end == (r)->pc ) \
>  )
>  
> +/*
> + * Synchronize SError unless the feature is selected.
> + * This is relying on the SErrors are currently unmasked.
> + */
> +#define SYNCHRONIZE_SERROR(feat)                                 \
> +    do {                                                         \
> +        asm volatile(ALTERNATIVE("dsb sy; isb",                  \
> +                                 "nop; nop", feat)               \
> +                                 : : : "memory");                \
> +    } while (0)
> +
>  #endif /* __ASSEMBLY__ */
>  #endif /* __ASM_ARM_PROCESSOR_H */
>  /*
> -- 
> 2.7.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel