On 17/02/2022 10:20, Jan Beulich wrote: > On 16.02.2022 23:17, Andrew Cooper wrote: >> On 14/02/2022 13:56, Jan Beulich wrote: >>> On 14.02.2022 14:50, Andrew Cooper wrote: >>>> On 14/02/2022 13:33, Jan Beulich wrote: >>>>> On 14.02.2022 13:50, Andrew Cooper wrote: >>>>>> From: Juergen Gross <jgr...@suse.com> >>>>>> >>>>>> When running as pv-shim the hypercall is modified today in order to >>>>>> replace the functions for __HYPERVISOR_event_channel_op and >>>>>> __HYPERVISOR_grant_table_op hypercalls. >>>>>> >>>>>> Change this to call the related functions from the normal handlers >>>>>> instead when running as shim. The performance implications are not >>>>>> really relevant, as a normal production hypervisor will not be >>>>>> configured to support shim mode, so the related calls will be dropped >>>>>> due to optimization of the compiler. >>>>>> >>>>>> Note that for the CONFIG_PV_SHIM_EXCLUSIVE case there is a dummy >>>>>> wrapper do_grant_table_op() needed, as in this case grant_table.c >>>>>> isn't being built. >>>>>> >>>>>> Signed-off-by: Juergen Gross <jgr...@suse.com> >>>>>> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com> >>>>> I don't think you sync-ed this with Jürgen's v3. There were only minor >>>>> changes but having a stale version sent two months later isn't very >>>>> nice. >>>> I did resync. What do you think is missing? >>> A few likely() / unlikely() as far as I could see. >> Oh those two. I appear to have forgot to email. >> >> They're wrong - observe they're in an ifndef block, not an ifdef block. > I don't see how the (unrelated) #ifndef matters here: The #ifndef > is about grant table availability. The two likely() are about > running as shim. I'm of the firm opinion that a binary built > without PV_SHIM_EXCLUSIVE is far more likely to be used as a bare > metal hypervisor. And for a PV_SHIM_EXCLUSIVE hypervisor the > conditions are constant anyway, and hence the unlikely() has no > effect. > > And if your way should really be followed, why did you deem the two > unlikely() in do_event_channel_op() and do_grant_table_op() okay?
Because those are at least not incorrect. (I still think we have far too many annotations, and I doubt they're all helpful.) The gnttab stubs in the !GNTTAB case exist strictly for compile tests (there's no such thing as a production build of Xen without grant tables) and PV_SHIM_EXCLUSIVE builds. Code layout only matters for cases where we're executing code, which is the PV Shim case, at which point the condition is constant and doesn't generate a branch. A compiler ought to raise a warning on finding that __builtin_expect() has a constant parameter, because it's a nop in one case, and demonstrably false in the other. As for the function in question, the compiled result is an unconditional tailcall to pv_shim_grant_table_op. ~Andrew
