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

[Wei Chen <Wei.Chen@xxxxxxx>]

On 2017/3/31 19:06, Julien Grall wrote:
>
>
> On 31/03/17 11:55, Wei Chen wrote:
>> Hi Julien,
>
> Hi Wei,
>
>> 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.
>
> ASSERTs are used to verify your assumption is correct, for non-debug
> built they will be turned into a NOP.
>
> I care about overhead in non-debug build, it is not much a concern for
> debug build. In the latter it is more important to verify the correctness.
>
> The abort bit is enabled at the entry in the hypervisor, and neither you
> nor I can tell if we need to disable abort in the future in some path.
> This ASSERT will catch those changes.
>
> So please give me a reason to remove it, because I see only benefits so far.

Ok, thanks, I understand know. As it just affects the debug built. I
will keep them in the macro.

>
> Cheers,
>


-- 
Regards,
Wei Chen

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