|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v1 08/11] xsplice: Implement support for applying patches
On Tue, Nov 03, 2015 at 06:16:05PM +0000, Ross Lagerwall wrote:
> Implement support for the apply, revert and replace actions.
>
> To perform and action on a payload, the hypercall sets up a data
> structure to schedule the work. A hook is added in all the
> return-to-guest paths to check for work to do and execute it if needed.
> In this way, patches can be applied with all CPUs idle and without
> stacks. The first CPU to do_xsplice() becomes the master and triggers a
> reschedule softirq to trigger all the other CPUs to enter do_xsplice()
> with no stack. Once all CPUs have rendezvoused, all CPUs disable IRQs
> and NMIs are ignored. The system is then quiscient and the master
> performs the action. After this, all CPUs enable IRQs and NMIs are
> re-enabled.
>
> The action to perform is one of:
> - APPLY: For each function in the module, store the first 5 bytes of the
> old function and replace it with a jump to the new function.
> - REVERT: Copy the previously stored bytes into the first 5 bytes of the
> old function.
> - REPLACE: Revert each applied module and then apply the new module.
>
> To prevent a deadlock with any other barrier in the system, the master
> will wait for up to 30ms before timing out. I've taken some
> measurements and found the patch application to take about 100 Îs on a
> 72 CPU system, whether idle or fully loaded.
>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> ---
> xen/arch/arm/xsplice.c | 8 ++
> xen/arch/x86/domain.c | 4 +
> xen/arch/x86/hvm/svm/svm.c | 2 +
> xen/arch/x86/hvm/vmx/vmcs.c | 2 +
> xen/arch/x86/xsplice.c | 19 ++++
> xen/common/xsplice.c | 264
> ++++++++++++++++++++++++++++++++++++++++++--
> xen/include/asm-arm/nmi.h | 13 +++
> xen/include/xen/xsplice.h | 7 +-
> 8 files changed, 306 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/arm/xsplice.c b/xen/arch/arm/xsplice.c
> index 8d85fa9..3c34eb3 100644
> --- a/xen/arch/arm/xsplice.c
> +++ b/xen/arch/arm/xsplice.c
> @@ -3,6 +3,14 @@
> #include <xen/xsplice_elf.h>
> #include <xen/xsplice.h>
>
> +void xsplice_apply_jmp(struct xsplice_patch_func *func)
> +{
> +}
> +
> +void xsplice_revert_jmp(struct xsplice_patch_func *func)
> +{
> +}
> +
> int xsplice_verify_elf(uint8_t *data, ssize_t len)
> {
> return -ENOSYS;
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index fe3be30..4420cfc 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -36,6 +36,7 @@
> #include <xen/cpu.h>
> #include <xen/wait.h>
> #include <xen/guest_access.h>
> +#include <xen/xsplice.h>
> #include <public/sysctl.h>
> #include <asm/regs.h>
> #include <asm/mc146818rtc.h>
> @@ -120,6 +121,7 @@ static void idle_loop(void)
> (*pm_idle)();
> do_tasklet();
> do_softirq();
> + do_xsplice();
> }
> }
>
> @@ -136,6 +138,7 @@ void startup_cpu_idle_loop(void)
>
> static void noreturn continue_idle_domain(struct vcpu *v)
> {
> + do_xsplice();
> reset_stack_and_jump(idle_loop);
> }
>
> @@ -143,6 +146,7 @@ static void noreturn continue_nonidle_domain(struct vcpu
> *v)
> {
> check_wakeup_from_wait();
> mark_regs_dirty(guest_cpu_user_regs());
> + do_xsplice();
> reset_stack_and_jump(ret_from_intr);
> }
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 8de41fa..65bf7e9 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -26,6 +26,7 @@
> #include <xen/hypercall.h>
> #include <xen/domain_page.h>
> #include <xen/xenoprof.h>
> +#include <xen/xsplice.h>
> #include <asm/current.h>
> #include <asm/io.h>
> #include <asm/paging.h>
> @@ -1071,6 +1072,7 @@ static void noreturn svm_do_resume(struct vcpu *v)
>
> hvm_do_resume(v);
>
> + do_xsplice();
> reset_stack_and_jump(svm_asm_do_resume);
> }
>
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index 4ea1ad1..d996f47 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -25,6 +25,7 @@
> #include <xen/kernel.h>
> #include <xen/keyhandler.h>
> #include <xen/vm_event.h>
> +#include <xen/xsplice.h>
> #include <asm/current.h>
> #include <asm/cpufeature.h>
> #include <asm/processor.h>
> @@ -1685,6 +1686,7 @@ void vmx_do_resume(struct vcpu *v)
> }
>
> hvm_do_resume(v);
> + do_xsplice();
> reset_stack_and_jump(vmx_asm_do_vmentry);
> }
>
> diff --git a/xen/arch/x86/xsplice.c b/xen/arch/x86/xsplice.c
> index dbff0d5..31e4124 100644
> --- a/xen/arch/x86/xsplice.c
> +++ b/xen/arch/x86/xsplice.c
> @@ -3,6 +3,25 @@
> #include <xen/xsplice_elf.h>
> #include <xen/xsplice.h>
>
> +#define PATCH_INSN_SIZE 5
> +
> +void xsplice_apply_jmp(struct xsplice_patch_func *func)
Don't we want for it to be 'int'
> +{
> + uint32_t val;
> + uint8_t *old_ptr;
> +
> + old_ptr = (uint8_t *)func->old_addr;
> + memcpy(func->undo, old_ptr, PATCH_INSN_SIZE);
And perhaps use something which can catch an exception (#GP) so that
this can error out?
> + *old_ptr++ = 0xe9; /* Relative jump */
> + val = func->new_addr - func->old_addr - PATCH_INSN_SIZE;
> + memcpy(old_ptr, &val, sizeof val);
> +}
> +
> +void xsplice_revert_jmp(struct xsplice_patch_func *func)
> +{
> + memcpy((void *)func->old_addr, func->undo, PATCH_INSN_SIZE);
> +}
> +
> int xsplice_verify_elf(uint8_t *data, ssize_t len)
> {
>
> diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c
> index 5e88c55..4476be5 100644
> --- a/xen/common/xsplice.c
> +++ b/xen/common/xsplice.c
> @@ -11,16 +11,21 @@
> #include <xen/guest_access.h>
> #include <xen/stdbool.h>
> #include <xen/sched.h>
> +#include <xen/softirq.h>
> #include <xen/lib.h>
> +#include <xen/wait.h>
> #include <xen/xsplice_elf.h>
> #include <xen/xsplice.h>
> #include <public/sysctl.h>
>
> #include <asm/event.h>
> +#include <asm/nmi.h>
>
> static DEFINE_SPINLOCK(payload_list_lock);
> static LIST_HEAD(payload_list);
>
> +static LIST_HEAD(applied_list);
> +
> static unsigned int payload_cnt;
> static unsigned int payload_version = 1;
>
> @@ -29,15 +34,34 @@ struct payload {
> int32_t rc; /* 0 or -EXX. */
>
> struct list_head list; /* Linked to 'payload_list'. */
> + struct list_head applied_list; /* Linked to 'applied_list'. */
>
> + struct xsplice_patch_func *funcs;
> + int nfuncs;
unsigned int;
> void *module_address;
> size_t module_pages;
>
> char id[XEN_XSPLICE_NAME_SIZE + 1]; /* Name of it. */
> };
>
> +/* Defines an outstanding patching action. */
> +struct xsplice_work
> +{
> + atomic_t semaphore; /* Used for rendezvous */
> + atomic_t irq_semaphore; /* Used to signal all IRQs disabled */
> + struct payload *data; /* The payload on which to act */
> + volatile bool_t do_work; /* Signals work to do */
> + volatile bool_t ready; /* Signals all CPUs synchronized */
> + uint32_t cmd; /* Action request. XSPLICE_ACTION_* */
Now since you have a pointer to 'data' can't you follow that for the
cmd? Or at least the 'data->state'?
Missing full stops.
> +};
> +
> +static DEFINE_SPINLOCK(xsplice_work_lock);
> +/* There can be only one outstanding patching action. */
> +static struct xsplice_work xsplice_work;
> +
> static int load_module(struct payload *payload, uint8_t *raw, ssize_t len);
> static void free_module(struct payload *payload);
> +static int schedule_work(struct payload *data, uint32_t cmd);
>
> static const char *state2str(int32_t state)
> {
> @@ -341,28 +365,22 @@ static int xsplice_action(xen_sysctl_xsplice_action_t
> *action)
> case XSPLICE_ACTION_REVERT:
> if ( data->state == XSPLICE_STATE_APPLIED )
> {
> - /* No implementation yet. */
> - data->state = XSPLICE_STATE_CHECKED;
> - data->rc = 0;
> - rc = 0;
> + data->rc = -EAGAIN;
> + rc = schedule_work(data, action->cmd);
> }
> break;
> case XSPLICE_ACTION_APPLY:
> if ( (data->state == XSPLICE_STATE_CHECKED) )
> {
> - /* No implementation yet. */
> - data->state = XSPLICE_STATE_APPLIED;
> - data->rc = 0;
> - rc = 0;
> + data->rc = -EAGAIN;
> + rc = schedule_work(data, action->cmd);
> }
> break;
> case XSPLICE_ACTION_REPLACE:
> if ( data->state == XSPLICE_STATE_CHECKED )
> {
> - /* No implementation yet. */
> - data->state = XSPLICE_STATE_CHECKED;
> - data->rc = 0;
> - rc = 0;
> + data->rc = -EAGAIN;
> + rc = schedule_work(data, action->cmd);
> }
> break;
> default:
> @@ -637,6 +655,24 @@ static int perform_relocs(struct xsplice_elf *elf)
> return 0;
> }
>
> +static int find_special_sections(struct payload *payload,
> + struct xsplice_elf *elf)
> +{
> + struct xsplice_elf_sec *sec;
> +
> + sec = xsplice_elf_sec_by_name(elf, ".xsplice.funcs");
> + if ( !sec )
> + {
> + printk(XENLOG_ERR ".xsplice.funcs is missing\n");
> + return -1;
> + }
> +
> + payload->funcs = (struct xsplice_patch_func *)sec->load_addr;
> + payload->nfuncs = sec->sec->sh_size / (sizeof *payload->funcs);
> +
> + return 0;
> +}
That looks like it should belong to another patch?
> +
> static int load_module(struct payload *payload, uint8_t *raw, ssize_t len)
> {
> struct xsplice_elf elf;
> @@ -662,6 +698,10 @@ static int load_module(struct payload *payload, uint8_t
> *raw, ssize_t len)
> if ( rc )
> goto err_module;
>
> + rc = find_special_sections(payload, &elf);
> + if ( rc )
> + goto err_module;
> +
Ditto?
> return 0;
>
> err_module:
> @@ -672,6 +712,206 @@ static int load_module(struct payload *payload, uint8_t
> *raw, ssize_t len)
> return rc;
> }
>
> +
> +/*
> + * The following functions get the CPUs into an appropriate state and
> + * apply (or revert) each of the module's functions.
s/module/payload/
> + */
> +
> +/*
> + * This function is executed having all other CPUs with no stack and IRQs
> + * disabled.
Well, there is some stack. For example from 'cpu_idle' - you have the
'cpu_idle' on the stack.
> + */
> +static int apply_payload(struct payload *data)
> +{
> + int i;
unsigned int
> +
> + printk(XENLOG_DEBUG "Applying payload: %s\n", data->id);
> +
> + for ( i = 0; i < data->nfuncs; i++ )
> + xsplice_apply_jmp(data->funcs + i);
And if this returns an error then we could skip adding
it to the applied_list..
> +
Also the patching in Linux seems to do some icache purging.
Should we use that?
> + list_add_tail(&data->applied_list, &applied_list);
> +
> + return 0;
> +}
> +
> +/*
> + * This function is executed having all other CPUs with no stack and IRQs
> + * disabled.
> + */
> +static int revert_payload(struct payload *data)
> +{
> + int i;
unsigned int i;
> +
> + printk(XENLOG_DEBUG "Reverting payload: %s\n", data->id);
> +
> + for ( i = 0; i < data->nfuncs; i++ )
> + xsplice_revert_jmp(data->funcs + i);
> +
> + list_del(&data->applied_list);
> +
> + return 0;
> +}
> +
> +/* Must be holding the payload_list lock */
Missing full stop.
Should that lock be called something else now? (Because it is certainly
not protecting the list anymore - but also the scheduling action).
> +static int schedule_work(struct payload *data, uint32_t cmd)
> +{
> + /* Fail if an operation is already scheduled */
> + if ( xsplice_work.do_work )
> + return -EAGAIN;
> +
> + xsplice_work.cmd = cmd;
> + xsplice_work.data = data;
> + atomic_set(&xsplice_work.semaphore, 0);
> + atomic_set(&xsplice_work.irq_semaphore, 0);
> + xsplice_work.ready = false;
> + smp_mb();
> + xsplice_work.do_work = true;
> + smp_mb();
So this is your 'GO GO' signal right? I think you may want
to have 'smb_wmb()'
> +
> + return 0;
> +}
> +
/me laughs. What a way to 'fix' the NMI watchdog.
> +static int mask_nmi_callback(const struct cpu_user_regs *regs, int cpu)
> +{
> + return 1;
> +}
> +
> +static void reschedule_fn(void *unused)
> +{
> + smp_mb(); /* Synchronize with setting do_work */
> + raise_softirq(SCHEDULE_SOFTIRQ);
> +}
> +
> +/*
> + * The main function which manages the work of quiescing the system and
> + * patching code.
> + */
> +void do_xsplice(void)
> +{
> + int id;
unsigned int id;
> + unsigned int total_cpus;
> + nmi_callback_t saved_nmi_callback;
> +
> + /* Fast path: no work to do */
Missing full stop.
> + if ( likely(!xsplice_work.do_work) )
> + return;
> +
> + ASSERT(local_irq_is_enabled());
> +
> + spin_lock(&xsplice_work_lock);
> + id = atomic_read(&xsplice_work.semaphore);
> + atomic_inc(&xsplice_work.semaphore);
> + spin_unlock(&xsplice_work_lock);
Could you use 'atomic_inc_and_test' and then you can get
rid of the spinlock.
> +
> + total_cpus = num_online_cpus();
Which could change across these invocations.. Perhaps
during these calls we need to lock up CPU up/down code?
> +
> + if ( id == 0 )
> + {
Can you just make this its own function? Perhaps call it
'xsplice_do_single' or such?
> + s_time_t timeout, start;
> +
> + /* Trigger other CPUs to execute do_xsplice */
Missing full stop.
> + smp_call_function(reschedule_fn, NULL, 0);
> +
> + /* Wait for other CPUs with a timeout */
Missing full stop.
> + start = NOW();
> + timeout = start + MILLISECS(30);
Nah. That should be gotten from the XSPLICE_ACTION_APPLY 'time'
parameter - which has an 'timeout' in it.
> + while ( atomic_read(&xsplice_work.semaphore) != total_cpus &&
> + NOW() < timeout )
> + cpu_relax();
> +
> + if ( atomic_read(&xsplice_work.semaphore) == total_cpus )
> + {
> + struct payload *data2;
s/data2/data/ ?
> +
> + /* "Mask" NMIs */
> + saved_nmi_callback = set_nmi_callback(mask_nmi_callback);
> +
> + /* All CPUs are waiting, now signal to disable IRQs */
> + xsplice_work.ready = true;
> + smp_mb();
> +
> + /* Wait for irqs to be disabled */
> + while ( atomic_read(&xsplice_work.irq_semaphore) != (total_cpus
> - 1) )
> + cpu_relax();
> +
> + local_irq_disable();
> + /* Now this function should be the only one on any stack.
> + * No need to lock the payload list or applied list. */
> + switch ( xsplice_work.cmd )
> + {
> + case XSPLICE_ACTION_APPLY:
> + xsplice_work.data->rc =
> apply_payload(xsplice_work.data);
> + if ( xsplice_work.data->rc == 0 )
> + xsplice_work.data->state = XSPLICE_STATE_APPLIED;
> + break;
> + case XSPLICE_ACTION_REVERT:
> + xsplice_work.data->rc =
> revert_payload(xsplice_work.data);
> + if ( xsplice_work.data->rc == 0 )
> + xsplice_work.data->state = XSPLICE_STATE_CHECKED;
> + break;
> + case XSPLICE_ACTION_REPLACE:
> + list_for_each_entry ( data2, &payload_list, list )
> + {
> + if ( data2->state != XSPLICE_STATE_APPLIED )
> + continue;
> +
> + data2->rc = revert_payload(data2);
> + if ( data2->rc == 0 )
> + data2->state = XSPLICE_STATE_CHECKED;
> + else
> + {
> + xsplice_work.data->rc = -EINVAL;
Why not copy the error code (from data2->rc?)
> + break;
> + }
> + }
> + if ( xsplice_work.data->rc != -EINVAL )
And here you can just check for zero.
> + {
> + xsplice_work.data->rc =
> apply_payload(xsplice_work.data);
> + if ( xsplice_work.data->rc == 0 )
> + xsplice_work.data->state =
> XSPLICE_STATE_APPLIED;
> + }
> + break;
> + default:
> + xsplice_work.data->rc = -EINVAL;
> + break;
> + }
> +
> + local_irq_enable();
> + set_nmi_callback(saved_nmi_callback);
> + }
> + else
> + {
> + xsplice_work.data->rc = -EBUSY;
> + }
> +
> + xsplice_work.do_work = 0;
> + smp_mb(); /* Synchronize with waiting CPUs */
Missing full stop.
> + }
> + else
> + {
> + /* Wait for all CPUs to rendezvous */
Missing full stop
> + while ( xsplice_work.do_work && !xsplice_work.ready )
> + {
> + cpu_relax();
> + smp_mb();
> + }
> +
> + /* Disable IRQs and signal */
Missing full stop.
> + local_irq_disable();
> + atomic_inc(&xsplice_work.irq_semaphore);
> +
> + /* Wait for patching to complete */
Missing full stop.
> + while ( xsplice_work.do_work )
> + {
> + cpu_relax();
> + smp_mb();
> + }
> + local_irq_enable();
> + }
> +}
> +
> static int __init xsplice_init(void)
> {
> register_keyhandler('x', xsplice_printall, "print xsplicing info", 1);
> diff --git a/xen/include/asm-arm/nmi.h b/xen/include/asm-arm/nmi.h
> index a60587e..82aff35 100644
> --- a/xen/include/asm-arm/nmi.h
> +++ b/xen/include/asm-arm/nmi.h
> @@ -4,6 +4,19 @@
> #define register_guest_nmi_callback(a) (-ENOSYS)
> #define unregister_guest_nmi_callback() (-ENOSYS)
>
> +typedef int (*nmi_callback_t)(const struct cpu_user_regs *regs, int cpu);
> +
> +/**
> + * set_nmi_callback
> + *
> + * Set a handler for an NMI. Only one handler may be
> + * set. Return the old nmi callback handler.
> + */
> +static inline nmi_callback_t set_nmi_callback(nmi_callback_t callback)
> +{
> + return NULL;
> +}
> +
> #endif /* ASM_NMI_H */
> /*
> * Local variables:
> diff --git a/xen/include/xen/xsplice.h b/xen/include/xen/xsplice.h
> index a3946a3..507829c 100644
> --- a/xen/include/xen/xsplice.h
> +++ b/xen/include/xen/xsplice.h
> @@ -11,7 +11,8 @@ struct xsplice_patch_func {
> unsigned long old_addr;
> unsigned long old_size;
> char *name;
> - unsigned char undo[8];
> + uint8_t undo[8];
> + uint8_t pad[56];
This should be in a different patch. As part of the
"xsplice: Implement payload loading"
> };
>
> struct xen_sysctl_xsplice_op;
> @@ -19,6 +20,8 @@ int xsplice_control(struct xen_sysctl_xsplice_op *);
>
> extern void xsplice_printall(unsigned char key);
>
> +void do_xsplice(void);
> +
> /* Arch hooks */
> int xsplice_verify_elf(uint8_t *data, ssize_t len);
> int xsplice_perform_rel(struct xsplice_elf *elf,
> @@ -27,5 +30,7 @@ int xsplice_perform_rel(struct xsplice_elf *elf,
> int xsplice_perform_rela(struct xsplice_elf *elf,
> struct xsplice_elf_sec *base,
> struct xsplice_elf_sec *rela);
> +void xsplice_apply_jmp(struct xsplice_patch_func *func);
> +void xsplice_revert_jmp(struct xsplice_patch_func *func);
>
> #endif /* __XEN_XSPLICE_H__ */
> --
> 2.4.3
>
>
> _______________________________________________
> 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 |