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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.03.16 at 20:22, <konrad.wilk@xxxxxxxxxx> wrote:
>> > + * 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,
> 
> The xen_version_op_val_t is definitly used by the toolstack.
> 
>> wouldn't a "char" handle be more natural?
> 
> Heh. It was char[] before but Andrew liked it as void.

But that was because you used it for non string types too,
wasn't it?

> @@ -380,6 +388,133 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) 
> arg)
>      return -ENOSYS;
>  }
>  
> +static const char *capabilities_info(unsigned int *len)
> +{
> +    static xen_capabilities_info_t __read_mostly cached_cap;
> +    static unsigned int __read_mostly cached_cap_len;
> +    static bool_t cached;
> +
> +    if ( unlikely(!cached) )
> +    {
> +        arch_get_xen_caps(&cached_cap);
> +        cached_cap_len = strlen(cached_cap) + 1;
> +        cached = 1;
> +    }

I'm sorry for noticing this only now, but without any locking this is
unsafe: x86's arch_get_xen_caps() using safe_strcat() to fill the
buffer, simultaneous invocations would possibly produce garbled
output to all (i.e. also subsequently started) guests. Either use a
real lock here, or make the guard a tristate one, which gets
transitioned e.g. from 0 to -1 by the first one coming here (doing
the initialization), with everyone else waiting for it to become +1
(to which the initializing party sets it once it is done).

> +DO(version_op)(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg,
> +               unsigned int len)
> +{
> +    union {
> +        xen_version_op_val_t val;
> +        xen_feature_info_t fi;
> +    } u = {};
> +    unsigned int sz = 0;
> +    const void *ptr = NULL;
> +    int rc = xsm_version_op(XSM_OTHER, cmd);
> +
> +    /* We can safely return -EPERM! */
> +    if ( rc )
> +        return rc;
> +
> +    /*
> +     * 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_version:
> +        sz = sizeof(xen_version_op_val_t);
> +        u.val = (xen_major_version() << 16) | xen_minor_version();
> +        break;
> +
> +    case XEN_VERSION_extraversion:
> +        sz = strlen(xen_extra_version()) + 1;
> +        ptr = xen_extra_version();
> +        break;
> +
> +    case XEN_VERSION_capabilities:
> +        ptr = capabilities_info(&sz);
> +        break;
> +
> +    case XEN_VERSION_changeset:
> +        sz = strlen(xen_changeset()) + 1;
> +        ptr = xen_changeset();
> +        break;
> +
> +    case XEN_VERSION_platform_parameters:
> +        sz = sizeof(xen_version_op_val_t);
> +        u.val = HYPERVISOR_VIRT_START;
> +        break;
> +
> +    case XEN_VERSION_get_features:
> +        if ( copy_from_guest(&u.fi, arg, 1) )

Afaict this is incompatible with the null handle check further down (i.e.
you also need to check for a null handle here).

> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -128,6 +128,9 @@
>   *    * VCPUOP_register_vcpu_info
>   *    * VCPUOP_register_runstate_memory_area
>   *
> + *  HYPERVISOR_version_op
> + *   All generic sub-operations
> + *
>   *
>   * Other notes on the ARM ABI:

I don't think the extra almost blank line is warranted here.

> --- a/xen/include/public/version.h
> +++ b/xen/include/public/version.h
> @@ -30,7 +30,15 @@
>  
>  #include "xen.h"
>  
> -/* NB. All ops return zero on success, except XENVER_{version,pagesize} */
> +/*
> + * There are two hypercalls mentioned in here. The XENVER_ are for
> + * HYPERCALL_xen_version (17), while VERSION_ are for the
> + * HYPERCALL_version_op (41).
> + *
> + * The subops are very similar except that the later hypercall has a
> + * sane interface.
> + */
> +
>  
>  /* arg == NULL; returns major:minor (16:16). */

Nor is the extra blank one here.

> @@ -87,6 +95,66 @@ 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

And three consecutive blank lines are too much in any event. (If
for no other reason that because that provides extremely bad
patch context if a later change happened right next to these three
lines.)

> +/*
> + * arg == char.
> + *
> + * The toolstack fills it out for guest consumption. It is intended to hold
> + * the UUID of the guest.
> + */
> +#define XEN_VERSION_guest_handle        8

So this is the place where I agree with Andrew char is not an
appropriate type. A void or uint8 handle seems like what you
want here.

> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -751,3 +751,22 @@ static XSM_INLINE int xsm_xen_version (XSM_DEFAULT_ARG 
> uint32_t op)
>          return xsm_default_action(XSM_PRIV, current->domain, NULL);
>      }
>  }
> +
> +static XSM_INLINE int xsm_version_op (XSM_DEFAULT_ARG uint32_t op)
> +{
> +    XSM_ASSERT_ACTION(XSM_OTHER);
> +    switch ( op )
> +    {
> +    case XEN_VERSION_version:
> +    case XEN_VERSION_extraversion:
> +    case XEN_VERSION_capabilities:
> +    case XEN_VERSION_platform_parameters:
> +    case XEN_VERSION_get_features:
> +    case XEN_VERSION_pagesize:
> +    case XEN_VERSION_guest_handle:
> +        /* These MUST always be accessible to any guest by default. */
> +        return xsm_default_action(XSM_HOOK, current->domain, NULL);
> +    default:
> +        return xsm_default_action(XSM_PRIV, current->domain, NULL);

Considering that we seem to have settled on some exceptions here
for the change adding XSM check to the legacy version op, do you
really think going with no exception at all here is the right approach?
Because if we do, that'll prevent guests getting fully converted over
to the new interface. Of course, we could also make this conversion
specifically a non-goal, and omit e.g. XEN_VERSION_VERSION from
this new interface.

Jan

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