[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


  • To: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Thu, 17 Feb 2022 11:20:49 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=hgaWmZ0lcq7FzIcTB3VPyEClczv06jhS9M81h14zoSc=; b=XitLCcnX8O75KKEjALtUl1eN1A+PikeTKGu/42if+VV+eG5GIcruyWl/gpw2cq20ke1Xm1AyBId82c1/KdEepryrVSh2FF/hW5Q/5GlfGk3uBFeTbLSGb/3Q24fOFga4EmEcG8ZJYXUkTJ60CamDTISyRGCKZ72JhJsvd/MmdHOoI3W/YQe/s7iDF9JSE2YLn8dVl1UIppPJXqzOc6K7xg1f0iCYyFx6uJL6cKMtR3LyNmT+YOx8RPZC72K3jxrqwmQlJ4GcLmIuZtlD/fxJWR3aBmw0AZI2ksOyscbiS97/hiSJNA8kqzREOIk6MmSjo6eJ8FBxYqwFJf/i0rCeBw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=AYhyvqieWnlrM+GQLxiRpFeVu7OPFGCGl7Et84oGBlAgcb8MXX3YSDTqtF0IvK38rHZ4Y29gu8HSZZQFbF3in7sDlSM+xwYL/nnv+icY0dur/SBldVDpzmVa8oXhgyNW2rBTFqyx5+zbzIIs9H9Z0hjz5MQ0HVb0kAiXzhJA/Ns6m+Z0Tcm9aFHYPbhuhdADSiFoskmrsW+eaP4pOlp0BOcG4oBoFwY1/WgIMSEfD129sCC6FNhNjp9rppAi3IkTDXj5oN9BbXtW/W2WXT8GAtWiqkXPTatIejkTOIGxoCepc5KoS3z+t/AGw3gVPFO8DNGKwrhct2GPtyr+UV9JWg==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Juergen Gross <jgross@xxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Thu, 17 Feb 2022 10:21:03 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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).

Jan




 


Rackspace

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