Re: [PATCH v2 09/13] xsplice: Implement support for applying/reverting/replacing patches. (v2)

From: Ross Lagerwall <ross.lagerwall@citrix.com>
To: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	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
Subject: Re: [PATCH v2 09/13] xsplice: Implement support for applying/reverting/replacing patches. (v2)
Date: Tue, 19 Jan 2016 14:39:40 +0000	[thread overview]
Message-ID: <569E4AAC.9070100@citrix.com> (raw)
In-Reply-To: <1452808031-706-10-git-send-email-konrad.wilk@oracle.com>

On 01/14/2016 09:47 PM, Konrad Rzeszutek Wilk wrote:
> From: Ross Lagerwall <ross.lagerwall@citrix.com>
>
> Implement support for the apply, revert and replace actions.
>
snip
> +#include <xen/cpu.h>
>   #include <xen/guest_access.h>
>   #include <xen/keyhandler.h>
>   #include <xen/lib.h>
> @@ -10,25 +11,38 @@
>   #include <xen/mm.h>
>   #include <xen/sched.h>
>   #include <xen/smp.h>
> +#include <xen/softirq.h>
>   #include <xen/spinlock.h>
> +#include <xen/wait.h>
>   #include <xen/xsplice_elf.h>
>   #include <xen/xsplice.h>
>
>   #include <asm/event.h>
> +#include <asm/nmi.h>
>   #include <public/sysctl.h>
>
> -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