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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.03.16 at 21:39, <konrad.wilk@xxxxxxxxxx> wrote:
> @@ -381,6 +389,137 @@ 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;
> +    }
> +
> +    *len = cached_cap_len;
> +    return cached_cap;
> +}

With the init time call to prefill the cache being quite far away, I
think you need a comment here. Even better, though, would be if
you ditched the function altogether and did the prefilling right in
that __init function below, while the consumers of the data would
access the static variables directly. In the end that might even
allow arch_get_xen_caps() to become __init.

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

Single line comment.

> @@ -418,6 +557,21 @@ DO(ni_hypercall)(void)
>      return -ENOSYS;
>  }
>  
> +static int __init kernel_cache_init(void)
> +{
> +    unsigned int len;
> +
> +    /*
> +     * Pre-allocate the cache so we do not have to worry about
> +     * simultaneous invocations on safe_strcat by guests and the cache
> +     * data becoming garbage.
> +     */
> +    (void)capabilities_info(&len);

No need for the cast, afaics.


> +/*
> + * arg == xen_version_op_val_t. Encoded as major:minor (31..16:15..0), while
> + * 63..32 are zero.
> + */
> +#define XEN_VERSION_version             0
> +
> +/* arg == char. Contains NUL terminated utf-8 string. */

I should have noticed this before: "char" isn't really what you mean
here and below. Perhaps better "char[]"?

> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -1658,6 +1658,40 @@ static int flask_xen_version (uint32_t op)
>      }
>  }
>  
> +static int flask_version_op (uint32_t op)
> +{
> +    u32 dsid = domain_sid(current->domain);
> +
> +    switch ( op )
> +    {
> +    case XEN_VERSION_version:
> +    case XEN_VERSION_platform_parameters:
> +    case XEN_VERSION_get_features:
> +        /* These MUST always be accessible to any guest by default. */
> +        return 0;

Perhaps these would better be taken care of in xsm_version_op()?
(That consideration then also applies to the other patch of course.)

Jan


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