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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

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

Think a bit more about it. This macro will import more check than read
serror_op. This macro seems as a generic macro, it can be used in any
place. So you enable the feature check and abort check. But in this
patch, we know where the macro will be used, we know the abort is
enabled. And want to reduce the overhead as much as possible.

I prefer to define a similar macro as the following:
#define SYNCHRONIZE_SERROR(feat)                                    \
     do {                                                            \
         ALTERNATIVE("dsb sy; isb" : : : "memory", "nop; nop", feat);\
     } while (0)

> 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.
>
> Cheers,
>


-- 
Regards,
Wei Chen

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