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

[Oleksandr <olekstysh@xxxxxxxxx>]

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