Re: [Xen-devel] [PATCH v3 08/15] argo: implement the unregister op

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

On Thu, Jan 10, 2019 at 3:40 AM Roger Pau Monné <royger@xxxxxxxxxxx> wrote:
>
> On Mon, Jan 7, 2019 at 8:44 AM Christopher Clark
> <christopher.w.clark@xxxxxxxxx> 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>
> > ---
> > 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         | 115 
> > ++++++++++++++++++++++++++++++++++++++++++++++
> >  xen/include/public/argo.h |  19 ++++++++
> >  xen/include/xlat.lst      |   1 +
> >  3 files changed, 135 insertions(+)
> >
> > diff --git a/xen/common/argo.c b/xen/common/argo.c
> > index 11988e7..59ce8c4 100644
> > --- a/xen/common/argo.c
> > +++ b/xen/common/argo.c
> > @@ -37,6 +37,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_argo_addr_t);
> >  DEFINE_XEN_GUEST_HANDLE(xen_argo_page_descr_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;
> > @@ -666,6 +667,105 @@ ring_find_info(const struct domain *d, const struct 
> > argo_ring_id *id)
> >      return NULL;
> >  }
> >
> > +static struct argo_send_info *
> > +send_find_info(const struct domain *d, const struct argo_ring_id *id)
> > +{
> > +    struct hlist_node *node;
> > +    struct argo_send_info *send_info;
> > +
> > +    hlist_for_each_entry(send_info, node, 
> > &d->argo->send_hash[hash_index(id)],
> > +                         node)
> > +    {
> > +        struct argo_ring_id *cmpid = &send_info->id;
>
> Const.

ack, done

>
> > +
> > +        if ( cmpid->port == id->port &&
> > +             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\n");
>
> Is this message actually helpful without printing any of the
> parameters provided to the function?

Good point. I've added the ring data to the output.

>
> > +
> > +    return NULL;
> > +}
> > +
> > +static long
> > +unregister_ring(struct domain *currd,
>
> Same as the comment made on the other patch, if this parameter is the
> current domain there's no need to pass it around, or else it should be
> named d instead of currd.

Response is as per the other for this one: where currd has been
retained, it's now ASSERTed as matching current->domain, and otherwise
'd' is used.

>
> > +                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;
> > +    struct domain *dst_d = NULL;
> > +    int ret;
> > +
> > +    ret = copy_from_guest(&unreg, unreg_hnd, 1) ? -EFAULT : 0;
> > +    if ( ret )
> > +        goto out;
> > +
> > +    ret = unreg.pad ? -EINVAL : 0;
> > +    if ( ret )
> > +        goto out;
>
> I don't see the point in the out label when you could just use 'return
> -EINVAL' or -EFAULT directly here and above.

ack - have done that, much better now.

thanks

Christopher

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