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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

> 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.

>             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.

> +
> +    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.

> +        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.

> +{
> +    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.

> +    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.  (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 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.

>          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.

~Andrew

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