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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jan 30, 2019 at 08:28:09PM -0800, 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 a new field to struct domain: 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.
> 
> Macros are defined to verify the internal locking state within the argo
> implementation. The macros are ASSERTed on entry to functions to validate
> and document the required lock state prior to calling.
> 
> The hash function for the hashtables that hold ring state is derived from
> the string hashing function djb2 (http://www.cse.yorku.ca/~oz/hash.html)
> by Daniel J. Bernstein. Basic testing with a limited number of domains and
> ports has shown reasonable distribution for the table size.
> 
> 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>
> Reviewed-by: Ross Philipson <ross.philipson@xxxxxxxxxx>
> Tested-by: Chris Patterson <pattersonc@xxxxxxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

I have some comments below that could be fixed upon commit if the
committer agrees.

> +static struct argo_ring_info *
> +find_ring_info(const struct domain *d, const struct argo_ring_id *id)
> +{
> +    struct argo_ring_info *ring_info;
> +    const struct list_head *bucket;
> +
> +    ASSERT(LOCKING_Read_rings_L2(d));
> +
> +    /* List is not modified here. Search and return the match if found. */
> +    bucket = &d->argo->ring_hash[hash_index(id)];
> +
> +    list_for_each_entry(ring_info, bucket, node)

I'm not sure what's the policy regarding list_ macros, should spaces
be added between the parentheses?

list_for_each_entry ( ring_info, bucket, node )

I don't have a strong opinion either way.

[...]
> +static void
> +pending_remove_all(const struct domain *d, struct argo_ring_info *ring_info)
> +{
> +    struct pending_ent *ent, *next;
> +
> +    ASSERT(LOCKING_L3(d, ring_info));
> +
> +    /* Delete all pending notifications from this ring's list. */
> +    list_for_each_entry_safe(ent, next, &ring_info->pending, node)

list_first_entry_or_null and you can get rid of next.

> +    {
> +        /* For wildcard rings, remove each from their wildcard list too. */
> +        if ( ring_info->id.partner_id == XEN_ARGO_DOMID_ANY )
> +            wildcard_pending_list_remove(ent->domain_id, ent);
> +        list_del(&ent->node);
> +        xfree(ent);
> +    }
> +    ring_info->npending = 0;
> +}
> +
> +static void
> +wildcard_rings_pending_remove(struct domain *d)
> +{
> +    struct pending_ent *ent, *next;
> +
> +    ASSERT(LOCKING_Write_L1);
> +
> +    /* Delete all pending signals to the domain about wildcard rings. */
> +    list_for_each_entry_safe(ent, next, &d->argo->wildcard_pend_list, node)

list_first_entry_or_null and you can get rid of next.

> +    {
> +        /*
> +         * The ent->node deleted here, and the npending value decreased,
> +         * belong to the ring_info of another domain, which is why this
> +         * function requires holding W(L1):
> +         * it implies the L3 lock that protects that ring_info struct.
> +         */
> +        ent->ring_info->npending--;
> +        list_del(&ent->node);
> +        list_del(&ent->wildcard_node);
> +        xfree(ent);
> +    }
> +}
> +
> +static void
> +ring_remove_mfns(const struct domain *d, struct argo_ring_info *ring_info)
> +{
> +    unsigned int i;
> +
> +    ASSERT(LOCKING_Write_rings_L2(d));
> +
> +    if ( !ring_info->mfns )
> +        return;
> +
> +    if ( !ring_info->mfn_mapping )
> +    {
> +        ASSERT_UNREACHABLE();
> +        return;
> +    }
> +
> +    ring_unmap(d, ring_info);
> +
> +    for ( i = 0; i < ring_info->nmfns; i++ )
> +        if ( !mfn_eq(ring_info->mfns[i], INVALID_MFN) )
> +            put_page_and_type(mfn_to_page(ring_info->mfns[i]));
> +
> +    ring_info->nmfns = 0;
> +    XFREE(ring_info->mfns);
> +    XFREE(ring_info->mfn_mapping);
> +}
> +
> +static void
> +ring_remove_info(const struct domain *d, struct argo_ring_info *ring_info)
> +{
> +    ASSERT(LOCKING_Write_rings_L2(d));
> +
> +    pending_remove_all(d, ring_info);
> +    list_del(&ring_info->node);
> +    ring_remove_mfns(d, ring_info);
> +    xfree(ring_info);
> +}
> +
> +static void
> +domain_rings_remove_all(struct domain *d)
> +{
> +    unsigned int i;
> +
> +    ASSERT(LOCKING_Write_rings_L2(d));
> +
> +    for ( i = 0; i < ARGO_HASHTABLE_SIZE; ++i )
> +    {
> +        struct argo_ring_info *ring_info, *next;
> +        struct list_head *bucket = &d->argo->ring_hash[i];
> +
> +        list_for_each_entry_safe(ring_info, next, bucket, node)

list_first_entry_or_null and you can get rid of next.

> +            ring_remove_info(d, ring_info);
> +    }
> +    d->argo->ring_count = 0;
> +}
> +
> +/*
> + * Tear down all rings of other domains where src_d domain is the partner.
> + * (ie. it is the single domain that can send to those rings.)
> + * This will also cancel any pending notifications about those rings.
> + */
> +static void
> +partner_rings_remove(struct domain *src_d)
> +{
> +    unsigned int i;
> +
> +    ASSERT(LOCKING_Write_L1);
> +
> +    for ( i = 0; i < ARGO_HASHTABLE_SIZE; ++i )
> +    {
> +        struct argo_send_info *send_info, *next;
> +        struct list_head *bucket = &src_d->argo->send_hash[i];
> +
> +        /* Remove all ents from the send list. Take each off their ring 
> list. */
> +        list_for_each_entry_safe(send_info, next, bucket, node)

list_first_entry_or_null and you can get rid of next.

> +        {
> +            struct domain *dst_d = get_domain_by_id(send_info->id.domain_id);
> +
> +            if ( dst_d && dst_d->argo )
> +            {
> +                struct argo_ring_info *ring_info =
> +                    find_ring_info(dst_d, &send_info->id);
> +
> +                if ( ring_info )
> +                {
> +                    ring_remove_info(dst_d, ring_info);
> +                    dst_d->argo->ring_count--;
> +                }
> +                else
> +                    ASSERT_UNREACHABLE();
> +            }
> +            else
> +                ASSERT_UNREACHABLE();
> +
> +            if ( dst_d )
> +                put_domain(dst_d);
> +
> +            list_del(&send_info->node);
> +            xfree(send_info);
> +        }
> +    }
> +}
> +
>  long
>  do_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1,
>             XEN_GUEST_HANDLE_PARAM(void) arg2, unsigned long arg3,
>             unsigned long arg4)
>  {
> -    return -ENOSYS;
> +    long rc = -EFAULT;
> +
> +    argo_dprintk("->do_argo_op(%u,%p,%p,%lu,0x%lx)\n", cmd,
> +                 (void *)arg1.p, (void *)arg2.p, arg3, arg4);
> +
> +    if ( unlikely(!opt_argo) )
> +        return -EOPNOTSUPP;
> +
> +    switch (cmd)
               ^ ^ missing spaces
> +    {
> +    default:
> +        rc = -EOPNOTSUPP;
> +        break;
> +    }
> +
> +    argo_dprintk("<-do_argo_op(%u)=%ld\n", cmd, rc);
> +
> +    return rc;
>  }
>  
>  #ifdef CONFIG_COMPAT
> @@ -42,6 +561,113 @@ compat_argo_op(unsigned int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg1,
>                 XEN_GUEST_HANDLE_PARAM(void) arg2, unsigned long arg3,
>                 unsigned long arg4)
>  {
> -    return -ENOSYS;
> +    long rc = -EFAULT;
> +
> +    argo_dprintk("->compat_argo_op(%u,%p,%p,%lu,0x%lx)\n", cmd,
> +                 (void *)arg1.p, (void *)arg2.p, arg3, arg4);
> +
> +    if ( unlikely(!opt_argo) )
> +        return -EOPNOTSUPP;
> +
> +    switch (cmd)
               ^ ^ missing spaces

[...]
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index c623dae..7470cd9 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,8 @@ static void _domain_destroy(struct domain *d)
>  
>      xfree(d->pbuf);
>  
> +    argo_destroy(d);
> +
>      rangeset_domain_destroy(d);
>  
>      free_cpumask_var(d->dirty_cpumask);
> @@ -445,6 +448,9 @@ struct domain *domain_create(domid_t domid,
>              goto fail;
>          init_status |= INIT_gnttab;
>  
> +        if ( (err = argo_init(d)) != 0 )

Here you are adding code that performs a variable assignment inside of
a condition, much like what would be required to use
list_first_entry_or_null.

Roger.

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