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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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. 
Obligatory citation of the recommendation for humans not to try annotating.

>>>> --- a/xen/common/compat/multicall.c
>>>> +++ b/xen/common/compat/multicall.c
>>>> @@ -5,7 +5,7 @@
>>>>  EMIT_FILE;
>>>>  
>>>>  #include <xen/types.h>
>>>> -#include <xen/multicall.h>
>>>> +#include <xen/hypercall.h>
>>>>  #include <xen/trace.h>
>>>>  
>>>>  #define COMPAT
>>>> @@ -19,7 +19,6 @@ static inline void xlat_multicall_entry(struct mc_state 
>>>> *mcs)
>>>>          mcs->compat_call.args[i] = mcs->call.args[i];
>>>>  }
>>>>  
>>>> -DEFINE_XEN_GUEST_HANDLE(multicall_entry_compat_t);
>>>>  #define multicall_entry      compat_multicall_entry
>>>>  #define multicall_entry_t    multicall_entry_compat_t
>>>>  #define do_multicall_call    compat_multicall_call
>>> Jürgen's patch doesn't have any change to this file, and I'm afraid I
>>> also don't see how these adjustments are related here. The commit
>>> message sadly also doesn't help ...
>> The changes are very necessary to split it out of Juergen's series.
>>
>> Without the adjustment, the correction of compat_platform_op()'s guest
>> handle type from void to compat_platform_op_t doesn't compile.
> Interesting. That's quite far from obvious in this context, so clarifying
> the purpose in the description would seem helpful.
>
> Coming back to the syncing with v3: Was this change the reason then why
> you did drop my R-b?

My porting of this patch is a non-trivial modification from Juergen's
version, and not eligible to retain any tags.

I thought I'd discussed this, but I appear to have missed it from both
versions of the series.  Sorry.

Either way.  It's exactly the same purpose as before, but modified to
compile in isolation.

Juergen's second patch, subsequent to this one, is unmodified which is
why I retained your R-by there.

~Andrew