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

[Christopher Clark <christopher.w.clark@gmail.com>]

On Thu, Jan 31, 2019 at 6:49 AM Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> 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@baesystems.com>
> > Acked-by: Lars Kurth <lars.kurth@citrix.com>
> > Reviewed-by: Ross Philipson <ross.philipson@oracle.com>
> > Tested-by: Chris Patterson <pattersonc@ainfosec.com>
>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

ack

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

I've applied all the requested changes below for the next series submission.

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

ack

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

ack

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

ack

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

ack

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

ack

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

ack

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

Noted.

On Thu, Jan 31, 2019 at 8:37 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 31.01.19 at 17:05, <roger.pau@citrix.com> wrote:
> > On Thu, Jan 31, 2019 at 08:13:27AM -0700, Jan Beulich wrote:
> >> >>> On 31.01.19 at 15:49, <roger.pau@citrix.com> wrote:
> >> > On Wed, Jan 30, 2019 at 08:28:09PM -0800, Christopher Clark wrote:
> >> >> +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.
> >>
> >> Well, that's a substitution I wouldn't like to do while committing,
> >> as it means doing a little more than a simple text replacement.
> >
> > From a functionality PoV the code does exactly the same, I just think
> > list_first_entry_or_null is more suitable and I haven't been convinced
> > by the argument posted on v5.
>
> Well, we're in agreement. All I did say is that a change like this
> I'd like to see done by the submitter, not the committer (at least
> not if I end up being the latter).

Ack, changes are applied for next series submission.

Christopher

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