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

From: Jan Beulich <jbeulich@suse.com>
To: Oleksandr Tyshchenko <olekstysh@gmail.com>
Cc: "Oleksandr Tyshchenko" <oleksandr_tyshchenko@epam.com>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Julien Grall" <julien@xen.org>,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Ian Jackson" <iwj@xenproject.org>, "Wei Liu" <wl@xen.org>,
	"Roger Pau Monné" <roger.pau@citrix.com>,
	"Julien Grall" <julien.grall@arm.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH V3 16/23] xen/mm: Handle properly reference in set_foreign_p2m_entry() on Arm
Date: Tue, 8 Dec 2020 15:24:18 +0100	[thread overview]
Message-ID: <5f1a2914-f894-0046-2911-9cccb5d94dbf@suse.com> (raw)
In-Reply-To: <1606732298-22107-17-git-send-email-olekstysh@gmail.com>

On 30.11.2020 11:31, Oleksandr Tyshchenko wrote:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1134,12 +1134,8 @@ static int acquire_resource(
>      xen_pfn_t mfn_list[32];
>      int rc;
>  
> -    /*
> -     * 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(currd) && !is_hardware_domain(currd) )
> +    if ( paging_mode_translate(currd) && !is_hardware_domain(currd) &&
> +         !arch_acquire_resource_check() )
>          return -EACCES;

Looks like I didn't express myself clearly enough when replying
to v2, by saying "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". While one may debate whether the
hwdom check may remain here, the "translated" one definitely
should move into the x86 hook. This (I think) will the also make
apparent that ...

> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -382,6 +382,19 @@ struct p2m_domain {
>  #endif
>  #include <xen/p2m-common.h>
>  
> +static inline bool arch_acquire_resource_check(void)
> +{
> +    /*
> +     * The reference counting of foreign entries in set_foreign_p2m_entry()
> +     * is not supported 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.
> +     */
> +    return false;
> +}

... the initial part of the comment is true only for translated
domains. The reference to hwdom in the latter part of the comment
(which merely gets moved here) is a good indication that the
hwdom check also wants moving here. In turn the check at the top
of p2m_add_foreign() should imo then also use this new function,
instead of effectively open-coding it (with a similar comment).
And x86's set_foreign_p2m_entry() may want to gain

    ASSERT(arch_acquire_resource_check(d));

perhaps alongside the same ASSERT() you add to the Arm variant.

Jan