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

[Wei Chen <Wei.Chen@xxxxxxx>]

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@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.
I will send a follow-up patch after this series has been done.

> For this patch:
>
> Reviewed-by: Julien Grall <julien.grall@xxxxxxx>
>
> Cheers,

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