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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

Jan