[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.