From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ross Lagerwall Subject: Re: [PATCH v2 09/13] xsplice: Implement support for applying/reverting/replacing patches. (v2) Date: Tue, 19 Jan 2016 14:39:40 +0000 Message-ID: <569E4AAC.9070100@citrix.com> References: <1452808031-706-1-git-send-email-konrad.wilk@oracle.com> <1452808031-706-10-git-send-email-konrad.wilk@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1aLXRl-0002EO-My for xen-devel@lists.xenproject.org; Tue, 19 Jan 2016 14:39:49 +0000 In-Reply-To: <1452808031-706-10-git-send-email-konrad.wilk@oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Konrad Rzeszutek Wilk , xen-devel@lists.xenproject.org, mpohlack@amazon.com, andrew.cooper3@citrix.com, stefano.stabellini@citrix.com, jbeulich@suse.com, ian.jackson@eu.citrix.com, ian.campbell@citrix.com, wei.liu2@citrix.com, sasha.levin@oracle.com List-Id: xen-devel@lists.xenproject.org On 01/14/2016 09:47 PM, Konrad Rzeszutek Wilk wrote: > From: Ross Lagerwall > > Implement support for the apply, revert and replace actions. > snip > +#include > #include > #include > #include > @@ -10,25 +11,38 @@ > #include > #include > #include > +#include > #include > +#include > #include > #include > > #include > +#include > #include > > -static DEFINE_SPINLOCK(payload_list_lock); > +/* > + * Protects against payload_list operations and also allows only one > + * caller in schedule_work. > + */ > +static DEFINE_SPINLOCK(payload_lock); I think it would be cleaner if all the payload_list_lock changes were folded into Patch 3. > static LIST_HEAD(payload_list); > > +static LIST_HEAD(applied_list); > + > static unsigned int payload_cnt; > static unsigned int payload_version = 1; > > struct payload { > int32_t state; /* One of the XSPLICE_STATE_*. */ > int32_t rc; /* 0 or -XEN_EXX. */ > + uint32_t timeout; /* Timeout to do the operation. */ This should go into struct xsplice_work. > struct list_head list; /* Linked to 'payload_list'. */ > void *payload_address; /* Virtual address mapped. */ > size_t payload_pages; /* Nr of the pages. */ > + struct list_head applied_list; /* Linked to 'applied_list'. */ > + struct xsplice_patch_func *funcs; /* The array of functions to patch. */ > + unsigned int nfuncs; /* Nr of functions to patch. */ > > char name[XEN_XSPLICE_NAME_SIZE + 1];/* Name of it. */ > }; > @@ -36,6 +50,23 @@ struct payload { > static int load_payload_data(struct payload *payload, uint8_t *raw, ssize_t len); > static void free_payload_data(struct payload *payload); > > +/* Defines an outstanding patching action. */ > +struct xsplice_work > +{ > + atomic_t semaphore; /* Used for rendezvous. First to grab it will > + do the patching. */ > + 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_* */ > +}; > + > +/* There can be only one outstanding patching action. */ > +static struct xsplice_work xsplice_work; > + > +static int schedule_work(struct payload *data, uint32_t cmd); > + snip > + > +/* > + * This function is executed having all other CPUs with no stack (we may > + * have cpu_idle on it) and IRQs disabled. > + */ > +static int revert_payload(struct payload *data) > +{ > + unsigned int i; > + > + printk(XENLOG_DEBUG "%s: Reverting.\n", data->name); > + > + 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. */ payload lock? > +static int schedule_work(struct payload *data, uint32_t cmd) > +{ > + /* Fail if an operation is already scheduled. */ > + if ( xsplice_work.do_work ) > + return -EAGAIN; Hmm, I don't think EAGAIN is correct. It will cause xen-xsplice to poll for a status update, but the operation hasn't actually been submitted. > + > + xsplice_work.cmd = cmd; > + xsplice_work.data = data; > + atomic_set(&xsplice_work.semaphore, -1); > + atomic_set(&xsplice_work.irq_semaphore, -1); > + > + xsplice_work.ready = 0; > + smp_wmb(); > + xsplice_work.do_work = 1; > + smp_wmb(); > + > + return 0; > +} > + > +/* > + * Note that because of this NOP code the do_nmi is not safely patchable. > + * Also if we do receive 'real' NMIs we have lost them. > + */ > +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); > +} > + > +static int xsplice_do_wait(atomic_t *counter, s_time_t timeout, > + unsigned int total_cpus, const char *s) > +{ > + int rc = 0; > + > + while ( atomic_read(counter) != total_cpus && NOW() < timeout ) > + cpu_relax(); > + > + /* Log & abort. */ > + if ( atomic_read(counter) != total_cpus ) > + { > + printk(XENLOG_DEBUG "%s: %s %u/%u\n", xsplice_work.data->name, > + s, atomic_read(counter), total_cpus); > + rc = -EBUSY; > + xsplice_work.data->rc = rc; > + xsplice_work.do_work = 0; > + smp_wmb(); > + return rc; > + } > + return rc; > +} > + > +static void xsplice_do_single(unsigned int total_cpus) > +{ > + nmi_callback_t saved_nmi_callback; > + s_time_t timeout; > + struct payload *data, *tmp; > + int rc; > + > + data = xsplice_work.data; > + timeout = data->timeout ? data->timeout : MILLISECS(30); The design doc says that a timeout of 0 means infinity. > + printk(XENLOG_DEBUG "%s: timeout is %"PRI_stime"ms\n", data->name, > + timeout / MILLISECS(1)); > + > + timeout += NOW(); > + > + if ( xsplice_do_wait(&xsplice_work.semaphore, timeout, total_cpus, > + "Timed out on CPU semaphore") ) > + return; > + > + /* "Mask" NMIs. */ > + saved_nmi_callback = set_nmi_callback(mask_nmi_callback); > + > + /* All CPUs are waiting, now signal to disable IRQs. */ > + xsplice_work.ready = 1; > + smp_wmb(); > + > + atomic_inc(&xsplice_work.irq_semaphore); > + if ( xsplice_do_wait(&xsplice_work.irq_semaphore, timeout, total_cpus, > + "Timed out on IRQ semaphore.") ) > + return; > + > + local_irq_disable(); As far as I can tell, the mechanics of how this works haven't changed, the code has just been reorganized. Which means the points that Martin raised about this mechanism are still outstanding. -- Ross Lagerwall