Re: [Xen-devel] [PATCH v4 08/14] argo: implement the unregister op

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

On Tue, Jan 15, 2019 at 7:07 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
>
> On Tue, Jan 15, 2019 at 01:27:40AM -0800, Christopher Clark wrote:
> > Takes a single argument: a handle to the ring unregistration struct,
> > which specifies the port and partner domain id or wildcard.
> >
> > The ring's entry is removed from the hashtable of registered rings;
> > any entries for pending notifications are removed; and the ring is
> > unmapped from Xen's address space.
> >
> > If the ring had been registered to communicate with a single specified
> > domain (ie. a non-wildcard ring) then the partner domain state is removed
> > from the partner domain's argo send_info hash table.
> >
> > Signed-off-by: Christopher Clark <christopher.clark6@xxxxxxxxxxxxxx>
>
> Thanks, LGTM. I just have one question below.
>
> > ---
> > v3 #08 Jan: pull xfree out of exclusive critical sections in unregister_ring
> > v3 #08 Jan: rename send_find_info to find_send_info
> > v3 #07 Jan: rename ring_find_info to find_ring_info
> > v3 #08 Roger: use return and remove the out label in unregister_ring
> > v3 #08 Roger: better debug output in send_find_info
> > v3 #10 Roger: move find functions to top of file and drop prototypes
> > v3 #04 Jan: meld compat check for unregister_ring struct
> > v3 #04 Roger/Jan: make lock names clearer and assert their state
> > v3 #04 Jan: port -> aport with type; distinguish argo port from evtchn
> > v3 feedback Roger/Jan: ASSERT currd is current->domain or use 'd' variable 
> > name
> > v3 feedback #07 Roger: const the argo_ring_id structs in send_find_info
> > v2 feedback Jan: drop cookie, implement teardown
> > v2 feedback Jan: drop message from argo_message_op
> > v2 self: OVERHAUL
> > v2 self: reorder logic to shorten critical section
> > v1 #13 feedback Jan: revise use of guest_handle_okay vs __copy ops
> > v1 feedback Roger, Jan: drop argo prefix on static functions
> > v1,2 feedback Jan/Roger/Paul: drop errno returning guest access functions
> > v1 #5 (#14) feedback Paul: use currd in do_argo_message_op
> > v1 #5 (#14) feedback Paul: full use currd in argo_unregister_ring
> > v1 #13 (#14) feedback Paul: replace do/while with goto; reindent
> > v1 self: add blank lines in unregister case in do_argo_message_op
> > v1: #13 feedback Jan: public namespace: prefix with xen
> > v1: #13 feedback Jan: blank line after op case in do_argo_message_op
> > v1: #14 feedback Jan: replace domain id override with validation
> > v1: #18 feedback Jan: meld the ring count limit into the series
> > v1: feedback #15 Jan: verify zero in unused hypercall args
> >
> >  xen/common/argo.c         | 118 
> > ++++++++++++++++++++++++++++++++++++++++++++++
> >  xen/common/compat/argo.c  |   1 +
> >  xen/include/public/argo.h |  19 ++++++++
> >  xen/include/xlat.lst      |   1 +
> >  4 files changed, 139 insertions(+)
> >
> > diff --git a/xen/common/argo.c b/xen/common/argo.c
> > index 076ee6c..3f95f80 100644
> > --- a/xen/common/argo.c
> > +++ b/xen/common/argo.c
> > @@ -43,6 +43,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_argo_addr_t);
> >  DEFINE_XEN_GUEST_HANDLE(xen_argo_gfn_t);
> >  DEFINE_XEN_GUEST_HANDLE(xen_argo_register_ring_t);
> >  DEFINE_XEN_GUEST_HANDLE(xen_argo_ring_t);
> > +DEFINE_XEN_GUEST_HANDLE(xen_argo_unregister_ring_t);
> >
> >  /* Xen command line option to enable argo */
> >  static bool __read_mostly opt_argo_enabled;
> > @@ -327,6 +328,33 @@ find_ring_info(const struct domain *d, const struct 
> > argo_ring_id *id)
> >      return NULL;
> >  }
> >
> > +static struct argo_send_info *
> > +find_send_info(const struct domain *d, const struct argo_ring_id *id)
> > +{
> > +    struct hlist_node *node;
> > +    struct argo_send_info *send_info;
> > +
> > +    ASSERT(LOCKING_send_L2(d));
> > +
> > +    hlist_for_each_entry(send_info, node, 
> > &d->argo->send_hash[hash_index(id)],
> > +                         node)
> > +    {
> > +        const struct argo_ring_id *cmpid = &send_info->id;
> > +
> > +        if ( cmpid->aport == id->aport &&
> > +             cmpid->domain_id == id->domain_id &&
> > +             cmpid->partner_id == id->partner_id )
> > +        {
> > +            argo_dprintk("send_info=%p\n", send_info);
> > +            return send_info;
> > +        }
> > +    }
> > +    argo_dprintk("no send_info found for ring(%u:%x %u)\n",
> > +                 id->domain_id, id->aport, id->partner_id);
> > +
> > +    return NULL;
> > +}
> > +
> >  static void
> >  ring_unmap(const struct domain *d, struct argo_ring_info *ring_info)
> >  {
> > @@ -695,6 +723,81 @@ find_ring_mfns(struct domain *d, struct argo_ring_info 
> > *ring_info,
> >   * * simplify the out label path when lock release has been moved.
> >   */
> >  static long
> > +unregister_ring(struct domain *currd,
> > +                XEN_GUEST_HANDLE_PARAM(xen_argo_unregister_ring_t) 
> > unreg_hnd)
> > +{
> > +    xen_argo_unregister_ring_t unreg;
> > +    struct argo_ring_id ring_id;
> > +    struct argo_ring_info *ring_info;
> > +    struct argo_send_info *send_info = NULL;
> > +    struct domain *dst_d = NULL;
> > +    int ret = 0;
> > +
> > +    ASSERT(currd == current->domain);
> > +
> > +    if ( copy_from_guest(&unreg, unreg_hnd, 1) )
> > +        return -EFAULT;
> > +
> > +    if ( unreg.pad )
> > +        return -EINVAL;
> > +
> > +    ring_id.partner_id = unreg.partner_id;
> > +    ring_id.aport = unreg.aport;
> > +    ring_id.domain_id = currd->domain_id;
> > +
> > +    read_lock(&L1_global_argo_rwlock);
> > +
> > +    if ( !currd->argo )
> > +    {
> > +        ret = -ENODEV;
> > +        goto out_unlock;
> > +    }
> > +
> > +    write_lock(&currd->argo->rings_L2_rwlock);
> > +
> > +    ring_info = find_ring_info(currd, &ring_id);
> > +    if ( ring_info )
> > +    {
> > +        ring_remove_info(currd, ring_info);
> > +        currd->argo->ring_count--;
> > +    }
> > +
> > +    dst_d = get_domain_by_id(ring_id.partner_id);
> > +    if ( dst_d )
> > +    {
> > +        if ( dst_d->argo )
> > +        {
> > +            spin_lock(&dst_d->argo->send_L2_lock);
> > +
> > +            send_info = find_send_info(dst_d, &ring_id);
> > +            if ( send_info )
> > +                hlist_del(&send_info->node);
> > +
> > +            spin_unlock(&dst_d->argo->send_L2_lock);
> > +
> > +        }
> > +        put_domain(dst_d);
> > +    }
>
> Can you actually find send_info if ring_info returns NULL?
>
> The ringid in send_info would then be stale, and point to a
> non-existing ring?

Your observation is correct, and it means that the above logic can be
simplified a bit, so have done so. It can also skip the send_info
lookup if it is unregistering a wildcard ring, as determined by the
partner_id.

Christopher

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