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

[Wei Chen <Wei.Chen@xxxxxxx>]

On 2017/3/31 2:38, Julien Grall wrote:
>
>
> On 30/03/17 19:32, Julien Grall wrote:
>> On 30/03/17 19:28, Julien Grall wrote:
>>> Hi Wei,
>>>
>>> On 30/03/17 10:13, Wei Chen wrote:
>>>> +void synchronize_serror(void)
>>>
>>> Sorry for been late in the party. Looking at the way you use the
>>> function, you execute depending on the behavior chosen by the user when
>>> an SErrors happen. This behavior will not change at runtime, so always
>>> checking the option chosen in the hot path does not sound very efficient.
>>>
>>> I would recommend to look at ALTERNATIVE and streamline (dsb sy, isb).
>>> I.e
>>>
>>> ALTERNATIVE("dsb sy; isb", "nop; nop", ...) or the invert depending of
>>> the place.
>>
>> To complete what I was suggestion, you could define:
>>
>> /* Synchronize SError unless the feature is selected */
>> #define SYNCHRONIZE_SERROR(feat) ALTERNATIVE("dsb sy; isb", "nop; nop");
>
> Or even:
>
> /*
>   * Synchronize SError unless the feature is selected.
>   * This is relying on the SErrors are currently unmasked.
>   */
> #define SYNCHRONIZE_SERROR(feat)                                \
>       do {                                                      \
>         ASSERT(cpus_have_cap(feat) && local_abort_is_enabled());\
>         ALTERNATIVE("dsb sy; isb", "nop; nop");                 \
>       while (0)
>
> The ASSERT is here to check that we have abort enabled. Otherwise, doing
> the synchronization would be pointless.
>
> Note that the function local_abort_is_enabled is not implemented. But it
> is easy to write it. Looking how local_irq_is_enabled was implemented.
>

This is a better solution than function, I would do it.

> Cheers,
>


-- 
Regards,
Wei Chen

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