Re: [Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.03.16 at 22:30, <konrad@xxxxxxxxxx> wrote:
> From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Date: Tue, 22 Mar 2016 16:53:19 -0400
> Subject: [PATCH] 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 small - truncated
>    data will be filled and hypercall will return -ENOBUFS.
>  - VERSION_commandline, VERSION_changeset are privileged.
>  - There is no XENVER_compile_info equivalent.
>  - The hypercall can return -EPERM and toolstack/OSes are expected
>    to deal with. However there are three subops: XEN_VERSION_version,
>    XEN_VERSION_platform_parameters and XEN_VERSION_get_features
>    that will always return an value as guests cannot survive without them.
> 
> While we combine some of the common code between XENVER_ and VERSION_
> take the liberty of moving pae_extended_cr3 in x86 area.
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Acked-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx> [XSM bits]
> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

This last line doesn't seem to match up with ...

> v1-v3: Was not part of the series.
> v4: New posting.
> v5: Remove memset and use {}. Tweak copy_to_guest and capabilities_info,
>     add ASSERT(sz) per Andrew's review. Add cached=1 back in.
>     Per Jan, s/VERSION_OP/VERSION/, squash size check with do_version_op,
>     update the comments. Dropped Andrew's Review-by. Ate newlines.

... the middle sentence here.

> +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! */

So what again was this comment supposed to tell us?

> +    if ( rc )
> +        return rc;
> +
> +    /*
> +     * The HYPERVISOR_xen_version differs in that some return the value,

I think there's a "subop" missing somewhere.

> +static int __init capabilities_cache_init(void)
> +{
> +    /*
> +     * Pre-allocate the cache so we do not have to worry about

"Pre-fill" or "Pre-populate"?


> --- a/xen/include/public/version.h
> +++ b/xen/include/public/version.h
> @@ -30,7 +30,14 @@
>  
>  #include "xen.h"
>  
> -/* NB. All ops return zero on success, except XENVER_{version,pagesize} */

Perhaps this comment doesn't belong here anymore, but I don't think
it should be deleted altogether.

> +/*
> + * There are two hypercalls mentioned in here. The XENVER_ are for
> + * HYPERCALL_xen_version (17), while VERSION_ are for the

XEN_VERSION_

> @@ -87,6 +94,67 @@ 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.

"returns ... returned" is a little strange. Perhaps "returns ... copied"?

> + *  - It will return -XEN_EPERM if the guest is not permitted
> + *    (Albeit XEN_VERSION_version, XEN_VERSION_platform_parameters, and
> + *    XEN_VERSION_get_features will always return an value as guest cannot
> + *    survive without this information).

Isn't there an object missing here, to say what 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

... a mechanism ...

> + * sub-op will require. Having the arg have an NULL handle will

... a NULL ...

> + * return the number of bytes requested for the operation. Or an
> + * negative value if an error is encountered.

... a negative ...

Since they're all cosmetic, if you take care of all of them, feel free
to stick my ack on the result.

Jan

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