Re: [PATCH v2 04/70] x86/pv-shim: Don't modify the hypercall table

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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 <jgross@xxxxxxxx>
>>>>>>
>>>>>> 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 <jgross@xxxxxxxx>
>>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>>> 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