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

[Julien Grall <julien.grall@xxxxxxx>]

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