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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Thu, Nov 12, 2015 at 09:28:36AM -0700, Jan Beulich wrote:
> >>> On 03.11.15 at 19:15, <ross.lagerwall@citrix.com> wrote:
> > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> > ---
> > 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.

I tried sorting it. I couldn't build the file. And then I put
it on the 'TODO yak-shaving pile' and hadn't yet touched it.

There has to be some automatic tool to help with figuring the
header dependencies for all the headers, not just the ones
I am uisng.
Not exactly sure how to make all of the head
> 
> > +}
> > +
> > +
> > +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?

Duh! Right.
> 
> > +    }
> > +    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.

You did. I hadn't had a chance to address them. Sorry about you
wasting your time and not addressing them in the code yet.

> 
> > --- 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?

<brainfart>
> 
> > +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.

<nods> Thanks for spotting that!
> 
> > --- /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.

Yes, good spoting. The code was written before Andrew's big
keyhandler rewrite and after the rebase I forgot about this.

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