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

From: Julien Grall <julien.grall@arm.com>
To: Wei Chen <Wei.Chen@arm.com>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>
Cc: Kaly Xin <Kaly.Xin@arm.com>, nd <nd@arm.com>,
	"sstabellini@kernel.org" <sstabellini@kernel.org>,
	Steve Capper <Steve.Capper@arm.com>
Subject: Re: [PATCH v2 16/19] xen/arm: Introduce a helper to synchronize SError
Date: Fri, 31 Mar 2017 12:06:03 +0100	[thread overview]
Message-ID: <017848e1-6281-9bcb-9cf5-c9a1c36ce832@arm.com> (raw)
In-Reply-To: <HE1PR08MB0587224B439C7A94A6DC728C9E370@HE1PR08MB0587.eurprd08.prod.outlook.com>

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.

Cheers,

-- 
Julien Grall

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