[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 disappearing. 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> Thanks. 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 { #endif #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())". ok -- Regards, Oleksandr Tyshchenko
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |