|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 01/23] xen/xsplice: Hypervisor implementation of XEN_XSPLICE_op (v10)
On 12/02/16 18:05, Konrad Rzeszutek Wilk wrote:
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index 6f404b4..619aa9e 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -152,4 +152,14 @@ config SCHED_DEFAULT
>
> endmenu
>
> +# Enable/Disable xsplice support
> +config XSPLICE
> + bool "xsplice support"
"XSplice live patching support" ?
> + default y
> + ---help---
> + Allows a running Xen hypervisor to be patched without rebooting.
> + This is primarily used to patch an hypervisor with XSA fixes.
Somewhere in here should use the terms "dynamic" and "binary patching",
to better describe its method of operation.
> +
> + If unsure, say Y.
> +
> endmenu
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 6e82b33..43b3911 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -72,3 +72,5 @@ subdir-$(coverage) += gcov
>
> subdir-y += libelf
> subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt
> +
> +obj-$(CONFIG_XSPLICE) += xsplice.o
Should be part of the main obj- selection higher up.
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index 1624024..68e3eb4 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -28,6 +28,7 @@
> #include <xsm/xsm.h>
> #include <xen/pmstat.h>
> #include <xen/gcov.h>
> +#include <xen/xsplice.h>
>
> long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> {
> @@ -460,6 +461,12 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t)
> u_sysctl)
> ret = tmem_control(&op->u.tmem_op);
> break;
>
> + case XEN_SYSCTL_xsplice_op:
> + ret = xsplice_control(&op->u.xsplice);
Could we name this do_xsplice_op() to match prevailing subop style.
> + if ( ret != -ENOSYS )
> + copyback = 1;
> + break;
> +
Not related to this patch. I (and by this, I mean someone with time ;p)
should do some cleanup and pass copyback by pointer to subops. This
allows for finer grain control of whether a copyback is needed.
> +static const char *state2str(int32_t state)
> +{
> +#define STATE(x) [XSPLICE_STATE_##x] = #x
> + static const char *const names[] = {
> + STATE(LOADED),
> + STATE(CHECKED),
> + STATE(APPLIED),
> + };
> +#undef STATE
> +
> + if (state >= ARRAY_SIZE(names))
> + return "unknown";
> +
> + if (state < 0)
> + return "-EXX";
> +
> + if (!names[state])
> + return "unknown";
This could be folded into the ARRAY_SIZE() check.
> +
> + return names[state];
> +}
> +
> +static void xsplice_printall(unsigned char key)
> +{
> + struct payload *data;
> +
> + spin_lock(&payload_lock);
> +
> + list_for_each_entry ( data, &payload_list, list )
> + printk(" name=%s state=%s(%d)\n", data->name,
> + state2str(data->state), data->state);
> +
> + spin_unlock(&payload_lock);
> +}
> +
> +static int verify_name(xen_xsplice_name_t *name)
const
> +{
> + if ( name->size == 0 || name->size > XEN_XSPLICE_NAME_SIZE )
> + return -EINVAL;
> +
> + if ( name->pad[0] || name->pad[1] || name->pad[2] )
> + return -EINVAL;
> +
> + if ( !guest_handle_okay(name->name, name->size) )
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int find_payload(xen_xsplice_name_t *name, bool_t need_lock,
> + struct payload **f)
> +{
> + struct payload *data;
> + XEN_GUEST_HANDLE_PARAM(char) str;
> + char n[XEN_XSPLICE_NAME_SIZE + 1] = { 0 };
> + int rc = -EINVAL;
> +
> + rc = verify_name(name);
> + if ( rc )
> + return rc;
> +
> + str = guest_handle_cast(name->name, char);
> + if ( copy_from_guest(n, str, name->size) )
> + return -EFAULT;
> +
> + if ( need_lock )
> + spin_lock(&payload_lock);
What is the usecase where the lock shouldn't be taken?
[Edit] From below, its clear that this should be a recursive spinlock.
> +
> + rc = -ENOENT;
> + list_for_each_entry ( data, &payload_list, list )
> + {
> + if ( !strcmp(data->name, n) )
> + {
> + *f = data;
> + rc = 0;
> + break;
> + }
> + }
> +
> + if ( need_lock )
> + spin_unlock(&payload_lock);
> +
> + return rc;
> +}
> +
> +static int verify_payload(xen_sysctl_xsplice_upload_t *upload)
const
> +{
> + if ( verify_name(&upload->name) )
> + return -EINVAL;
> +
> + if ( upload->size == 0 )
> + return -EINVAL;
> +
> + if ( !guest_handle_okay(upload->payload, upload->size) )
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +/*
> + * We MUST be holding the payload_lock spinlock.
In which case ASSERT(spin_is_locked())
> + */
> +static void free_payload(struct payload *data)
> +{
> + list_del(&data->list);
> + payload_cnt--;
> + payload_version++;
> + xfree(data);
> +}
> +
> +static int xsplice_upload(xen_sysctl_xsplice_upload_t *upload)
> +{
> + struct payload *data = NULL;
> + uint8_t *raw_data;
> + int rc;
> +
> + rc = verify_payload(upload);
> + if ( rc )
> + return rc;
> +
> + rc = find_payload(&upload->name, 1 /* true. */, &data);
> + if ( rc == 0 /* Found. */ )
> + return -EEXIST;
> +
> + if ( rc != -ENOENT )
> + return rc;
> +
> + data = xzalloc(struct payload);
> + if ( !data )
> + return -ENOMEM;
> +
> + memset(data, 0, sizeof *data);
xzalloc() has already zeroed data for you.
> + rc = -EFAULT;
> + if ( copy_from_guest(data->name, upload->name.name, upload->name.size) )
> + goto err_data;
> +
> + rc = -ENOMEM;
> + raw_data = alloc_xenheap_pages(get_order_from_bytes(upload->size), 0);
Better to use valloc(), as it won't fail given lots of memory fragmentation.
> + if ( !raw_data )
> + goto err_data;
> +
> + rc = -EFAULT;
> + if ( copy_from_guest(raw_data, upload->payload, upload->size) )
> + goto err_raw;
> +
> + data->state = XSPLICE_STATE_LOADED;
> + data->rc = 0;
> + INIT_LIST_HEAD(&data->list);
> +
> + spin_lock(&payload_lock);
> + list_add_tail(&data->list, &payload_list);
> + payload_cnt++;
> + payload_version++;
> + spin_unlock(&payload_lock);
> +
> + free_xenheap_pages(raw_data, get_order_from_bytes(upload->size));
> + return 0;
> +
> + err_raw:
> + free_xenheap_pages(raw_data, get_order_from_bytes(upload->size));
> + err_data:
> + xfree(data);
It would be cleaner to combine these two err lables into a single err
path. Both free() functions function sensibly with NULL pointers.
> + return rc;
> +}
> +
> +static int xsplice_get(xen_sysctl_xsplice_summary_t *summary)
> +{
> + struct payload *data;
> + int rc;
> +
> + if ( summary->status.state )
> + return -EINVAL;
> +
> + if ( summary->status.rc != 0 )
> + return -EINVAL;
> +
> + rc = verify_name(&summary->name);
> + if ( rc )
> + return rc;
> +
> + rc = find_payload(&summary->name, 1 /* true. */, &data);
> + if ( rc )
> + return rc;
> +
> + summary->status.state = data->state;
> + summary->status.rc = data->rc;
> +
> + return 0;
> +}
> +
> +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;
> +
> + if ( list->nr > 1024 )
> + return -E2BIG;
> +
> + if ( list->pad != 0 )
> + return -EINVAL;
> +
> + if ( !guest_handle_okay(list->status, sizeof(status) * list->nr) ||
> + !guest_handle_okay(list->name, XEN_XSPLICE_NAME_SIZE * list->nr) ||
> + !guest_handle_okay(list->len, sizeof(uint32_t) * list->nr) )
> + return -EINVAL;
> +
> + spin_lock(&payload_lock);
> + if ( list->idx > payload_cnt || !list->nr )
> + {
> + spin_unlock(&payload_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->name);
> +
> + /* N.B. 'idx' != 'i'. */
> + if ( __copy_to_guest_offset(list->name, idx * XEN_XSPLICE_NAME_SIZE,
> + data->name, len) ||
> + __copy_to_guest_offset(list->len, idx, &len, 1) ||
> + __copy_to_guest_offset(list->status, idx, &status, 1) )
> + {
> + rc = -EFAULT;
> + break;
> + }
> + idx++;
Some extra newlines around here please.
> + if ( hypercall_preempt_check() || (idx + 1 > list->nr) )
> + break;
> + }
> + list->nr = payload_cnt - i; /* Remaining amount. */
> + list->version = payload_version;
> + spin_unlock(&payload_lock);
> +
> + /* And how many we have processed. */
> + return rc ? : idx;
> +}
> +
> +static int xsplice_action(xen_sysctl_xsplice_action_t *action)
> +{
> + struct payload *data;
> + int rc;
> +
> + rc = verify_name(&action->name);
> + if ( rc )
> + return rc;
> +
> + spin_lock(&payload_lock);
> + rc = find_payload(&action->name, 0 /* We are holding the lock. */,
> &data);
Looks like payload_lock should be a recursive lock. Please do that,
rather than risk accessing locked data without the lock held at all.
> + if ( rc )
> + goto out;
> +
> + switch ( action->cmd )
> + {
> + case XSPLICE_ACTION_CHECK:
> + if ( (data->state == XSPLICE_STATE_LOADED) ||
> + (data->state == XSPLICE_STATE_CHECKED) )
> + {
> + /* No implementation yet. */
> + data->state = XSPLICE_STATE_CHECKED;
> + data->rc = 0;
> + rc = 0;
> + }
> + break;
Newlines between break and case statements please.
> + case XSPLICE_ACTION_UNLOAD:
> + if ( (data->state == XSPLICE_STATE_LOADED) ||
> + (data->state == XSPLICE_STATE_CHECKED) )
> + {
> + free_payload(data);
> + /* No touching 'data' from here on! */
> + rc = 0;
> + }
> + break;
> + case XSPLICE_ACTION_REVERT:
> + if ( data->state == XSPLICE_STATE_APPLIED )
> + {
> + /* No implementation yet. */
> + data->state = XSPLICE_STATE_CHECKED;
> + data->rc = 0;
> + rc = 0;
> + }
> + break;
> + case XSPLICE_ACTION_APPLY:
> + if ( (data->state == XSPLICE_STATE_CHECKED) )
> + {
> + /* No implementation yet. */
> + data->state = XSPLICE_STATE_APPLIED;
> + data->rc = 0;
> + rc = 0;
> + }
> + break;
> + case XSPLICE_ACTION_REPLACE:
> + if ( data->state == XSPLICE_STATE_CHECKED )
> + {
> + /* No implementation yet. */
> + data->state = XSPLICE_STATE_CHECKED;
> + data->rc = 0;
> + rc = 0;
> + }
> + break;
> + default:
> + rc = -EOPNOTSUPP;
> + break;
> + }
> +
> + out:
> + spin_unlock(&payload_lock);
> +
> + return rc;
> +}
> +
> +int xsplice_control(xen_sysctl_xsplice_op_t *xsplice)
> +{
> + int rc;
> +
> + if ( xsplice->pad != 0 )
> + return -EINVAL;
> +
> + switch ( xsplice->cmd )
> + {
> + case XEN_SYSCTL_XSPLICE_UPLOAD:
> + rc = xsplice_upload(&xsplice->u.upload);
> + break;
Newlines for these as well please.
> + case XEN_SYSCTL_XSPLICE_GET:
> + rc = xsplice_get(&xsplice->u.get);
> + break;
> + case XEN_SYSCTL_XSPLICE_LIST:
> + rc = xsplice_list(&xsplice->u.list);
> + break;
> + case XEN_SYSCTL_XSPLICE_ACTION:
> + rc = xsplice_action(&xsplice->u.action);
> + break;
> + default:
> + rc = -EOPNOTSUPP;
> + break;
> + }
> +
> + return rc;
> +}
> +
> +static int __init xsplice_init(void)
> +{
> + register_keyhandler('x', xsplice_printall, "print xsplicing info", 1);
> + return 0;
> +}
> +__initcall(xsplice_init);
Local variable block please.
> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> index 96680eb..d549e7a 100644
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -766,6 +766,160 @@ 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 http://xenbits.xenproject.org/docs/unstable/misc/xsplice.html
I would refer to the file in the source tree, so docs/misc/xsplice.$FOO
which is far less likely to change.
> + * for the design details of this hyprcall.
> + */
> +
> +/*
> + * Structure describing an ELF payload. Uniquely identifies the
> + * payload. Should be human readable.
> + * Recommended length is upto XEN_XSPLICE_NAME_SIZE.
> + */
> +#define XEN_XSPLICE_NAME_SIZE 128
> +struct xen_xsplice_name {
> + XEN_GUEST_HANDLE_64(char) name; /* IN: pointer to name. */
> + uint16_t size; /* IN: size of name. May be upto
> + XEN_XSPLICE_NAME_SIZE. */
> + uint16_t pad[3]; /* IN: MUST be zero. */
> +};
> +typedef struct xen_xsplice_name xen_xsplice_name_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_xsplice_name_t);
> +
> +/*
> + * Upload a payload to the hypervisor. The payload is verified
> + * against basic checks and if there are any issues the proper return code
> + * will be returned. The payload is not applied at this time - that is
> + * controlled by XEN_SYSCTL_XSPLICE_ACTION.
> + *
> + * The return value is zero if the payload was succesfully uploaded.
> + * Otherwise an EXX return value is provided. Duplicate `name` are not
> + * supported.
I would recommend having a full state diagram in the xsplice
documentation and referring to that, rather than having half a "but not
this yet" set of comments in the header file.
> + *
> + * The payload at this point is verified against the basic checks.
> + *
> + * The `payload` is the ELF payload as mentioned in the `Payload format`
> + * section in the xSplice design document.
> + */
> +#define XEN_SYSCTL_XSPLICE_UPLOAD 0
> +struct xen_sysctl_xsplice_upload {
> + xen_xsplice_name_t name; /* IN, name of the patch. */
> + uint64_t size; /* IN, size of the ELF file. */
> + XEN_GUEST_HANDLE_64(uint8) payload; /* IN, the ELF file. */
> +};
> +typedef struct xen_sysctl_xsplice_upload xen_sysctl_xsplice_upload_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_sysctl_xsplice_upload_t);
> +
> +/*
> + * Retrieve an status of an specific payload.
> + *
> + * Upon completion the `struct xen_xsplice_status` is updated.
> + *
> + * The return value is zero on success and XEN_EXX on failure. This operation
> + * is synchronous and does not require preemption.
> + */
> +#define XEN_SYSCTL_XSPLICE_GET 1
> +
> +struct xen_xsplice_status {
> +#define XSPLICE_STATE_LOADED 1
> +#define XSPLICE_STATE_CHECKED 2
> +#define XSPLICE_STATE_APPLIED 3
> + int32_t state; /* OUT: XSPLICE_STATE_*. IN: MUST be
> zero. */
> + int32_t rc; /* OUT: 0 if no error, otherwise
> -XEN_EXX. */
> + /* IN: MUST be zero. */
> +};
> +typedef struct xen_xsplice_status xen_xsplice_status_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_xsplice_status_t);
> +
> +struct xen_sysctl_xsplice_summary {
> + xen_xsplice_name_t name; /* IN, name of the payload. */
> + xen_xsplice_status_t status; /* IN/OUT, state of it. */
> +};
> +typedef struct xen_sysctl_xsplice_summary xen_sysctl_xsplice_summary_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_sysctl_xsplice_summary_t);
> +
> +/*
> + * Retrieve an array of abbreviated status and names of payloads that are
> + * loaded in the hypervisor.
> + *
> + * If the hypercall returns an positive number, it is the number (up to `nr`)
> + * of the payloads returned, along with `nr` updated with the number of
> remaining
> + * payloads, `version` updated (it may be the same across hypercalls. If it
> + * varies the data is stale and further calls could fail). The `status`,
> + * `name`, and `len`' are updated at their designed index value (`idx`) with
> + * the returned value of data.
> + *
> + * If the hypercall returns E2BIG the `nr` is too big and should be
> + * lowered.
What would cause this situation to occur?
> + *
> + * This operation can be preempted by the hypercall returning EAGAIN.
> + * Retry.
Again, why is this necessary or useful?
> + *
> + * Note that due to the asynchronous nature of hypercalls the domain might
> have
> + * added or removed the number of payloads making this information stale. It
> is
> + * the responsibility of the toolstack to use the `version` field to check
> + * between each invocation. if the version differs it should discard the
> stale
> + * data and start from scratch. It is OK for the toolstack to use the new
> + * `version` field.
> + */
> +#define XEN_SYSCTL_XSPLICE_LIST 2
> +struct xen_sysctl_xsplice_list {
> + uint32_t version; /* IN/OUT: Initially *MUST* be
> zero.
> + On subsequent calls reuse
> value.
> + If varies between calls, we
> are
> + * getting stale data. */
> + uint32_t idx; /* IN/OUT: Index into array. */
> + uint32_t nr; /* IN: How many status, name,
> and len
> + should fill out.
> + OUT: How many payloads left.
> */
> + uint32_t pad; /* IN: Must be zero. */
> + XEN_GUEST_HANDLE_64(xen_xsplice_status_t) status; /* OUT. Must have
> enough
> + space allocate for nr of
> them. */
> + XEN_GUEST_HANDLE_64(char) name; /* OUT: Array of names. Each
> member
> + MUST XEN_XSPLICE_NAME_SIZE in
> size.
> + Must have nr of them. */
> + XEN_GUEST_HANDLE_64(uint32) len; /* OUT: Array of lengths of
> name's.
> + Must have nr of them. */
> +};
> +typedef struct xen_sysctl_xsplice_list xen_sysctl_xsplice_list_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_sysctl_xsplice_list_t);
> +
> +/*
> + * Perform an operation on the payload structure referenced by the `name`
> field.
> + * The operation request is asynchronous and the status should be retrieved
> + * by using either XEN_SYSCTL_XSPLICE_GET or XEN_SYSCTL_XSPLICE_LIST
> hypercall.
> + */
> +#define XEN_SYSCTL_XSPLICE_ACTION 3
> +struct xen_sysctl_xsplice_action {
> + xen_xsplice_name_t name; /* 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 timeout; /* IN: Zero if no timeout. */
> + /* Or upper bound of time (ms) */
> + /* for operation to take. */
> +};
> +typedef struct xen_sysctl_xsplice_action xen_sysctl_xsplice_action_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_sysctl_xsplice_action_t);
> +
> +struct xen_sysctl_xsplice_op {
> + uint32_t cmd; /* IN: XEN_SYSCTL_XSPLICE_*. */
> + uint32_t pad; /* IN: Always zero. */
> + union {
> + xen_sysctl_xsplice_upload_t upload;
> + xen_sysctl_xsplice_list_t list;
> + xen_sysctl_xsplice_summary_t get;
> + xen_sysctl_xsplice_action_t action;
> + } u;
> +};
> +typedef struct xen_sysctl_xsplice_op xen_sysctl_xsplice_op_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_sysctl_xsplice_op_t);
> +
> struct xen_sysctl {
> uint32_t cmd;
> #define XEN_SYSCTL_readconsole 1
> @@ -791,6 +945,7 @@ struct xen_sysctl {
> #define XEN_SYSCTL_pcitopoinfo 22
> #define XEN_SYSCTL_psr_cat_op 23
> #define XEN_SYSCTL_tmem_op 24
> +#define XEN_SYSCTL_xsplice_op 25
> uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
> union {
> struct xen_sysctl_readconsole readconsole;
> @@ -816,6 +971,7 @@ struct xen_sysctl {
> struct xen_sysctl_psr_cmt_op psr_cmt_op;
> struct xen_sysctl_psr_cat_op psr_cat_op;
> struct xen_sysctl_tmem_op tmem_op;
> + struct xen_sysctl_xsplice_op xsplice;
> uint8_t pad[128];
> } u;
> };
> diff --git a/xen/include/xen/xsplice.h b/xen/include/xen/xsplice.h
> new file mode 100644
> index 0000000..cf465c4
> --- /dev/null
> +++ b/xen/include/xen/xsplice.h
> @@ -0,0 +1,15 @@
> +#ifndef __XEN_XSPLICE_H__
> +#define __XEN_XSPLICE_H__
> +
> +struct xen_sysctl_xsplice_op;
> +
> +#ifdef CONFIG_XSPLICE
No reason for this all to be squashed completely together.
> +int xsplice_control(struct xen_sysctl_xsplice_op *);
> +#else
> +#include <xen/errno.h> /* For -ENOSYS */
> +static inline int xsplice_control(struct xen_sysctl_xsplice_op *op)
> +{
> + return -ENOSYS;
> +}
> +#endif
> +#endif /* __XEN_XSPLICE_H__ */
Variable block please.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |