[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.