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

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

On 21.01.21 15:57, Jan Beulich wrote:

Hi Jan

On 12.01.2021 22:52, Oleksandr Tyshchenko wrote:
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>

This patch implements reference counting of foreign entries in
in set_foreign_p2m_entry() on Arm. This is a mandatory action if
we want to run emulator (IOREQ server) in other than dom0 domain,
as we can't trust it to do the right thing if it is not running
in dom0. So we need to grab a reference on the page to avoid it

It is valid to always pass "p2m_map_foreign_rw" type to
guest_physmap_add_entry() since the current and foreign domains
would be always different. A case when they are equal would be
rejected by rcu_lock_remote_domain_by_id(). Besides the similar
comment in the code put a respective ASSERT() to catch incorrect
usage in future.

It was tested with IOREQ feature to confirm that all the pages given
to this function belong to a domain, so we can use the same approach
as for XENMAPSPACE_gmfn_foreign handling in xenmem_add_to_physmap_one().

This involves adding an extra parameter for the foreign domain to
set_foreign_p2m_entry() and a helper to indicate whether the arch
supports the reference counting of foreign entries and the restriction
for the hardware domain in the common code can be skipped for it.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
CC: Julien Grall <julien.grall@xxxxxxx>
[On Arm only]
Tested-by: Wei Chen <Wei.Chen@xxxxxxx>
In principle x86 parts
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>


However, being a maintainer of ...

--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -382,6 +382,22 @@ struct p2m_domain {
  #include <xen/p2m-common.h>
+static inline bool arch_acquire_resource_check(struct domain *d)
+    /*
+     * The reference counting of foreign entries in set_foreign_p2m_entry()
+     * is not supported for translated domains on x86.
+     *
+     * FIXME: Until foreign pages inserted into the P2M are properly
+     * reference counted, it is unsafe to allow mapping of
+     * resource pages unless the caller is the hardware domain.
+     */
+    if ( paging_mode_translate(d) && !is_hardware_domain(d) )
+        return false;
+    return true;

... this code, I'd like to ask that such constructs be avoided
and this be a single return statement:

     return !paging_mode_translate(d) || is_hardware_domain(d);

ok, looks better.

I also think you may want to consider dropping the initial
"The" from the comment. I'm further unconvinced "foreign
entries" needs saying when set_foreign_p2m_entry() deals with
exclusively such. In the end the original comment moved here
would probably suffice, no need for any more additions than
perhaps a simple "(see set_foreign_p2m_entry())".



Oleksandr Tyshchenko



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