[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
- To: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
- From: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
- Date: Thu, 5 Nov 2015 11:45:42 +0000
- Cc: Kevin Tian <kevin.tian@xxxxxxxxx>, Ian Campbell <ian.campbell@xxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, xen-devel@xxxxxxxxxxxxx, Jan Beulich <jbeulich@xxxxxxxx>, Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>, Jun Nakajima <jun.nakajima@xxxxxxxxx>, Aravind Gopalakrishnan <Aravind.Gopalakrishnan@xxxxxxx>, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>, Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
- Delivery-date: Thu, 05 Nov 2015 11:45:52 +0000
- List-id: Xen developer discussion <xen-devel.lists.xen.org>
On 11/05/2015 03:17 AM, Konrad Rzeszutek Wilk wrote:
snip
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'
Only if an error is expected.
+{
+ 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?
Why would this fail?
+ *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'?
I moved cmd out of the payload and into xsplice_work since cmd is only
needed when there is work to do.
data->state contains the current state of the payload (i.e. before the
action has been performed) so it provides no indication of what command
needs to be performed.
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?
Why? The array of functions is specifically needed for applying a
payload so the code belongs in this 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..
Only if an error is expected.
+
Also the patching in Linux seems to do some icache purging.
Should we use that?
I didn't see that when I looked for it. The alternatives patching in Xen
doesn't purge the icache (afaict). We need feedback from an x86
maintainer here.
+ 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).
Hmm...
+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()'
A full review of the memory barriers and synchronization is needed by
someone more knowledgeable than me.
+
+ return 0;
+}
+
/me laughs. What a way to 'fix' the NMI watchdog.
It comes directly from the alternatives code.
+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.
OK.
+
+ total_cpus = num_online_cpus();
Which could change across these invocations.. Perhaps
during these calls we need to lock up CPU up/down code?
OK.
+
+ 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?)
No. Reverting a different payload updates the error code for that
payload. The payload to-be-applied has failed because a dependent action
has failed. This is not the same as the original error. The original
error is visible through data2->rc.
+ break;
+ }
+ }
+ if ( xsplice_work.data->rc != -EINVAL )
And here you can just check for zero.
No, because xsplice_work.data->rc is originally -EAGAIN (in progress). I
suppose I could check for xsplice_work.data->rc == -EAGAIN.
+ {
+ 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"
Oops, that's what I intended.
--
Ross Lagerwall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
