Re: [PATCH v2 09/15] x86/pv-shim: don't modify hypercall table

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.11.2021 16:20, Juergen Gross wrote:
> 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>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
albeit with a remark:

> @@ -1190,6 +1194,11 @@ long do_event_channel_op(int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>  {
>      int rc;
>  
> +#ifdef CONFIG_PV_SHIM
> +    if ( pv_shim )
> +        return pv_shim_event_channel_op(cmd, arg);
> +#endif

At the example of this, this is sub-optimal: In !PV_SHIM_EXCLUSIVE
this would imo better be unlikely(pv_shim). If it was just for the
positive forms, the annotation could actually be included in
pv_shim itself:

extern bool pv_shim;
#define pv_shim unlikely(pv_shim)

but this wouldn't necessarily play well with e.g. "if ( !pv_shim )".

As I would hope compilers don't mind constructs like "unlikely(1)",
I'd like to suggest considering to add likely() / unlikely() on
this and similar paths you add here.

Jan