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

[Juergen Gross <jgross@xxxxxxxx>]

On 15.10.21 15:57, Jan Beulich wrote:
> 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?
The "exclusive" case is normally the one where CONFIG_GRANT_TABLE is not
set. I highlighted this as it is a common situation.

> > +/* 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.
Ah, right.

> 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.)
Oh, you mean for the weird case of !pv_shim? Yes, I need to return
-ENOSYS in this case.

> 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().)
I didn't think of an alias. But I don't think I can make
compat_grant_table_op() an alias of do_grant_table_op(), as they have
different return types.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature