Re: [PATCH 08/12] x86/pv-shim: don't modify hypercall table

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.10.2021 14:51, 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.

While you say CONFIG_PV_SHIM_EXCLUSIVE here, ...

> @@ -845,6 +822,23 @@ static long pv_shim_grant_table_op(unsigned int cmd,
>      return rc;
>  }
>  
> +#ifndef CONFIG_GRANT_TABLE

... you don't actually enforce this here. I also don't see why it would
be needed in the "exclusive" case only. A binary usable both ways would
still need these, wouldn't it?

> +/* Thin wrapper(s) needed. */
> +long do_grant_table_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop,
> +                       unsigned int count)
> +{
> +    return pv_shim_grant_table_op(cmd, uop, count);
> +}
> +
> +#ifdef CONFIG_PV32
> +int compat_grant_table_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop,
> +                          unsigned int count)
> +{
> +    return pv_shim_grant_table_op(cmd, uop, count);
> +}
> +#endif
> +#endif

Don't you also need to adjust the respective #ifdef in
pv_hypercall_table[]? Otherwise I don't see how, at this point of the
series, the functions would actually get hooked up. In a bi-modal
binary further guarding will then be needed inside the wrappers, I
think. (While the table is going to go away, that guarding is going
to be needed even at the end of this series, I believe.)

Talking of wrappers - do you need actual code to be emitted for this?
Can't you simply put in place an alias each, which is permitted now
that pv_shim_grant_table_op() isn't static anymore? (Albeit that's
partly moot if guarding code gets added to the functions - in that
case only compat_grant_table_op() could become an alias of
do_grant_table_op().)

Jan