Re: [Xen-devel] [PATCH v4 04/34] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/03/16 17:56, Konrad Rzeszutek Wilk wrote:
> @@ -388,6 +395,188 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) 
> arg)
>      return -ENOSYS;
>  }
>  
> +static const char *capabilities_info(ssize_t *len)
> +{
> +    static xen_capabilities_info_t cached_cap;
> +    static unsigned int cached_cap_len;
> +    static bool_t cached;
> +
> +    if ( cached )

I am surprised that Coverity didn't complain about this being unused...

> +    {
> +        *len = cached_cap_len;
> +        return cached_cap;
> +    }
> +    arch_get_xen_caps(&cached_cap);
> +    cached_cap_len = strlen(cached_cap) + 1;
> +
> +    *len = cached_cap_len;
> +    return cached_cap;

You can turn the logic around as

if ( unliklely(!cached) )
{
    arch_get_xen_caps(&cached_cap);
    cached_cap_len = strlen(cached_cap) + 1;
    cached = 1;
}

and have a single return path.

> +}
> +
> +static int size_of_subops_data(unsigned int cmd, ssize_t *sz)
> +{
> +    int rc = 0;
> +    /* Compute size. */
> +    switch ( cmd )
> +    {
> +    case XEN_VERSION_OP_version:
> +        *sz = sizeof(xen_version_op_val_t);
> +        break;
> +
> +    case XEN_VERSION_OP_extraversion:
> +        *sz = strlen(xen_extra_version()) + 1;
> +        break;
> +
> +    case XEN_VERSION_OP_capabilities:
> +        capabilities_info(sz);
> +        break;
> +
> +    case XEN_VERSION_OP_platform_parameters:
> +        *sz = sizeof(xen_version_op_val_t);
> +        break;
> +
> +    case XEN_VERSION_OP_changeset:
> +        *sz = strlen(xen_changeset()) + 1;
> +        break;
> +
> +    case XEN_VERSION_OP_get_features:
> +        *sz = sizeof(xen_feature_info_t);
> +        break;
> +
> +    case XEN_VERSION_OP_pagesize:
> +        *sz = sizeof(xen_version_op_val_t);
> +        break;
> +
> +    case XEN_VERSION_OP_guest_handle:
> +        *sz = ARRAY_SIZE(current->domain->handle);
> +        break;
> +
> +    case XEN_VERSION_OP_commandline:
> +        *sz = ARRAY_SIZE(saved_cmdline);
> +        break;
> +
> +    default:
> +        rc = -ENOSYS;
> +    }
> +
> +    return rc;
> +}
> +
> +/*
> + * Similar to HYPERVISOR_xen_version but with a sane interface
> + * (has a length, one can probe for the length) and with one less sub-ops:
> + * missing XENVER_compile_info.
> + */
> +DO(version_op)(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg,
> +               unsigned int len)
> +{
> +    union {
> +        xen_version_op_val_t n;
> +        xen_feature_info_t fi;
> +    } u;

= {}; and you can forgo the explicit memset() below.

> +    ssize_t sz = 0;
> +    const void *ptr = NULL;
> +    int rc = xsm_version_op(XSM_OTHER, cmd);
> +
> +    /* We can safely return -EPERM! */
> +    if ( rc )
> +        return rc;
> +
> +    rc = size_of_subops_data(cmd, &sz);
> +    if ( rc )
> +        return rc;
> +
> +    /* Some of the subops may have no data. */
> +    if ( !sz )
> +        return 0;

Really? I would have thought it would be reasonable to assert that
either sz != 0 after the rc != 0 return.

> +    /*
> +     * This hypercall also allows the client to probe. If it provides
> +     * a NULL arg we will return the size of the space it has to
> +     * allocate for the specific sub-op.
> +     */
> +    if ( guest_handle_is_null(arg) )
> +        return sz;
> +
> +    memset(&u, 0, sizeof(u));
> +    /*
> +     * The HYPERVISOR_xen_version differs in that some return the value,
> +     * and some copy it on back on argument. We follow the same rule for all
> +     * sub-ops: return 0 on success, positive value of bytes returned, and
> +     * always copy the result in arg. Yeey sanity!
> +     */
> +
> +    rc = 0;

rc is guaranteed to be 0 at this point.

> +    switch ( cmd )
> +    {
> +    case XEN_VERSION_OP_version:
> +        u.n = (xen_major_version() << 16) | xen_minor_version();
> +        break;
> +
> +    case XEN_VERSION_OP_extraversion:
> +        ptr = xen_extra_version();
> +        break;
> +
> +    case XEN_VERSION_OP_capabilities:
> +        ptr = capabilities_info(&sz);
> +        break;
> +
> +    case XEN_VERSION_OP_platform_parameters:
> +        u.n = HYPERVISOR_VIRT_START;
> +        break;
> +
> +    case XEN_VERSION_OP_changeset:
> +        ptr = xen_changeset();
> +        break;
> +
> +    case XEN_VERSION_OP_get_features:
> +        if ( copy_from_guest(&u.fi, arg, 1) )
> +        {
> +            rc = -EFAULT;
> +            break;
> +        }
> +        rc = get_features(current->domain, &u.fi);
> +        break;
> +
> +    case XEN_VERSION_OP_pagesize:
> +        u.n = PAGE_SIZE;
> +        break;
> +
> +    case XEN_VERSION_OP_guest_handle:
> +        ptr = current->domain->handle;
> +        break;
> +
> +    case XEN_VERSION_OP_commandline:
> +        ptr = saved_cmdline;
> +        break;
> +
> +    default:
> +        rc = -ENOSYS;
> +    }
> +
> +    if ( !rc )
> +    {
> +        ssize_t bytes;
> +
> +        if ( sz > len )
> +            bytes = len;
> +        else
> +            bytes = sz;
> +
> +        if ( copy_to_guest(arg, ptr ? ptr : &u, bytes) )

Can be shortened to ptr ?: &u

> +            rc = -EFAULT;
> +    }
> +    if ( !rc )
> +    {
> +        /*
> +         * We return len (truncate) worth of data even if we fail.
> +         */
> +        if ( sz > len )
> +            rc = -ENOBUFS;

This needs to be in the previous if() clause to avoid overriding -EFAULT
with -ENOBUFS.

> +
> +/*
> + * The HYPERCALL_version_op has a set of sub-ops which mirror the
> + * sub-ops of HYPERCALL_xen_version. However this hypercall differs
> + * radically from the former:
> + *  - It returns the amount of bytes returned.
> + *  - It will return -XEN_EPERM if the guest is not permitted.
> + *  - It will return the requested data in arg.
> + *  - It requires an third argument (len) for the length of the
> + *    arg. Naturally the arg has to fit the requested data otherwise
> + *    -XEN_ENOBUFS is returned.
> + *
> + * It also offers an mechanism to probe for the amount of bytes an
> + * sub-op will require. Having the arg have an NULL pointer will
> + * return the number of bytes requested for the operation. Or an
> + * negative value if an error is encountered.
> + */
> +
> +typedef uint64_t xen_version_op_val_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_version_op_val_t);
> +
> +typedef unsigned char xen_version_op_buf_t[];
> +DEFINE_XEN_GUEST_HANDLE(xen_version_op_buf_t);

Strictly speaking this should be a void* guest handle, as not all data
is returned via this mechanism is unsigned char.

> +
> +/* arg == version_op_val_t. Encoded as major:minor (31..16:15..0) */
> +#define XEN_VERSION_OP_version      0
> +
> +/* arg == version_op_buf. */
> +#define XEN_VERSION_OP_extraversion 1
> +
> +/* arg == version_op_buf */
> +#define XEN_VERSION_OP_capabilities 3
> +
> +/* arg == version_op_buf */
> +#define XEN_VERSION_OP_changeset 4

Might be worth stating that these return NUL terminated utf-8 strings?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel