[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH V2 16/23] xen/mm: Handle properly reference in set_foreign_p2m_entry() on Arm




On 12.11.20 14:18, Jan Beulich wrote:

Hi Jan

On 15.10.2020 18:44, Oleksandr Tyshchenko wrote:
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1380,6 +1380,27 @@ int guest_physmap_remove_page(struct domain *d, gfn_t 
gfn, mfn_t mfn,
      return p2m_remove_mapping(d, gfn, (1 << page_order), mfn);
  }
+int set_foreign_p2m_entry(struct domain *d, const struct domain *fd,
+                          unsigned long gfn, mfn_t mfn)
+{
+    struct page_info *page = mfn_to_page(mfn);
+    int rc;
+
+    if ( !get_page(page, fd) )
+        return -EINVAL;
+
+    /*
+     * It is valid to always use p2m_map_foreign_rw here as if this gets
+     * called that d != fd. A case when d == fd would be rejected by
+     * rcu_lock_remote_domain_by_id() earlier.
+     */
Are you describing things here on the assumption that no new
callers may surface later on? To catch such, I'd recommend
adding at least a respective ASSERT().

Agree, will add.



+    rc = guest_physmap_add_entry(d, _gfn(gfn), mfn, 0, p2m_map_foreign_rw);
+    if ( rc )
+        put_page(page);
+
+    return 0;
I can't imagine it's correct to not signal the error to the
caller(s).

It is not correct, I have just missed to place return rc. Thank you for the catch.



--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1099,7 +1099,8 @@ static int acquire_resource(
       *        reference counted, it is unsafe to allow mapping of
       *        resource pages unless the caller is the hardware domain.
       */
-    if ( paging_mode_translate(currd) && !is_hardware_domain(currd) )
+    if ( paging_mode_translate(currd) && !is_hardware_domain(currd) &&
+         !arch_refcounts_p2m() )
          return -EACCES;
I guess the hook may want naming differently, as both prior parts of
the condition should be needed only on the x86 side, and there (for
PV) there's no p2m involved in the refcounting. Maybe
arch_acquire_resource_check()? And then the comment wants moving into
the x86 hook as well. You may want to consider leaving a more generic
one here...

ok, again, I am fine with the name). Will follow everything suggested above.

--
Regards,

Oleksandr Tyshchenko




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.