Hi Andrew,
On 18/11/2021 01:47, Andrew Cooper wrote:
On 12/11/2021 09:36, Julien Grall wrote:
On 11/11/2021 17:57, Andrew Cooper wrote:
Retpolines are expensive, and all these do are select between the
sync and
nosync helpers. Pass a boolean instead, and use direct calls
everywhere.
To be honest, I much prefer to read the old code. I am totally not
against the change but I can see how I would be ready to introduce new
function pointers use in the future.
Really? The only reason there are function points to begin with was
because of a far more naive (and far pre-spectre) me fixing a reference
counting mess in 2014 by consolidating the logic. My mistake was not
spotting that the function pointers weren't actually necessary in the
first place.
So I think we need some guidelines on when to use function pointers in
Xen.
It's easy. If you are in any way unsure, they're probably the wrong
answer. (Ok - I'm being a little facetious)
There are concrete security improvements from not using function
pointers, demonstrated by fact that JOP/COP attacks are so pervasive
that all major hardware and software vendors are working on techniques
(both hardware and software) to prevent forward-edge control flow
integrity violations. (The mandate from the NSA to make this happen
certainly helped spur things on, too.)
There are also concrete performance improvements too. All competitive
processors in the market today can cope with direct branches more
efficiently than indirect branches, and a key principle behind
profile-guided-optimsiation is to rearrange your `ptr()` function
pointer call into `if ( ptr == a ) a(); else if ( ptr == b ) b(); else
ptr()` based on the frequency of destinations, because this really does
make orders of magnitude improvements in some cases.
We have some shockingly inappropriate uses of function pointers in Xen
right now (patches 4 and 5 in particular, and "x86/hvm: Remove callback
from paging->flush_tlb() hook" posted today). While this specific
example doesn't fall into shockingly inappropriate in my books, it is
firmly in the "not appropriate" category.
Thanks for the full explanation. What I am looking for is an update of
CODING_STYLE to make clear the function pointers should be avoided in
Xen and when we would accept them.
This actually compiles smaller than before:
... the code doesn't really compile smaller on Arm:
42sh> ../scripts/bloat-o-meter xen-syms-old xen-syms
add/remove: 4/2 grow/shrink: 0/6 up/down: 272/-252 (20)
Function old new delta
_domain_pause - 136 +136
_domain_pause_by_systemcontroller - 120 +120
domain_pause_by_systemcontroller_nosync - 8 +8
domain_pause_by_systemcontroller - 8 +8
domain_resume 136 132 -4
domain_pause_nosync 12 8 -4
domain_pause 12 8 -4
domain_pause_except_self 188 180 -8
do_domctl 5480 5472 -8
domain_kill 372 356 -16
do_domain_pause 88 - -88
__domain_pause_by_systemcontroller 120 - -120
Total: Before=966919, After=966939, chg +0.00%
ARM, like x86, compiles for speed, not size. "it got a bit larger" is
generally not as interesting as "it got smaller, despite everything the
compiler would normally do in the opposite direction".
My point is you have a generic section "this compiles smaller" section
in your commit message when in fact this was only tested with one x86
compiler version.
So at the minimum you should specify the version/architecture because
otherwise this sounds like you claim smaller Xen for everyone.
But to be honest, I don't really see the value to mention them as this
is depending on your compiler (e.g. it may be bigger or smaller) and as
you wrote it yourself "you're saying that for an removed 5 instructions".
Cheers,
--
Julien Grall