[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v2 16/19] xen/arm: Introduce a helper to synchronize SError
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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
