diff mbox

[v3,16/19] xen/arm: Introduce a macro to synchronize SError

Message ID 1490965679-619-17-git-send-email-Wei.Chen@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Wei Chen March 31, 2017, 1:07 p.m. UTC
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@arm.com>
---
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(+)

Comments

Julien Grall March 31, 2017, 2:33 p.m. UTC | #1
Hi Wei,

On 31/03/17 14:07, 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@arm.com>
> ---
> 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.

Because the ASSERT I suggested was wrong, sorry for that. It should have 
been:

ASSERT(!cpus_have_cap(feat) && local_abort_is_enabled());

This is because we want abort enabled when the "feature" is not present.

This series looks good so far, so I would be happy if you send a 
follow-up patch to add the ASSERT rather than modifying this patch.

For this patch:

Reviewed-by: Julien Grall <julien.grall@arm.com>

Cheers,
Stefano Stabellini March 31, 2017, 6:36 p.m. UTC | #2
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@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> 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
>
Wei Chen April 5, 2017, 7:14 a.m. UTC | #3
Hi Julien,

On 2017/3/31 22:33, Julien Grall wrote:
> Hi Wei,
>
> On 31/03/17 14:07, 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@arm.com>
>> ---
>> 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.
>
> Because the ASSERT I suggested was wrong, sorry for that. It should have
> been:
>
> ASSERT(!cpus_have_cap(feat) && local_abort_is_enabled());
>
> This is because we want abort enabled when the "feature" is not present.
>
> This series looks good so far, so I would be happy if you send a
> follow-up patch to add the ASSERT rather than modifying this patch.
>

I will send a follow-up patch after this series has been done.

> For this patch:
>
> Reviewed-by: Julien Grall <julien.grall@arm.com>
>
> Cheers,
>
Julien Grall April 5, 2017, 7:29 a.m. UTC | #4
On 05/04/2017 08:14, Wei Chen wrote:
>> Because the ASSERT I suggested was wrong, sorry for that. It should have
>> been:
>>
>> ASSERT(!cpus_have_cap(feat) && local_abort_is_enabled());
>>
>> This is because we want abort enabled when the "feature" is not present.
>>
>> This series looks good so far, so I would be happy if you send a
>> follow-up patch to add the ASSERT rather than modifying this patch.
>>
>
> I will send a follow-up patch after this series has been done.

Well, you have to resend this series. So why don't you add the ASSERT in 
the new version?

Cheers,
Wei Chen April 5, 2017, 7:35 a.m. UTC | #5
On 2017/4/5 15:29, Julien Grall wrote:
>
>
> On 05/04/2017 08:14, Wei Chen wrote:
>>> Because the ASSERT I suggested was wrong, sorry for that. It should have
>>> been:
>>>
>>> ASSERT(!cpus_have_cap(feat) && local_abort_is_enabled());
>>>
>>> This is because we want abort enabled when the "feature" is not present.
>>>
>>> This series looks good so far, so I would be happy if you send a
>>> follow-up patch to add the ASSERT rather than modifying this patch.
>>>
>>
>> I will send a follow-up patch after this series has been done.
>
> Well, you have to resend this series. So why don't you add the ASSERT in
> the new version?

I am happy to do that. I misunderstood this comment "rather than 
modifying this patch".

I will add a patch in this series for ASSERT.

>
> Cheers,
>
Julien Grall April 5, 2017, 8:02 a.m. UTC | #6
Hi Wei,

On 05/04/2017 08:35, Wei Chen wrote:
> On 2017/4/5 15:29, Julien Grall wrote:
>>
>>
>> On 05/04/2017 08:14, Wei Chen wrote:
>>>> Because the ASSERT I suggested was wrong, sorry for that. It should
>>>> have
>>>> been:
>>>>
>>>> ASSERT(!cpus_have_cap(feat) && local_abort_is_enabled());
>>>>
>>>> This is because we want abort enabled when the "feature" is not
>>>> present.
>>>>
>>>> This series looks good so far, so I would be happy if you send a
>>>> follow-up patch to add the ASSERT rather than modifying this patch.
>>>>
>>>
>>> I will send a follow-up patch after this series has been done.
>>
>> Well, you have to resend this series. So why don't you add the ASSERT in
>> the new version?
>
> I am happy to do that. I misunderstood this comment "rather than
> modifying this patch".

My point was if you didn't need to resend the series, I would have been 
happy to see a follow-up avoiding sending 19 patches again for a small fix.

>
> I will add a patch in this series for ASSERT.

Again, as you resend the series. Why don't you update this patch to add 
the ASSERT?

Cheers,
Wei Chen April 5, 2017, 8:08 a.m. UTC | #7
Hi Julien,

On 2017/4/5 15:29, Julien Grall wrote:
>
>
> On 05/04/2017 08:14, Wei Chen wrote:
>>> Because the ASSERT I suggested was wrong, sorry for that. It should have
>>> been:
>>>
>>> ASSERT(!cpus_have_cap(feat) && local_abort_is_enabled());
>>>
>>> This is because we want abort enabled when the "feature" is not present.
>>>

Think more about this assert, I feel confused.

Currently, enable abort or not has not combined with the "feature".
In my testing, the abort is always enabled in the places where we want
to use this macro.

Use "SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT" for example,
	ASSERT(cpus_have_cap(feat) && local_abort_is_enabled());
will panic by when option == DIVERSE, and 	
	ASSERT(!cpus_have_cap(feat) && local_abort_is_enabled());
will panic by when option != DIVERSE

I can't see any significance about this change.

>>> This series looks good so far, so I would be happy if you send a
>>> follow-up patch to add the ASSERT rather than modifying this patch.
>>>
>>
>> I will send a follow-up patch after this series has been done.
>
> Well, you have to resend this series. So why don't you add the ASSERT in
> the new version?
>
> Cheers,
>
Julien Grall April 5, 2017, 8:20 a.m. UTC | #8
On 05/04/2017 09:08, Wei Chen wrote:
> Hi Julien,
>
> On 2017/4/5 15:29, Julien Grall wrote:
>>
>>
>> On 05/04/2017 08:14, Wei Chen wrote:
>>>> Because the ASSERT I suggested was wrong, sorry for that. It should
>>>> have
>>>> been:
>>>>
>>>> ASSERT(!cpus_have_cap(feat) && local_abort_is_enabled());
>>>>
>>>> This is because we want abort enabled when the "feature" is not
>>>> present.
>>>>
>
> Think more about this assert, I feel confused.
>
> Currently, enable abort or not has not combined with the "feature".
> In my testing, the abort is always enabled in the places where we want
> to use this macro.
>
> Use "SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT" for example,
>     ASSERT(cpus_have_cap(feat) && local_abort_is_enabled());
> will panic by when option == DIVERSE, and
>     ASSERT(!cpus_have_cap(feat) && local_abort_is_enabled());
> will panic by when option != DIVERSE

Because I keep making mistake in the ASSERT and I am surprised that 
nobody spot and able to fix...

This should be ASSERT(!cpus_have_cap(feat) || local_abort_is_enabled());

Thinking a bit more, we can also do an ASSERT(local_abort_is_enabled()) 
unconditionally.

> I can't see any significance about this change.

As I said earlier, the main purpose of the ASSERT is to ensure your 
assumption is correct. The abort has been unmasked in the entry but you 
don't know if someone will mask the abort later.

And yes, nobody is playing with the abort mask so far. But are you able 
to predict what will be done in the future? I am not, so the ASSERT is 
here to be sure the abort is unmasked.

Cheers,
Wei Chen April 5, 2017, 8:32 a.m. UTC | #9
Hi Julien,

On 2017/4/5 16:20, Julien Grall wrote:
>
>
> On 05/04/2017 09:08, Wei Chen wrote:
>> Hi Julien,
>>
>> On 2017/4/5 15:29, Julien Grall wrote:
>>>
>>>
>>> On 05/04/2017 08:14, Wei Chen wrote:
>>>>> Because the ASSERT I suggested was wrong, sorry for that. It should
>>>>> have
>>>>> been:
>>>>>
>>>>> ASSERT(!cpus_have_cap(feat) && local_abort_is_enabled());
>>>>>
>>>>> This is because we want abort enabled when the "feature" is not
>>>>> present.
>>>>>
>>
>> Think more about this assert, I feel confused.
>>
>> Currently, enable abort or not has not combined with the "feature".
>> In my testing, the abort is always enabled in the places where we want
>> to use this macro.
>>
>> Use "SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT" for example,
>>     ASSERT(cpus_have_cap(feat) && local_abort_is_enabled());
>> will panic by when option == DIVERSE, and
>>     ASSERT(!cpus_have_cap(feat) && local_abort_is_enabled());
>> will panic by when option != DIVERSE
>
> Because I keep making mistake in the ASSERT and I am surprised that
> nobody spot and able to fix...
>

I think maybe I still in the mood of holiday and my head is not clear
:)

> This should be ASSERT(!cpus_have_cap(feat) || local_abort_is_enabled());
>
> Thinking a bit more, we can also do an ASSERT(local_abort_is_enabled())
> unconditionally.
>
>> I can't see any significance about this change.
>
> As I said earlier, the main purpose of the ASSERT is to ensure your
> assumption is correct. The abort has been unmasked in the entry but you
> don't know if someone will mask the abort later.
>
> And yes, nobody is playing with the abort mask so far. But are you able
> to predict what will be done in the future? I am not, so the ASSERT is
> here to be sure the abort is unmasked.

I meant change from cpus_have_cap(feat) to !cpus_have_cap(feat) :)

>
> Cheers,
>
diff mbox

Patch

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 */
 /*