Re: [Xen-devel] [PATCH v3 04/15] argo: init, destroy and soft-reset, with enable command line opt

[Christopher Clark <christopher.w.clark@xxxxxxxxx>]

On Mon, Jan 14, 2019 at 6:58 AM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> On 07/01/2019 07:42, Christopher Clark wrote:
> > Initialises basic data structures and performs teardown of argo state
> > for domain shutdown.
> >
> > Inclusion of the Argo implementation is dependent on CONFIG_ARGO.
> >
> > Introduces a new Xen command line parameter 'argo': bool to enable/disable
> > the argo hypercall. Defaults to disabled.
> >
> > New headers:
> >   public/argo.h: with definions of addresses and ring structure, including
> >   indexes for atomic update for communication between domain and hypervisor.
> >
> >   xen/argo.h: to expose the hooks for integration into domain lifecycle:
> >     argo_init: per-domain init of argo data structures for domain_create.
> >     argo_destroy: teardown for domain_destroy and the error exit
> >                   path of domain_create.
> >     argo_soft_reset: reset of domain state for domain_soft_reset.
> >
> > Adds two new fields to struct domain:
> >     rwlock_t argo_lock;
> >     struct argo_domain *argo;
> >
> > In accordance with recent work on _domain_destroy, argo_destroy is
> > idempotent. It will tear down: all rings registered by this domain, all
> > rings where this domain is the single sender (ie. specified partner,
> > non-wildcard rings), and all pending notifications where this domain is
> > awaiting signal about available space in the rings of other domains.
> >
> > A count will be maintained of the number of rings that a domain has
> > registered in order to limit it below the fixed maximum limit defined here.
> >
> > The software license on the public header is the BSD license, standard
> > procedure for the public Xen headers. The public header was originally
> > posted under a GPL license at: [1]:
> > https://lists.xenproject.org/archives/html/xen-devel/2013-05/msg02710.html
> >
> > The following ACK by Lars Kurth is to confirm that only people being
> > employees of Citrix contributed to the header files in the series posted at
> > [1] and that thus the copyright of the files in question is fully owned by
> > Citrix. The ACK also confirms that Citrix is happy for the header files to
> > be published under a BSD license in this series (which is based on [1]).
> >
> > Signed-off-by: Christopher Clark <christopher.clark6@xxxxxxxxxxxxxx>
> > Acked-by: Lars Kurth <lars.kurth@xxxxxxxxxx>
>
> I hope I've not trodden on the toes of any other reviews.  I've got some
> minor requests, but hopefully its all fairly trivial.
>
> > diff --git a/docs/misc/xen-command-line.pandoc 
> > b/docs/misc/xen-command-line.pandoc
> > index a755a67..aea13eb 100644
> > --- a/docs/misc/xen-command-line.pandoc
> > +++ b/docs/misc/xen-command-line.pandoc
> > @@ -182,6 +182,17 @@ Permit Xen to use "Always Running APIC Timer" support 
> > on compatible hardware
> >  in combination with cpuidle.  This option is only expected to be useful for
> >  developers wishing Xen to fall back to older timing methods on newer 
> > hardware.
> >
> > +### argo
> > +> `= <boolean>`
> > +
> > +> Default: `false`
> > +
> > +Enable the Argo hypervisor-mediated interdomain communication mechanism.
> > +
> > +This allows domains access to the Argo hypercall, which supports 
> > registration
> > +of memory rings with the hypervisor to receive messages, sending messages 
> > to
> > +other domains by hypercall and querying the ring status of other domains.
>
> Please do include a note about CONFIG_ARGO.  I know this doc is
> inconsistent on the matter (as Kconfig postdates the written entries
> here), but I have been trying to fix up, and now about half of the
> documentation does mention appropriate Kconfig information.

Ack, note added.

>
> > diff --git a/xen/common/argo.c b/xen/common/argo.c
> > index 6f782f7..86195d3 100644
> > --- a/xen/common/argo.c
> > +++ b/xen/common/argo.c
> >  long
> >  do_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1,
>
> I know I'm commenting on the wrong patch, but please use unsigned long
> cmd, so the type definition here doesn't truncate the caller provided
> value.  We have similar buggy code all over Xen, but its too late to fix
> that, and I'd prefer not to propagate the error.

On this one, given Jan's reply, I've left it as is for the series that
I'll push tonight. That patch is carrying an Ack from Jan at the
moment, so I wasn't going to touch it if it's not required. If there's
consensus that it should change, let me know and I'll switch it.

>
> >             XEN_GUEST_HANDLE_PARAM(void) arg2, unsigned long arg3,
> >             unsigned long arg4)
> >  {
> > -    return -ENOSYS;
> > +    struct domain *currd = current->domain;
> > +    long rc = -EFAULT;
> > +
> > +    argo_dprintk("->do_argo_op(%u,%p,%p,%d,%d)\n", cmd,
> > +                 (void *)arg1.p, (void *)arg2.p, (int) arg3, (int) arg4);
>
> For debugging purposes, you don't want to truncate any of these values,
> or you'll have a print message which doesn't match what the guest
> provided.  I'd use %ld for arg3 and arg4.

I've gone with %lu for arg3 and %lx for arg4, for the next round, but
I could be missing something: is there a reason to prefer '%ld' over
'%lu' for using with an unsigned type? Should I be using %d elsewhere,
eg. for a domid?

>
> > +
> > +    if ( unlikely(!opt_argo_enabled) )
> > +    {
> > +        rc = -EOPNOTSUPP;
>
> Shouldn't this be ENOSYS instead?  There isn't a conceptual difference
> between CONFIG_ARGO compiled out, and opt_argo clear on the command
> line, and I don't think a guest should be able to tell the difference.

EOPNOTSUPP has been retained here, as per an earlier review and also a
response later in this thread.

>
> > +        return rc;
> > +    }
> > +
> > +    domain_lock(currd);
> > +
> > +    switch (cmd)
> > +    {
> > +    default:
> > +        rc = -EOPNOTSUPP;
> > +        break;
> > +    }
> > +
> > +    domain_unlock(currd);
> > +
> > +    argo_dprintk("<-do_argo_op(%u)=%ld\n", cmd, rc);
> > +
> > +    return rc;
> > +}
> > +
> > +static void
> > +argo_domain_init(struct argo_domain *argo)
> > +{
> > +    unsigned int i;
> > +
> > +    rwlock_init(&argo->lock);
> > +    spin_lock_init(&argo->send_lock);
> > +    spin_lock_init(&argo->wildcard_lock);
> > +    argo->ring_count = 0;
> > +
> > +    for ( i = 0; i < ARGO_HTABLE_SIZE; ++i )
> > +    {
> > +        INIT_HLIST_HEAD(&argo->ring_hash[i]);
> > +        INIT_HLIST_HEAD(&argo->send_hash[i]);
> > +    }
> > +    INIT_HLIST_HEAD(&argo->wildcard_pend_list);
> > +}
> > +
> > +int
> > +argo_init(struct domain *d)
>
> Are there any per-vcpu argo resources?  I don't see any in the series,
> but I'd be tempted to name the external functions as
> argo_domain_{init,destroy}() which is slightly clearer in the caller
> context.

I haven't renamed this as I'm out of time today but I slightly wary
about whether it ends up helping clarity, given 1) the name of an
existing data type 'argo_domain' and 2) the other similar init
functions that are invoked in domain_create don't seem to follow the
<subsystem>_domain_init naming scheme. Are you sure I should do this?

>
> > +{
> > +    struct argo_domain *argo;
> > +
> > +    if ( !opt_argo_enabled )
> > +    {
> > +        argo_dprintk("argo disabled, domid: %d\n", d->domain_id);
> > +        return 0;
> > +    }
> > +
> > +    argo_dprintk("init: domid: %d\n", d->domain_id);
> > +
> > +    argo = xmalloc(struct argo_domain);
>
> For sanity sake, I'd suggest xzalloc() here, not that I can spot
> anything wrong with the current code.

ack, done.

>
> > +    if ( !argo )
> > +        return -ENOMEM;
> > +
> > +    write_lock(&argo_lock);
> > +
> > +    argo_domain_init(argo);
>
> This call doesn't need to be within the global argo_lock critical
> region, because it exclusively operates on state which is inaccessible
> to the rest of the system until d->argo is written.  This then shrinks
> the critical region to a single pointer write.

ack, done

> (Further, with a patch I
> haven't posted yet, the memset(0) in zxalloc() can be write-merged with
> the setup code to avoid repeated writes, which can't happen with a
> spinlock in between.)
>
> > +
> > +    d->argo = argo;
> > +
> > +    write_unlock(&argo_lock);
> > +
> > +    return 0;
> > +}
> > +
> > +void
> > +argo_destroy(struct domain *d)
>
> Is this function fully idempotent?  Given its current calling context,
> it needs to be.  (I think it is, but I just want to double check,
> because it definitely wants to be.)

I think so and it is intended to be - it takes a lock, only does work
if a pointer isn't NULL, and then NULLs it before dropping the lock,
so should be ok.

>
> I ask, because...
>
> > +{
> > +    BUG_ON(!d->is_dying);
> > +
> > +    write_lock(&argo_lock);
> > +
> > +    argo_dprintk("destroy: domid %d d->argo=%p\n", d->domain_id, d->argo);
> > +
> > +    if ( d->argo )
> > +    {
> > +        domain_rings_remove_all(d);
> > +        partner_rings_remove(d);
> > +        wildcard_rings_pending_remove(d);
> > +        xfree(d->argo);
> > +        d->argo = NULL;
> > +    }
> > +    write_unlock(&argo_lock);
> > +}
> > +
> > +void
> > +argo_soft_reset(struct domain *d)
> > +{
> > +    write_lock(&argo_lock);
> > +
> > +    argo_dprintk("soft reset d=%d d->argo=%p\n", d->domain_id, d->argo);
> > +
> > +    if ( d->argo )
> > +    {
> > +        domain_rings_remove_all(d);
> > +        partner_rings_remove(d);
> > +        wildcard_rings_pending_remove(d);
> > +
> > +        if ( !opt_argo_enabled )
> > +        {
> > +            xfree(d->argo);
> > +            d->argo = NULL;
> > +        }
> > +        else
> > +            argo_domain_init(d->argo);
> > +    }
> > +
> > +    write_unlock(&argo_lock);
> >  }
> > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > index c623dae..9596840 100644
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -32,6 +32,7 @@
> >  #include <xen/grant_table.h>
> >  #include <xen/xenoprof.h>
> >  #include <xen/irq.h>
> > +#include <xen/argo.h>
> >  #include <asm/debugger.h>
> >  #include <asm/p2m.h>
> >  #include <asm/processor.h>
> > @@ -277,6 +278,10 @@ static void _domain_destroy(struct domain *d)
> >
> >      xfree(d->pbuf);
> >
> > +#ifdef CONFIG_ARGO
> > +    argo_destroy(d);
> > +#endif
>
> ... given this call (which is correct), ...
>
> > +
> >      rangeset_domain_destroy(d);
> >
> >      free_cpumask_var(d->dirty_cpumask);
> > @@ -376,6 +381,9 @@ struct domain *domain_create(domid_t domid,
> >      spin_lock_init(&d->hypercall_deadlock_mutex);
> >      INIT_PAGE_LIST_HEAD(&d->page_list);
> >      INIT_PAGE_LIST_HEAD(&d->xenpage_list);
> > +#ifdef CONFIG_ARGO
> > +    rwlock_init(&d->argo_lock);
> > +#endif
> >
> >      spin_lock_init(&d->node_affinity_lock);
> >      d->node_affinity = NODE_MASK_ALL;
> > @@ -445,6 +453,11 @@ struct domain *domain_create(domid_t domid,
> >              goto fail;
> >          init_status |= INIT_gnttab;
> >
> > +#ifdef CONFIG_ARGO
> > +        if ( (err = argo_init(d)) != 0 )
> > +            goto fail;
> > +#endif
> > +
> >          err = -ENOMEM;
> >
> >          d->pbuf = xzalloc_array(char, DOMAIN_PBUF_SIZE);
> > @@ -717,6 +730,9 @@ int domain_kill(struct domain *d)
> >          if ( d->is_dying != DOMDYING_alive )
> >              return domain_kill(d);
> >          d->is_dying = DOMDYING_dying;
> > +#ifdef CONFIG_ARGO
> > +        argo_destroy(d);
> > +#endif
>
> ... this one isn't necessary.
>
> I'm in the middle of fixing all this destruction logic, and
> _domain_destroy() is called below.
>
> The rule is that everything in _domain_destroy() should be idempotent,
> and all destruction logic needs moving there, so I can remove
> DOMCTL_setmaxvcpus and fix a load of toolstack-triggerable NULL pointer
> dereferences in Xen.
>
> Eventually, everything in this hunk will disappear.

Thanks for the guidance. I've added a FIXME for this for the series
I'm about to push and will work on resolving it tomorrow.

>
> >          evtchn_destroy(d);
> >          gnttab_release_mappings(d);
> >          tmem_destroy(d->tmem_client);
> > diff --git a/xen/include/xen/argo.h b/xen/include/xen/argo.h
> > new file mode 100644
> > index 0000000..29d32a9
> > --- /dev/null
> > +++ b/xen/include/xen/argo.h
> > @@ -0,0 +1,23 @@
> > +/******************************************************************************
> > + * Argo : Hypervisor-Mediated data eXchange
> > + *
> > + * Copyright (c) 2018, BAE Systems
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  
> > USA
> > + */
> > +
> > +#ifndef __XEN_ARGO_H__
> > +#define __XEN_ARGO_H__
> > +
> > +int argo_init(struct domain *d);
> > +void argo_destroy(struct domain *d);
> > +void argo_soft_reset(struct domain *d);
>
> Instead of the #ifdefary in the calling code, please could you stub
> these out in this file?  See the tail of include/asm-x86/pv/domain.h for
> an example based on CONFIG_PV.

ack, done at Roger's earlier request.

Thanks for the review,

Christopher

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel