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

Re: [PATCH 1/5] x86/HVM: wire up multicalls


  • To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Fri, 18 Jun 2021 15:11:58 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; 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-SenderADCheck; bh=zxmM7mI43q5nKOuc7n+9LSefo/HVyKynhrajXY2Sq2Q=; b=JtTOkLT91vrkeKlSb2y+qArWOOU2PDC/6YNX8DNh3qalweXB0IkqE25blLe6dCqHIHoTkzPcnLZ7O6zobV7zGzCLZ0XxPcs/i2JI0/ofgHHSCY/5KSsW4VzXq9tm0gRsjhWt4dYJabDyloFFEEEJNpgBHfnYzUqMJDfV2DetfLA2GnxVWfh7dcWMch9l1BpEL4Y309LVMaZJ7d5/gY1d4meBYauwKTiecXoI2fk13hJ4IgwgrsnKPetR62H6Va5/YtAs+q0ZHEhYFHJ58OJPUOe7PFcI6vFt7meJSwNXkWxFtrCJywKFFCuo6I1wRlWNrhllxMMLLH1nnYwFU2b+bw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=f/Yu9DY+9IYIKReECdgEMdzI0by1PEyL5JvAFGYKigAwDmX/0wN1E9nuVZnTXnM1PRxf0NCcgGiOXNof5Ez4MpnclLodJkHyOXRRKxhSCw36WfvMNxYzetBFE86+pPGq90OGbaEvfX+ubaQ1tp2kO6DZCEKa7JkNQFSmaEKZ5a4/OslX/Jl5ncZ1RuabdWIaWrMWqE3Dui4mBS86ovYr4u8x6l2ehrXFVBD5h4L/82U8JXGb7uO3mrI4qIaT8+7Ss5VuTRqWi3Pi6pdfI4waXy9fd6IwNyY98AbXv9U5Y1A9wKdXFxhfJ5TqPzAXuvlvD6InStfR2wj10fVoGL5Jxw==
  • Authentication-results: lists.xenproject.org; dkim=none (message not signed) header.d=none;lists.xenproject.org; dmarc=none action=none header.from=suse.com;
  • Cc: Wei Liu <wl@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Juergen Gross <jgross@xxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Fri, 18 Jun 2021 13:12:09 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 18.06.2021 15:02, Andrew Cooper wrote:
> On 18/06/2021 11:23, Jan Beulich wrote:
>> To be able to use them from, in particular, the tool stack, they need to
>> be supported for all guest types. Note that xc_resource_op() already
>> does, so would not work without this on PVH Dom0.
> 
> I'm not a fan of multicalls as a concept - they're mostly a layering
> violation adding substantial complexity - and frankly, working around a
> Linux kernel/user ABI error is a terrible reason to make this change.

While I agree with the latter, I don't think there's much complexity
here, and there are certainly savings in terms of mode switch between
guest and hypervisor when you can batch up arbitrary calls (and not
just sufficiently similar ones with built-in batching).

>> @@ -334,6 +336,39 @@ int hvm_hypercall(struct cpu_user_regs *
>>      return curr->hcall_preempted ? HVM_HCALL_preempted : 
>> HVM_HCALL_completed;
>>  }
>>  
>> +enum mc_disposition hvm_do_multicall_call(struct mc_state *state)
>> +{
>> +    struct vcpu *curr = current;
>> +    hypercall_fn_t *func = NULL;
>> +
>> +    if ( hvm_guest_x86_mode(curr) == 8 )
>> +    {
>> +        struct multicall_entry *call = &state->call;
>> +
>> +        if ( call->op < ARRAY_SIZE(hvm_hypercall_table) )
>> +            func = array_access_nospec(hvm_hypercall_table, 
>> call->op).native;
>> +        if ( func )
>> +            call->result = func(call->args[0], call->args[1], call->args[2],
>> +                                call->args[3], call->args[4], 
>> call->args[5]);
>> +        else
>> +            call->result = -ENOSYS;
>> +    }
>> +    else
>> +    {
>> +        struct compat_multicall_entry *call = &state->compat_call;
>> +
>> +        if ( call->op < ARRAY_SIZE(hvm_hypercall_table) )
>> +            func = array_access_nospec(hvm_hypercall_table, 
>> call->op).compat;
>> +        if ( func )
>> +            call->result = func(call->args[0], call->args[1], call->args[2],
>> +                                call->args[3], call->args[4], 
>> call->args[5]);
>> +        else
>> +            call->result = -ENOSYS;
>> +    }
>> +
>> +    return !hvm_get_cpl(curr) ? mc_continue : mc_preempt;
> 
> This is ported across from XSA-213, but even for PV guests, it was just
> defence in depth IIRC for any cases we hadn't spotted, changing privilege.
> 
> There is no pagetable accounting in HVM guests to become confused by a
> privilege change, and hvm_get_cpl() isn't totally free.  Any kernel
> which puts VCPUOP_initialise in a multicall gets to keep all resulting
> pieces.
> 
> I think this wants to be just "return mc_continue;"

I had it this way first, but I think the state setting hypercalls
ought to be similarly protected. In fact I did this adjustment at
the last moment before sending, after having looked at Arm code.
If we don't want it here, it ought to go away there as well, and
then also for PV (where then only IRET would need special casing).

> If so, Begrudingly acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Thanks, I'll take this for the moment (ignoring the "if so"), but
I'll wait some to see whether the above wants further discussing.

Jan




 


Rackspace

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