Re: [PATCH 4/5] libxc: use multicall for memory-op on Linux (and Solaris)

[Jan Beulich <jbeulich@suse.com>]

On 18.06.2021 17:05, Andrew Cooper wrote:
> On 18/06/2021 11:24, Jan Beulich wrote:
>> Some sub-functions, XENMEM_maximum_gpfn in particular, can return values
>> requiring more than 31 bits to represent. Hence we cannot issue the
>> hypercall directly when the return value of ioctl() is used to propagate
>> this value (note that this is not the case for the BSDs, and MiniOS
>> already wraps all hypercalls in a multicall).
>>
>> Suggested-by: Jürgen Groß <jgross@suse.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/tools/libs/ctrl/xc_private.c
>> +++ b/tools/libs/ctrl/xc_private.c
>> @@ -337,8 +337,47 @@ long do_memory_op(xc_interface *xch, int
>>          goto out1;
>>      }
>>  
>> -    ret = xencall2(xch->xcall, __HYPERVISOR_memory_op,
>> -                   cmd, HYPERCALL_BUFFER_AS_ARG(arg));
>> +#if defined(__linux__) || defined(__sun__)
>> +    /*
>> +     * Some sub-ops return values which don't fit in "int". On platforms
>> +     * without a specific hypercall return value field in the privcmd
>> +     * interface structure, issue the request as a single-element multicall,
>> +     * to be able to capture the full return value.
>> +     */
>> +    if ( sizeof(long) > sizeof(int) )
> 
> This is very fragile.  I spent a while coming up with
> 
>     __builtin_types_compatible_p(
>         typeof(ioctl) *,
>         long (*)(int, unsigned long, ...));
> 
> (which does work if you change int for long), just to realise that this
> won't actually help.

Help with what exactly? I'm not sure I see the severe fragility that
you see. I'd call this a little fragile because new OSes would need
to explicitly be checked for which behavior they ought to get. But
if one failed to do so, all that would happen is that they'd start
out with the same issue we're now trying to address.

>  I'm suck on trying to see whether
> privcmd_hypercall_t has a result member.

I.e. you're imagining __builtin_has_field()?

>> +    {
>> +        multicall_entry_t multicall = {
>> +            .op = __HYPERVISOR_memory_op,
>> +            .args[0] = cmd,
>> +            .args[1] = HYPERCALL_BUFFER_AS_ARG(arg),
>> +        }, *call = &multicall;
>> +        DECLARE_HYPERCALL_BOUNCE(call, sizeof(*call),
>> +                                 XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
>> +
>> +        if ( xc_hypercall_bounce_pre(xch, call) )
>> +        {
>> +            PERROR("Could not bounce buffer for memory_op hypercall");
>> +            goto out1;
>> +        }
>> +
>> +        ret = do_multicall_op(xch, HYPERCALL_BUFFER(call), 1);
>> +
>> +        xc_hypercall_bounce_post(xch, call);
>> +
>> +        if ( !ret )
>> +        {
>> +            ret = multicall.result;
>> +            if ( multicall.result > ~0xfffUL )
> 
> Wouldn't this be clearer as > -4095 ?

Not to me.

Jan