[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v1 02/11] xen/xsplice: Hypervisor implementation of XEN_XSPLICE_op
On Thu, Nov 12, 2015 at 09:28:36AM -0700, Jan Beulich wrote: > >>> 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. 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@xxxxxxxxxxxxx > http://lists.xen.org/xen-devel _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |