[PATCH 0/5] domain referencing adjustments

[PATCH 0/5] domain referencing adjustments

[Jan Beulich]

In the course of preparing v4 of "evtchn: (not so) recent XSAs follow-on"
(which is a contextual prereq to some of the patches here) I've noticed
some slight inefficiencies. I've then also looked for similar patterns
elsewhere.

1: common: don't (kind of) open-code rcu_lock_domain_by_any_id()
2: evtchn: don't pointlessly use get_domain()
3: argo: don't pointlessly use get_domain_by_id()
4: x86: don't pointlessly use get_domain_by_id()
5: x86/mem-sharing: don't pointlessly use get_domain_by_id()

Jan

[PATCH 1/5] common: don't (kind of) open-code rcu_lock_domain_by_any_id()

[Jan Beulich]

Even more so when using rcu_lock_domain_by_id() in place of the more
efficient rcu_lock_current_domain().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Besides get_pg_owner(), gnttab_copy_lock_domain() has similar strange(?)
behavior: They accept DOMID_SELF, but not the resolved ID of the caller.
---
v4: New.

--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -353,10 +353,7 @@ static int evtchn_bind_interdomain(evtch
     evtchn_port_t  rport = bind->remote_port;
     domid_t        rdom = bind->remote_dom;
 
-    if ( rdom == DOMID_SELF )
-        rdom = current->domain->domain_id;
-
-    if ( (rd = rcu_lock_domain_by_id(rdom)) == NULL )
+    if ( (rd = rcu_lock_domain_by_any_id(rdom)) == NULL )
         return -ESRCH;
 
     write_lock(&ld->event_lock);
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2568,12 +2568,6 @@ struct domain *get_pg_owner(domid_t domi
 {
     struct domain *pg_owner = NULL, *curr = current->domain;
 
-    if ( likely(domid == DOMID_SELF) )
-    {
-        pg_owner = rcu_lock_current_domain();
-        goto out;
-    }
-
     if ( unlikely(domid == curr->domain_id) )
     {
         gdprintk(XENLOG_WARNING, "Cannot specify itself as foreign domain\n");
@@ -2591,7 +2585,7 @@ struct domain *get_pg_owner(domid_t domi
         break;
 
     default:
-        if ( (pg_owner = rcu_lock_domain_by_id(domid)) == NULL )
+        if ( (pg_owner = rcu_lock_domain_by_any_id(domid)) == NULL )
             gdprintk(XENLOG_WARNING, "Unknown domain d%d\n", domid);
         break;
     }

[PATCH 2/5] evtchn: don't pointlessly use get_domain()

[Jan Beulich]

For short-lived references rcu_lock_domain() is the better (slightly
cheaper) alternative.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -642,7 +642,7 @@ int evtchn_close(struct domain *d1, int
              */
             write_unlock(&chn1->lock);
             write_unlock(&chn2->lock);
-            put_domain(d2);
+            rcu_unlock_domain(d2);
             chn2 = NULL;
         }
 
@@ -677,7 +677,7 @@ int evtchn_close(struct domain *d1, int
              */
             write_unlock(&chn1->lock);
             write_unlock(&chn2->lock);
-            put_domain(d2);
+            rcu_unlock_domain(d2);
             chn2 = NULL;
         }
 
@@ -725,9 +725,8 @@ int evtchn_close(struct domain *d1, int
 
         if ( !chn2 )
         {
-            /* If we unlock chn1 then we could lose d2. Must get a reference. */
-            if ( unlikely(!get_domain(d2)) )
-                BUG();
+            /* If we unlock chn1 then we could lose d2. */
+            rcu_lock_domain(d2);
 
             chn2 = _evtchn_from_port(d2, chn1->u.interdomain.remote_port);
             BUG_ON(!chn2);
@@ -778,7 +777,7 @@ int evtchn_close(struct domain *d1, int
          */
         write_unlock(&chn1->lock);
         write_unlock(&chn2->lock);
-        put_domain(d2);
+        rcu_unlock_domain(d2);
     }
 
     write_unlock(&d1->event_lock);

[PATCH 3/5] argo: don't pointlessly use get_domain_by_id()

[Jan Beulich]

For short-lived references rcu_lock_domain_by_id() is the better
(slightly cheaper) alternative.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Is it really intentional for fill_ring_data() to return success (0) in
case the domain can't be found or has argo disabled? Even if so, the
uninitialized ent.max_message_size will be leaked to the caller then
(and similarly when find_ring_info_by_match() returns NULL). Perhaps
the field should only be copied back when ent.flags is non-zero?

--- a/xen/common/argo.c
+++ b/xen/common/argo.c
@@ -445,13 +445,13 @@ signal_domain(struct domain *d)
 static void
 signal_domid(domid_t domain_id)
 {
-    struct domain *d = get_domain_by_id(domain_id);
+    struct domain *d = rcu_lock_domain_by_id(domain_id);
 
     if ( !d )
         return;
 
     signal_domain(d);
-    put_domain(d);
+    rcu_unlock_domain(d);
 }
 
 static void
@@ -983,7 +983,7 @@ ringbuf_insert(const struct domain *d, s
 static void
 wildcard_pending_list_remove(domid_t domain_id, struct pending_ent *ent)
 {
-    struct domain *d = get_domain_by_id(domain_id);
+    struct domain *d = rcu_lock_domain_by_id(domain_id);
 
     if ( !d )
         return;
@@ -996,13 +996,13 @@ wildcard_pending_list_remove(domid_t dom
         list_del(&ent->wildcard_node);
         spin_unlock(&d->argo->wildcard_L2_lock);
     }
-    put_domain(d);
+    rcu_unlock_domain(d);
 }
 
 static void
 wildcard_pending_list_insert(domid_t domain_id, struct pending_ent *ent)
 {
-    struct domain *d = get_domain_by_id(domain_id);
+    struct domain *d = rcu_lock_domain_by_id(domain_id);
 
     if ( !d )
         return;
@@ -1015,7 +1015,7 @@ wildcard_pending_list_insert(domid_t dom
         list_add(&ent->wildcard_node, &d->argo->wildcard_pend_list);
         spin_unlock(&d->argo->wildcard_L2_lock);
     }
-    put_domain(d);
+    rcu_unlock_domain(d);
 }
 
 static void
@@ -1283,7 +1283,7 @@ partner_rings_remove(struct domain *src_
                                                       struct argo_send_info,
                                                       node)) )
         {
-            struct domain *dst_d = get_domain_by_id(send_info->id.domain_id);
+            struct domain *dst_d = rcu_lock_domain_by_id(send_info->id.domain_id);
 
             if ( dst_d && dst_d->argo )
             {
@@ -1302,7 +1302,7 @@ partner_rings_remove(struct domain *src_
                 ASSERT_UNREACHABLE();
 
             if ( dst_d )
-                put_domain(dst_d);
+                rcu_unlock_domain(dst_d);
 
             list_del(&send_info->node);
             xfree(send_info);
@@ -1330,7 +1330,7 @@ fill_ring_data(const struct domain *curr
 
     ent.flags = 0;
 
-    dst_d = get_domain_by_id(ent.ring.domain_id);
+    dst_d = rcu_lock_domain_by_id(ent.ring.domain_id);
     if ( !dst_d || !dst_d->argo )
         goto out;
 
@@ -1340,10 +1340,7 @@ fill_ring_data(const struct domain *curr
      */
     ret = xsm_argo_send(currd, dst_d);
     if ( ret )
-    {
-        put_domain(dst_d);
-        return ret;
-    }
+        goto out;
 
     read_lock(&dst_d->argo->rings_L2_rwlock);
 
@@ -1405,7 +1402,7 @@ fill_ring_data(const struct domain *curr
 
  out:
     if ( dst_d )
-        put_domain(dst_d);
+        rcu_unlock_domain(dst_d);
 
     if ( !ret && (__copy_field_to_guest(data_ent_hnd, &ent, flags) ||
                   __copy_field_to_guest(data_ent_hnd, &ent, max_message_size)) )
@@ -1569,7 +1566,7 @@ unregister_ring(struct domain *currd,
     if ( ring_id.partner_id == XEN_ARGO_DOMID_ANY )
         goto out;
 
-    dst_d = get_domain_by_id(ring_id.partner_id);
+    dst_d = rcu_lock_domain_by_id(ring_id.partner_id);
     if ( !dst_d || !dst_d->argo )
     {
         ASSERT_UNREACHABLE();
@@ -1592,7 +1589,7 @@ unregister_ring(struct domain *currd,
     read_unlock(&L1_global_argo_rwlock);
 
     if ( dst_d )
-        put_domain(dst_d);
+        rcu_unlock_domain(dst_d);
 
     xfree(send_info);
 
@@ -1663,7 +1660,7 @@ register_ring(struct domain *currd,
     }
     else
     {
-        dst_d = get_domain_by_id(reg.partner_id);
+        dst_d = rcu_lock_domain_by_id(reg.partner_id);
         if ( !dst_d )
         {
             argo_dprintk("!dst_d, ESRCH\n");
@@ -1845,7 +1842,7 @@ register_ring(struct domain *currd,
 
  out:
     if ( dst_d )
-        put_domain(dst_d);
+        rcu_unlock_domain(dst_d);
 
     if ( ret )
         xfree(send_info);
@@ -1988,7 +1985,7 @@ sendv(struct domain *src_d, xen_argo_add
     src_id.domain_id = src_d->domain_id;
     src_id.partner_id = dst_addr->domain_id;
 
-    dst_d = get_domain_by_id(dst_addr->domain_id);
+    dst_d = rcu_lock_domain_by_id(dst_addr->domain_id);
     if ( !dst_d )
         return -ESRCH;
 
@@ -1998,7 +1995,7 @@ sendv(struct domain *src_d, xen_argo_add
         gprintk(XENLOG_ERR, "argo: XSM REJECTED %i -> %i\n",
                 src_d->domain_id, dst_d->domain_id);
 
-        put_domain(dst_d);
+        rcu_unlock_domain(dst_d);
 
         return ret;
     }
@@ -2068,7 +2065,7 @@ sendv(struct domain *src_d, xen_argo_add
         signal_domain(dst_d);
 
     if ( dst_d )
-        put_domain(dst_d);
+        rcu_unlock_domain(dst_d);
 
     return ( ret < 0 ) ? ret : len;
 }

[PATCH 4/5] x86: don't pointlessly use get_domain_by_id()

[Jan Beulich]

For short-lived references rcu_lock_domain_by_id() is the better
(slightly cheaper) alternative.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/cpu/mcheck/mcaction.c
+++ b/xen/arch/x86/cpu/mcheck/mcaction.c
@@ -87,7 +87,7 @@ mc_memerr_dhandler(struct mca_binfo *bin
             BUG_ON( bank->mc_domid == DOMID_COW );
             if ( bank->mc_domid != DOMID_XEN )
             {
-                d = get_domain_by_id(bank->mc_domid);
+                d = rcu_lock_domain_by_id(bank->mc_domid);
                 ASSERT(d);
                 gfn = get_gpfn_from_mfn((bank->mc_addr) >> PAGE_SHIFT);
 
@@ -132,6 +132,8 @@ mc_memerr_dhandler(struct mca_binfo *bin
                     goto vmce_failed;
                 }
 
+                rcu_unlock_domain(d);
+
                 /*
                  * Impacted domain go on with domain's recovery job
                  * if the domain has its own MCA handler.
@@ -139,12 +141,11 @@ mc_memerr_dhandler(struct mca_binfo *bin
                  * its own recovery job.
                  */
                 *result = MCER_RECOVERED;
-                put_domain(d);
 
                 return;
 vmce_failed:
-                put_domain(d);
                 domain_crash(d);
+                rcu_unlock_domain(d);
             }
         }
     }
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -1497,7 +1497,6 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_m
 
         if ( mc_msrinject->mcinj_flags & MC_MSRINJ_F_GPADDR )
         {
-            domid_t domid;
             struct domain *d;
             struct mcinfo_msr *msr;
             unsigned int i;
@@ -1505,17 +1504,17 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_m
             unsigned long gfn, mfn;
             p2m_type_t t;
 
-            domid = (mc_msrinject->mcinj_domid == DOMID_SELF) ?
-                    current->domain->domain_id : mc_msrinject->mcinj_domid;
-            if ( domid >= DOMID_FIRST_RESERVED )
-                return x86_mcerr("do_mca inject: incompatible flag "
-                                 "MC_MSRINJ_F_GPADDR with domain %d",
-                                 -EINVAL, domid);
-
-            d = get_domain_by_id(domid);
+            d = rcu_lock_domain_by_any_id(mc_msrinject->mcinj_domid);
             if ( d == NULL )
+            {
+                if ( mc_msrinject->mcinj_domid >= DOMID_FIRST_RESERVED )
+                    return x86_mcerr("do_mca inject: incompatible flag "
+                                     "MC_MSRINJ_F_GPADDR with domain %d",
+                                     -EINVAL, domid);
+
                 return x86_mcerr("do_mca inject: bad domain id %d",
                                  -EINVAL, domid);
+            }
 
             for ( i = 0, msr = &mc_msrinject->mcinj_msr[0];
                   i < mc_msrinject->mcinj_count;
@@ -1528,7 +1527,7 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_m
                 if ( mfn == mfn_x(INVALID_MFN) )
                 {
                     put_gfn(d, gfn);
-                    put_domain(d);
+                    rcu_unlock_domain(d);
                     return x86_mcerr("do_mca inject: bad gfn %#lx of domain %d",
                                      -EINVAL, gfn, domid);
                 }
@@ -1538,7 +1537,7 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_m
                 put_gfn(d, gfn);
             }
 
-            put_domain(d);
+            rcu_unlock_domain(d);
         }
 
         if ( !x86_mc_msrinject_verify(mc_msrinject) )
--- a/xen/arch/x86/debug.c
+++ b/xen/arch/x86/debug.c
@@ -164,13 +164,13 @@ unsigned int dbg_rw_mem(void * __user ad
                         unsigned int len, domid_t domid, bool toaddr,
                         uint64_t pgd3)
 {
-        struct domain *d = get_domain_by_id(domid);
+    struct domain *d = rcu_lock_domain_by_id(domid);
 
     if ( d )
     {
         if ( !d->is_dying )
             len = dbg_rw_guest_mem(d, addr, buf, len, toaddr, pgd3);
-        put_domain(d);
+        rcu_unlock_domain(d);
     }
 
     return len;
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -323,7 +323,7 @@ void destroy_irq(unsigned int irq)
 
     if ( desc->arch.creator_domid != DOMID_INVALID )
     {
-        struct domain *d = get_domain_by_id(desc->arch.creator_domid);
+        struct domain *d = rcu_lock_domain_by_id(desc->arch.creator_domid);
 
         if ( d )
         {
@@ -334,7 +334,7 @@ void destroy_irq(unsigned int irq)
                        "Could not revoke %pd access to IRQ%u (error %d)\n",
                        d, irq, err);
 
-            put_domain(d);
+            rcu_unlock_domain(d);
         }
 
         desc->arch.creator_domid = DOMID_INVALID;

[PATCH 5/5] x86/mem-sharing: don't pointlessly use get_domain_by_id()

[Jan Beulich]

For short-lived references rcu_lock_domain_by_id() is the better
(slightly cheaper) alternative.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -537,7 +537,7 @@ static int audit(void)
             p2m_type_t t;
             mfn_t o_mfn;
 
-            d = get_domain_by_id(g->domain);
+            d = rcu_lock_domain_by_id(g->domain);
             if ( d == NULL )
             {
                 gdprintk(XENLOG_ERR,
@@ -562,7 +562,7 @@ static int audit(void)
                          d, g->gfn, mfn_x(mfn), p2m_ram_shared, t);
                 errors++;
             }
-            put_domain(d);
+            rcu_unlock_domain(d);
             nr_gfns++;
         }
         /* The type count has an extra ref because we have locked the page */
@@ -1043,10 +1043,10 @@ static int share_pages(struct domain *sd
         rmap_del(gfn, cpage, 0);
         rmap_add(gfn, spage);
         put_count++;
-        d = get_domain_by_id(gfn->domain);
+        d = rcu_lock_domain_by_id(gfn->domain);
         BUG_ON(!d);
         BUG_ON(set_shared_p2m_entry(d, gfn->gfn, smfn));
-        put_domain(d);
+        rcu_unlock_domain(d);
     }
     ASSERT(list_empty(&cpage->sharing->gfns));
     BUG_ON(!put_count);

Re: [PATCH 5/5] x86/mem-sharing: don't pointlessly use get_domain_by_id()

[Tamas K Lengyel]

On Tue, Jan 5, 2021 at 8:28 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> For short-lived references rcu_lock_domain_by_id() is the better
> (slightly cheaper) alternative.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

Re: [PATCH 1/5] common: don't (kind of) open-code rcu_lock_domain_by_any_id()

[Andrew Cooper]

On 05/01/2021 13:24, Jan Beulich wrote:
> Even more so when using rcu_lock_domain_by_id() in place of the more
> efficient rcu_lock_current_domain().
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
> Besides get_pg_owner(), gnttab_copy_lock_domain() has similar strange(?)
> behavior: They accept DOMID_SELF, but not the resolved ID of the caller.

I queried that behaviour years and years ago.

According to Keir, it is part of the attempt to force guests to not know
their own domid.  Xen itself does (or did) its very hardest to prevent a
domU knowing its own domid, while at the same time the "domid" key in
xenstore is mandatory for setting up PV rings.

I'd personally think was a short sighted move.

~Andrew

Re: [PATCH 1/5] common: don't (kind of) open-code rcu_lock_domain_by_any_id()

[Jan Beulich]

On 05.01.2021 14:38, Andrew Cooper wrote:
> On 05/01/2021 13:24, Jan Beulich wrote:
>> Even more so when using rcu_lock_domain_by_id() in place of the more
>> efficient rcu_lock_current_domain().
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

>> ---
>> Besides get_pg_owner(), gnttab_copy_lock_domain() has similar strange(?)
>> behavior: They accept DOMID_SELF, but not the resolved ID of the caller.
> 
> I queried that behaviour years and years ago.
> 
> According to Keir, it is part of the attempt to force guests to not know
> their own domid.  Xen itself does (or did) its very hardest to prevent a
> domU knowing its own domid, while at the same time the "domid" key in
> xenstore is mandatory for setting up PV rings.
> 
> I'd personally think was a short sighted move.

Let me make another patch then, unless you can see reasons we should
stick to the current behavior. Figuring out its own domid is possible
for a domain, after all, and even shouldn't be overly difficult.

Jan

Re: [PATCH 3/5] argo: don't pointlessly use get_domain_by_id()

[Christopher Clark]

On Tue, Jan 5, 2021 at 5:26 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> For short-lived references rcu_lock_domain_by_id() is the better
> (slightly cheaper) alternative.

This should scale better than the existing use of get_domain_by_id.

I actually found it pretty hard to study the code for the effects of
switching over to use the RCU domain reference logic, and I would have
been happier with reviewing if I'd been able to exercise it with some
focused testing; XTF needs support for tests with multiple test VMs to
enable Argo tests of communication and interactions with hypervisor
state.

> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Christopher Clark <christopher.w.clark@gmail.com>

> ---
> Is it really intentional for fill_ring_data() to return success (0) in
> case the domain can't be found or has argo disabled?

Good question; I think this logic can and should be improved. I will
work on a patch.

> Even if so, the
> uninitialized ent.max_message_size will be leaked to the caller then
> (and similarly when find_ring_info_by_match() returns NULL). Perhaps
> the field should only be copied back when ent.flags is non-zero?

Yes.

thanks,

Christopher

>
> --- a/xen/common/argo.c
> +++ b/xen/common/argo.c
> @@ -445,13 +445,13 @@ signal_domain(struct domain *d)
>  static void
>  signal_domid(domid_t domain_id)
>  {
> -    struct domain *d = get_domain_by_id(domain_id);
> +    struct domain *d = rcu_lock_domain_by_id(domain_id);
>
>      if ( !d )
>          return;
>
>      signal_domain(d);
> -    put_domain(d);
> +    rcu_unlock_domain(d);
>  }
>
>  static void
> @@ -983,7 +983,7 @@ ringbuf_insert(const struct domain *d, s
>  static void
>  wildcard_pending_list_remove(domid_t domain_id, struct pending_ent *ent)
>  {
> -    struct domain *d = get_domain_by_id(domain_id);
> +    struct domain *d = rcu_lock_domain_by_id(domain_id);
>
>      if ( !d )
>          return;
> @@ -996,13 +996,13 @@ wildcard_pending_list_remove(domid_t dom
>          list_del(&ent->wildcard_node);
>          spin_unlock(&d->argo->wildcard_L2_lock);
>      }
> -    put_domain(d);
> +    rcu_unlock_domain(d);
>  }
>
>  static void
>  wildcard_pending_list_insert(domid_t domain_id, struct pending_ent *ent)
>  {
> -    struct domain *d = get_domain_by_id(domain_id);
> +    struct domain *d = rcu_lock_domain_by_id(domain_id);
>
>      if ( !d )
>          return;
> @@ -1015,7 +1015,7 @@ wildcard_pending_list_insert(domid_t dom
>          list_add(&ent->wildcard_node, &d->argo->wildcard_pend_list);
>          spin_unlock(&d->argo->wildcard_L2_lock);
>      }
> -    put_domain(d);
> +    rcu_unlock_domain(d);
>  }
>
>  static void
> @@ -1283,7 +1283,7 @@ partner_rings_remove(struct domain *src_
>                                                        struct argo_send_info,
>                                                        node)) )
>          {
> -            struct domain *dst_d = get_domain_by_id(send_info->id.domain_id);
> +            struct domain *dst_d = rcu_lock_domain_by_id(send_info->id.domain_id);
>
>              if ( dst_d && dst_d->argo )
>              {
> @@ -1302,7 +1302,7 @@ partner_rings_remove(struct domain *src_
>                  ASSERT_UNREACHABLE();
>
>              if ( dst_d )
> -                put_domain(dst_d);
> +                rcu_unlock_domain(dst_d);
>
>              list_del(&send_info->node);
>              xfree(send_info);
> @@ -1330,7 +1330,7 @@ fill_ring_data(const struct domain *curr
>
>      ent.flags = 0;
>
> -    dst_d = get_domain_by_id(ent.ring.domain_id);
> +    dst_d = rcu_lock_domain_by_id(ent.ring.domain_id);
>      if ( !dst_d || !dst_d->argo )
>          goto out;
>
> @@ -1340,10 +1340,7 @@ fill_ring_data(const struct domain *curr
>       */
>      ret = xsm_argo_send(currd, dst_d);
>      if ( ret )
> -    {
> -        put_domain(dst_d);
> -        return ret;
> -    }
> +        goto out;
>
>      read_lock(&dst_d->argo->rings_L2_rwlock);
>
> @@ -1405,7 +1402,7 @@ fill_ring_data(const struct domain *curr
>
>   out:
>      if ( dst_d )
> -        put_domain(dst_d);
> +        rcu_unlock_domain(dst_d);
>
>      if ( !ret && (__copy_field_to_guest(data_ent_hnd, &ent, flags) ||
>                    __copy_field_to_guest(data_ent_hnd, &ent, max_message_size)) )
> @@ -1569,7 +1566,7 @@ unregister_ring(struct domain *currd,
>      if ( ring_id.partner_id == XEN_ARGO_DOMID_ANY )
>          goto out;
>
> -    dst_d = get_domain_by_id(ring_id.partner_id);
> +    dst_d = rcu_lock_domain_by_id(ring_id.partner_id);
>      if ( !dst_d || !dst_d->argo )
>      {
>          ASSERT_UNREACHABLE();
> @@ -1592,7 +1589,7 @@ unregister_ring(struct domain *currd,
>      read_unlock(&L1_global_argo_rwlock);
>
>      if ( dst_d )
> -        put_domain(dst_d);
> +        rcu_unlock_domain(dst_d);
>
>      xfree(send_info);
>
> @@ -1663,7 +1660,7 @@ register_ring(struct domain *currd,
>      }
>      else
>      {
> -        dst_d = get_domain_by_id(reg.partner_id);
> +        dst_d = rcu_lock_domain_by_id(reg.partner_id);
>          if ( !dst_d )
>          {
>              argo_dprintk("!dst_d, ESRCH\n");
> @@ -1845,7 +1842,7 @@ register_ring(struct domain *currd,
>
>   out:
>      if ( dst_d )
> -        put_domain(dst_d);
> +        rcu_unlock_domain(dst_d);
>
>      if ( ret )
>          xfree(send_info);
> @@ -1988,7 +1985,7 @@ sendv(struct domain *src_d, xen_argo_add
>      src_id.domain_id = src_d->domain_id;
>      src_id.partner_id = dst_addr->domain_id;
>
> -    dst_d = get_domain_by_id(dst_addr->domain_id);
> +    dst_d = rcu_lock_domain_by_id(dst_addr->domain_id);
>      if ( !dst_d )
>          return -ESRCH;
>
> @@ -1998,7 +1995,7 @@ sendv(struct domain *src_d, xen_argo_add
>          gprintk(XENLOG_ERR, "argo: XSM REJECTED %i -> %i\n",
>                  src_d->domain_id, dst_d->domain_id);
>
> -        put_domain(dst_d);
> +        rcu_unlock_domain(dst_d);
>
>          return ret;
>      }
> @@ -2068,7 +2065,7 @@ sendv(struct domain *src_d, xen_argo_add
>          signal_domain(dst_d);
>
>      if ( dst_d )
> -        put_domain(dst_d);
> +        rcu_unlock_domain(dst_d);
>
>      return ( ret < 0 ) ? ret : len;
>  }
>

Re: [PATCH 1/5] common: don't (kind of) open-code rcu_lock_domain_by_any_id()

[Andrew Cooper]

On 05/01/2021 14:17, Jan Beulich wrote:
>>> ---
>>> Besides get_pg_owner(), gnttab_copy_lock_domain() has similar strange(?)
>>> behavior: They accept DOMID_SELF, but not the resolved ID of the caller.
>> I queried that behaviour years and years ago.
>>
>> According to Keir, it is part of the attempt to force guests to not know
>> their own domid.  Xen itself does (or did) its very hardest to prevent a
>> domU knowing its own domid, while at the same time the "domid" key in
>> xenstore is mandatory for setting up PV rings.
>>
>> I'd personally think was a short sighted move.
> Let me make another patch then, unless you can see reasons we should
> stick to the current behavior. Figuring out its own domid is possible
> for a domain, after all, and even shouldn't be overly difficult.

I don't see any reason to keep these semantics, other than the general
argument against relaxing things vs compatibility with older hypervisors.

I honestly can't make up my mind, so whichever you prefer.

~Andrew