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

["Jan Beulich" <JBeulich@suse.com>]

> Subject: [PATCH v4 05/35] HYPERCALL_version_op. New hypercall mirroring
>  XENVER_ but sane.
> 
> This hypercall mirrors the XENVER_ in that it has similar functionality.
> However it is designed differently:
>  - No compat layer. The data structures are the same size on 32
>    as on 64-bit.
>  - The hypercall accepts three arguments - the command, pointer to
>    an buffer, and the length of the buffer.
>  - Each sub-ops can be "probed" for size by returning the size of
>    buffer that will be needed - if the buffer is NULL.
>  - Subops can complete even if the buffer is too slow - truncated
>    data will be filled and hypercall will return -ENOBUFS.

s/too slow/too small/ ?

>  - VERSION_OP_commandline, VERSION_OP_changeset are privileged.

Aiui this is no difference to the old one anymore if we assume
patches get committed in the order they're being presented in
this series.

> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -221,6 +221,47 @@ void __init do_initcalls(void)
>  
>  #endif
>  
> +static int get_features(struct domain *d, xen_feature_info_t *fi)
> +{
> +    switch ( fi->submap_idx )
> +    {
> +    case 0:
> +        fi->submap = (1U << XENFEAT_memory_op_vnode_supported);
> +        if ( VM_ASSIST(d, pae_extended_cr3) )
> +            fi->submap |= (1U << XENFEAT_pae_pgdir_above_4gb);

Since you already move this code, I think the two lines above
would better go into the x86-specific section below.

> @@ -302,50 +343,16 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>      case XENVER_get_features:
>      {
>          xen_feature_info_t fi;
> -        struct domain *d = current->domain;
>  
>          if ( copy_from_guest(&fi, arg, 1) )
>              return -EFAULT;
>  
> -        switch ( fi.submap_idx )
> +        if ( !deny )
>          {
> -        case 0:
> -            if ( deny )
> -                break;
> -            fi.submap = (1U << XENFEAT_memory_op_vnode_supported);
> -            if ( VM_ASSIST(d, pae_extended_cr3) )
> -                fi.submap |= (1U << XENFEAT_pae_pgdir_above_4gb);
> -            if ( paging_mode_translate(d) )
> -                fi.submap |= 
> -                    (1U << XENFEAT_writable_page_tables) |
> -                    (1U << XENFEAT_auto_translated_physmap);
> -            if ( is_hardware_domain(d) )
> -                fi.submap |= 1U << XENFEAT_dom0;
> -#ifdef CONFIG_X86
> -            switch ( d->guest_type )
> -            {
> -            case guest_type_pv:
> -                fi.submap |= (1U << XENFEAT_mmu_pt_update_preserve_ad) |
> -                             (1U << XENFEAT_highmem_assist) |
> -                             (1U << XENFEAT_gnttab_map_avail_bits);
> -                break;
> -            case guest_type_pvh:
> -                fi.submap |= (1U << XENFEAT_hvm_safe_pvclock) |
> -                             (1U << XENFEAT_supervisor_mode_kernel) |
> -                             (1U << XENFEAT_hvm_callback_vector);
> -                break;
> -            case guest_type_hvm:
> -                fi.submap |= (1U << XENFEAT_hvm_safe_pvclock) |
> -                             (1U << XENFEAT_hvm_callback_vector) |
> -                             (1U << XENFEAT_hvm_pirqs);
> -                break;
> -            }
> -#endif
> -            break;
> -        default:
> -            return -EINVAL;
> +            int rc = get_features(current->domain, &fi);
> +            if ( rc )

Blank line between declaration(s) and statement(s) please.

> @@ -388,6 +395,182 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) 
> arg)
>      return -ENOSYS;
>  }
>  
> +static const char *capabilities_info(ssize_t *len)

Why ssize_t?

> +{
> +    static xen_capabilities_info_t cached_cap;

__read_mostly?

> +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;

Please combine all the cases producing this value.

> +    case XEN_VERSION_OP_guest_handle:
> +        *sz = ARRAY_SIZE(current->domain->handle);
> +        break;
> +
> +    case XEN_VERSION_OP_commandline:
> +        *sz = ARRAY_SIZE(saved_cmdline);

strlen()?

> +DO(version_op)(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg,
> +               unsigned int len)
> +{
> +    union {
> +        xen_version_op_val_t n;

Would "v" or "val" be the more natural name here?

> +        xen_feature_info_t fi;
> +    } u = {};
> +    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;
> +
> +    ASSERT(sz);
> +    /*
> +     * 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;
> +
> +    /*
> +     * 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!
> +     */
> +
> +    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;
> +    }

Seeing this long switch() a second time I wonder why
size_of_subops_data() doesn't just get folded here, with the null
handle being taken care of below instead of above.

> +    if ( !rc )
> +    {
> +        ssize_t bytes;
> +
> +        if ( sz > len )
> +            bytes = len;
> +        else
> +            bytes = sz;

min() (for which sz and bytes being "unsigned int" would help)?

> +        if ( copy_to_guest(arg, ptr ? : &u, bytes) )
> +            rc = -EFAULT;
> +    }
> +    if ( !rc )
> +    {
> +        /*
> +         * We return len (truncate) worth of data even if we fail.
> +         */
> +        if ( sz > len )
> +            rc = -ENOBUFS;

Perhaps worth moving this up into the previous if(), such that
-EFAULT would also take precedence over -ENOBUFS?

> @@ -87,6 +95,68 @@ typedef struct xen_feature_info xen_feature_info_t;
>  #define XENVER_commandline 9
>  typedef char xen_commandline_t[1024];
>  
> +
> +
> +/*
> + * 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

s/pointer/handle/

> + * 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 void xen_version_op_buf_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_version_op_buf_t);

Are these actually useful for anything? And for the various strings,
wouldn't a "char" handle be more natural?

> +/* arg == version_op_val_t. Encoded as major:minor (31..16:15..0) */

Please make explicit that 63...32 are zero (or whatever they really
are).

> +#define XEN_VERSION_OP_version      0
> +
> +/* arg == version_op_buf. Contains NUL terminated utf-8 string. */
> +#define XEN_VERSION_OP_extraversion 1
> +
> +/* arg == version_op_buf. Contains NUL terminated utf-8 string. */
> +#define XEN_VERSION_OP_capabilities 3
> +
> +/* arg == version_op_buf. Contains NUL terminated utf-8 string. */
> +#define XEN_VERSION_OP_changeset 4
> +
> +/*
> + * arg == xen_version_op_val_t. Contains the virtual address
> + * of the hypervisor encoded as [63..0].

I'd say the encoding info here is unnecessary and could - just like
you already do for pagesize below - be omitted.

> + */
> +#define XEN_VERSION_OP_platform_parameters 5
> +
> +/*
> + * arg = xen_feature_info_t - shares the same structure
> + * as the XENVER_get_features.
> + */
> +#define XEN_VERSION_OP_get_features 6
> +
> +/* arg == xen_version_op_val_t. */
> +#define XEN_VERSION_OP_pagesize 7
> +
> +/* arg == version_op_buf.
> + *
> + * The toolstack fills it out for guest consumption. It is intended to hold
> + * the UUID of the guest.
> + */
> +#define XEN_VERSION_OP_guest_handle 8
> +
> +/* arg = version_op_buf. Contains NUL terminated utf-8 string. */
> +#define XEN_VERSION_OP_commandline 9
> +
>  #endif /* __XEN_PUBLIC_VERSION_H__ */

Would leaving out the _OP and _op everywhere result in any
name collisions with the old one?

Jan

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