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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> --- 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.  I'm suck on trying to see whether
privcmd_hypercall_t has a result member.

> +    {
> +        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 ?

~Andrew

> +            {
> +                errno = -ret;
> +                ret = -1;
> +            }
> +        }
> +    }
> +    else
> +#endif
> +        ret = xencall2L(xch->xcall, __HYPERVISOR_memory_op,
> +                        cmd, HYPERCALL_BUFFER_AS_ARG(arg));
>  
>      xc_hypercall_bounce_post(xch, arg);
>   out1:
>
>