Re: [Xen-devel] [PATCH v1 02/11] xen/xsplice: Hypervisor implementation of XEN_XSPLICE_op

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.11.15 at 19:15, <ross.lagerwall@xxxxxxxxxx> wrote:
> Signed-off-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> ---
> v2: Rebased on keyhandler: rework keyhandler infrastructure
> v3: Fixed XSM.
> v4: Removed REVERTED state.
>     Split status and error code.
>     Add REPLACE action.
>     Separate payload data from the payload structure.
>     s/XSPLICE_ID_../XSPLICE_NAME_../

Odd - the subject says v1.

> --- /dev/null
> +++ b/xen/common/xsplice.c
> @@ -0,0 +1,398 @@
> +/*
> + * Copyright (c) 2015 Oracle and/or its affiliates. All rights reserved.
> + *
> + */
> +
> +#include <xen/smp.h>
> +#include <xen/keyhandler.h>
> +#include <xen/spinlock.h>
> +#include <xen/mm.h>
> +#include <xen/list.h>
> +#include <xen/guest_access.h>
> +#include <xen/stdbool.h>

No use of this header except in code shared with the tools.

Also I'd like to encourage you to sort all xen/ headers in a file
(and all public/ and asm/ ones, just that this doesn't apply here)
alphabetically in new code.

> +}
> +
> +
> +static int verify_payload(xen_sysctl_xsplice_upload_t *upload)

Double blank line above.

> +/*
> + * We MUST be holding the spinlock.
> + */

Which spinlock? Also this is a single line comment.

> +static void __free_payload(struct payload *data)

I see no reason for this function to have two underscores at the
beginning of its name.


> +err_raw:
> +    free_xenheap_pages(raw_data, get_order_from_bytes(upload->size));
> +err_data:

Labels indented by at least one blank please.

> +static int xsplice_list(xen_sysctl_xsplice_list_t *list)
> +{
> +    xen_xsplice_status_t status;
> +    struct payload *data;
> +    unsigned int idx = 0, i = 0;
> +    int rc = 0;
> +    unsigned int ver = payload_version;
> +
> +    if ( list->nr > 1024 )
> +        return -E2BIG;
> +
> +    if ( list->pad != 0 )
> +        return -EINVAL;
> +
> +    if ( guest_handle_is_null(list->status) ||
> +         guest_handle_is_null(list->id) ||
> +         guest_handle_is_null(list->len) )
> +        return -EINVAL;

???

> +    if ( !guest_handle_okay(list->status, sizeof(status) * list->nr) ||
> +         !guest_handle_okay(list->id, XEN_XSPLICE_NAME_SIZE * list->nr) ||
> +         !guest_handle_okay(list->len, sizeof(uint32_t) * list->nr) )
> +        return -EINVAL;
> +
> +    spin_lock(&payload_list_lock);
> +    if ( list->idx > payload_cnt )
> +    {
> +        spin_unlock(&payload_list_lock);
> +        return -EINVAL;
> +    }
> +
> +    list_for_each_entry( data, &payload_list, list )
> +    {
> +        uint32_t len;
> +
> +        if ( list->idx > i++ )
> +            continue;
> +
> +        status.state = data->state;
> +        status.rc = data->rc;
> +        len = strlen(data->id);
> +
> +        /* N.B. 'idx' != 'i'. */
> +        if ( copy_to_guest_offset(list->id, idx * XEN_XSPLICE_NAME_SIZE,
> +                                  data->id, len) ||
> +             copy_to_guest_offset(list->len, idx, &len, 1) ||
> +             copy_to_guest_offset(list->status, idx, &status, 1) )

You having used guest_handle_okay() above, all of these can be
__copy_to_guest_offset)() afaict.

> +        {
> +            rc = -EFAULT;
> +            break;
> +        }
> +        idx ++;

Spurious blank.

> +        if ( hypercall_preempt_check() || (idx + 1 > list->nr) )
> +        {
> +            break;
> +        }

Pointless braces.

Also - what about an input list->nr of zero?

> +    }
> +    list->nr = payload_cnt - i; /* Remaining amount. */
> +    spin_unlock(&payload_list_lock);
> +    list->version = ver;
> +
> +    /* And how many we have processed. */
> +    return rc ? rc : idx;

Please omit the middle expression in cases like this. To be honest I
can't help myself thinking that I'v already made at least some of
these comments.

> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -766,6 +766,161 @@ struct xen_sysctl_tmem_op {
>  typedef struct xen_sysctl_tmem_op xen_sysctl_tmem_op_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_tmem_op_t);
>  
> +/*
> + * XEN_SYSCTL_XSPLICE_op
> + *
> + * Refer to the docs/misc/xsplice.markdown for the design details
> + * of this hypercall.

To someone importing this header into another project, this
reference may be quite odd. Don't these get translated to
some html with a canonical place on the web?

> +struct xen_sysctl_xsplice_action {
> +    xen_xsplice_id_t id;                    /* IN, name of the patch. */
> +#define XSPLICE_ACTION_CHECK        1
> +#define XSPLICE_ACTION_UNLOAD       2
> +#define XSPLICE_ACTION_REVERT       3
> +#define XSPLICE_ACTION_APPLY        4
> +#define XSPLICE_ACTION_REPLACE      5
> +    uint32_t    cmd;                        /* IN: XSPLICE_ACTION_*. */
> +    uint32_t    pad;                        /* IN: MUST be zero. */
> +    uint64_aligned_t time;                  /* IN: Zero if no timeout. */
> +                                            /* Or upper bound of time (ms) */
> +                                            /* for operation to take. */

So if the field represent a timeout, why not call it such?

> +struct xen_sysctl_xsplice_op {
> +    uint32_t cmd;                           /* IN: XEN_SYSCTL_XSPLICE_* */
> +    uint32_t pad;                           /* IN: Always zero. */

Other "pad" fields get checked to be zero, but I wasn't able to
spot a check for this one.

> --- /dev/null
> +++ b/xen/include/xen/xsplice.h
> @@ -0,0 +1,9 @@
> +#ifndef __XEN_XSPLICE_H__
> +#define __XEN_XSPLICE_H__
> +
> +struct xen_sysctl_xsplice_op;
> +int xsplice_control(struct xen_sysctl_xsplice_op *);
> +
> +extern void xsplice_printall(unsigned char key);

What is this declaration good for? The function ought to be static
afaics.

Jan

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