[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 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> 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); 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())". Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |