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

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>,
	"Ian Jackson" <iwj@xenproject.org>,
	"Juergen Gross" <jgross@suse.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 1/5] x86/HVM: wire up multicalls
Date: Fri, 18 Jun 2021 15:11:58 +0200	[thread overview]
Message-ID: <c46915cb-15a4-10ca-a566-b892eb8dc4f1@suse.com> (raw)
In-Reply-To: <511f161f-75b8-f23b-c6ba-cd7afe303760@citrix.com>

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@citrix.com>

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