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

[Juergen Gross <jgross@xxxxxxxx>]

On 17.02.22 11:20, Jan Beulich wrote:
> On 16.02.2022 23:17, Andrew Cooper wrote:
> > 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.
> I don't see how the (unrelated) #ifndef matters here: The #ifndef
> is about grant table availability. The two likely() are about
> running as shim. I'm of the firm opinion that a binary built
> without PV_SHIM_EXCLUSIVE is far more likely to be used as a bare
> metal hypervisor. And for a PV_SHIM_EXCLUSIVE hypervisor the
> conditions are constant anyway, and hence the unlikely() has no
> effect.
>
> And if your way should really be followed, why did you deem the two
> unlikely() in do_event_channel_op() and do_grant_table_op() okay?
>
> > > > > > --- 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.
> I see. I'm under the impression though that parts were effectively
> present elsewhere in Jürgen's series. Perhaps it would have been easier
> if his series (at least up to the point to which you need it here)
> would (long) have gone in already. What it looks to be blocked on are
> two or three Arm acks and an x86 ack on patch 1 (which I've expressed
> I'm not entirely happy about, and hence I'm not going to either ack or
> nack it).
The main blocking point currently is that Julien would like me to let
all hypercalls return an int (apart from the ones which really need
a long). This will affect lot of common code and I need to have more
time for that endeavor.

An alternative to that would be to not rework the Arm side of the
hypercall logic.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature