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

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

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?

Thanks, Roger.

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