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

[Julien Grall <julien.grall@xxxxxxx>]

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

Cheers,

--
Julien Grall

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