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

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <Andrew.Cooper3@citrix.com>
Cc: Juergen Gross <jgross@suse.com>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v2 04/70] x86/pv-shim: Don't modify the hypercall table
Date: Tue, 22 Feb 2022 09:41:37 +0100	[thread overview]
Message-ID: <2fd2f479-575f-34db-ddd2-89df8eac81d7@suse.com> (raw)
In-Reply-To: <f1a3e319-4810-49eb-7ae0-a044499e85a1@citrix.com>

On 21.02.2022 20:21, Andrew Cooper wrote:
> 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@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 <jgross@suse.com>
>>>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@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.)

I'm afraid I'm completely lost then as to the (consistent) model you
want to see used. When putting them side by side:

@@ -3543,6 +3547,11 @@ do_grant_table_op(
     long rc;
     unsigned int opaque_in = cmd & GNTTABOP_ARG_MASK, opaque_out = 0;
 
+#ifdef CONFIG_PV_SHIM
+    if ( unlikely(pv_shim) )
+        return pv_shim_grant_table_op(cmd, uop, count);
+#endif
+
     if ( (int)count < 0 )
         return -EINVAL;

and

long do_grant_table_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop,
                       unsigned int count)
{
    if ( likely(!pv_shim) )
        return -ENOSYS;

    return pv_shim_grant_table_op(cmd, uop, count);
}

it is (to me at least) quite obvious that the unlikely() and likely()
both express _exactly_ the same thing.

> 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.

If certain options (or combinations thereof) are not supposed to be
used in practice, why would we allow them in the first place? Sadly
the commit introducing the GRANT_TABLE option supplies no justification
at all as to _why_ this control is/was wanted.

> 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.

Such a warning would imo be as appropriate (or not) as one for e.g.
"if ( 1 )".

> As for the function in question, the compiled result is an unconditional
> tailcall to pv_shim_grant_table_op.

For the "#define pv_shim true" case, I suppose. And then yes, as expected
for this particular case.

Anyway - once again in the interest of not blocking progress of the full
series, and once again contrary to my intention to not ever again do so
in situations like this one:
Reviewed-by: Jan Beulich <jbeulich@suse.com>
(with at least half a sentence said on the seemingly unrelated changes)

Jan


