[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





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,

--
Julien Grall

_______________________________________________
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®.