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

Commit Message

Oleksandr Tyshchenko Nov. 30, 2020, 10:31 a.m. UTC

From: Oleksandr Tyshchenko <[e-mail]>

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 <[e-mail]>
CC: Julien Grall <[e-mail]>

---
Please note, this is a split/cleanup/hardening of Julien's PoC:
"Add support for Guest IO forwarding to a device emulator"

Changes RFC -> V1:
   - new patch, was split from:
     "[RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features"
   - rewrite a logic to handle properly reference in set_foreign_p2m_entry()
     instead of treating foreign entries as p2m_ram_rw

Changes V1 -> V2:
   - rebase according to the recent changes to acquire_resource()
   - update patch description
   - introduce arch_refcounts_p2m()
   - add an explanation why p2m_map_foreign_rw is valid
   - move set_foreign_p2m_entry() to p2m-common.h
   - add const to new parameter

Changes V2 -> V3:
   - update patch description
   - rename arch_refcounts_p2m() to arch_acquire_resource_check()
   - move comment to x86’s arch_acquire_resource_check()
   - return rc in Arm's set_foreign_p2m_entry()
   - put a respective ASSERT() into Arm's set_foreign_p2m_entry()
---
---
 xen/arch/arm/p2m.c           | 24 ++++++++++++++++++++++++
 xen/arch/x86/mm/p2m.c        |  5 +++--
 xen/common/memory.c          | 10 +++-------
 xen/include/asm-arm/p2m.h    | 19 +++++++++----------
 xen/include/asm-x86/p2m.h    | 16 +++++++++++++---
 xen/include/xen/p2m-common.h |  4 ++++
 6 files changed, 56 insertions(+), 22 deletions(-)

Comments

Jan Beulich Dec. 8, 2020, 2:24 p.m. UTC | #1

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

Oleksandr Tyshchenko Dec. 8, 2020, 4:41 p.m. UTC | #2

On 08.12.20 16:24, Jan Beulich wrote:

Hi Jan

> 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.

Well, will do. I was about to mention, that new function wanted to gain 
domain as parameter, but noticed you had given a hint in the ASSERT 
example.

Stefano Stabellini Dec. 9, 2020, 11:49 p.m. UTC | #3

On Mon, 30 Nov 2020, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <[e-mail]>
> 
> 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 <[e-mail]>
> CC: Julien Grall <[e-mail]>

The arm side looks OK to me


> ---
> Please note, this is a split/cleanup/hardening of Julien's PoC:
> "Add support for Guest IO forwarding to a device emulator"
> 
> Changes RFC -> V1:
>    - new patch, was split from:
>      "[RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features"
>    - rewrite a logic to handle properly reference in set_foreign_p2m_entry()
>      instead of treating foreign entries as p2m_ram_rw
> 
> Changes V1 -> V2:
>    - rebase according to the recent changes to acquire_resource()
>    - update patch description
>    - introduce arch_refcounts_p2m()
>    - add an explanation why p2m_map_foreign_rw is valid
>    - move set_foreign_p2m_entry() to p2m-common.h
>    - add const to new parameter
> 
> Changes V2 -> V3:
>    - update patch description
>    - rename arch_refcounts_p2m() to arch_acquire_resource_check()
>    - move comment to x86’s arch_acquire_resource_check()
>    - return rc in Arm's set_foreign_p2m_entry()
>    - put a respective ASSERT() into Arm's set_foreign_p2m_entry()
> ---
> ---
>  xen/arch/arm/p2m.c           | 24 ++++++++++++++++++++++++
>  xen/arch/x86/mm/p2m.c        |  5 +++--
>  xen/common/memory.c          | 10 +++-------
>  xen/include/asm-arm/p2m.h    | 19 +++++++++----------
>  xen/include/asm-x86/p2m.h    | 16 +++++++++++++---
>  xen/include/xen/p2m-common.h |  4 ++++
>  6 files changed, 56 insertions(+), 22 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 4eeb867..5b8d494 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1380,6 +1380,30 @@ int guest_physmap_remove_page(struct domain *d, gfn_t gfn, mfn_t mfn,
>      return p2m_remove_mapping(d, gfn, (1 << page_order), mfn);
>  }
>  
> +int set_foreign_p2m_entry(struct domain *d, const struct domain *fd,
> +                          unsigned long gfn, mfn_t mfn)
> +{
> +    struct page_info *page = mfn_to_page(mfn);
> +    int rc;
> +
> +    if ( !get_page(page, fd) )
> +        return -EINVAL;
> +
> +    /*
> +     * It is valid to always use p2m_map_foreign_rw here as if this gets
> +     * called then d != fd. A case when d == fd would be rejected by
> +     * rcu_lock_remote_domain_by_id() earlier. Put a respective ASSERT()
> +     * to catch incorrect usage in future.
> +     */
> +    ASSERT(d != fd);
> +
> +    rc = guest_physmap_add_entry(d, _gfn(gfn), mfn, 0, p2m_map_foreign_rw);
> +    if ( rc )
> +        put_page(page);
> +
> +    return rc;
> +}
> +
>  static struct page_info *p2m_allocate_root(void)
>  {
>      struct page_info *page;
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 7a2ba82..4772c86 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1321,7 +1321,8 @@ static int set_typed_p2m_entry(struct domain *d, unsigned long gfn_l,
>  }
>  
>  /* Set foreign mfn in the given guest's p2m table. */
> -int set_foreign_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
> +int set_foreign_p2m_entry(struct domain *d, const struct domain *fd,
> +                          unsigned long gfn, mfn_t mfn)
>  {
>      return set_typed_p2m_entry(d, gfn, mfn, PAGE_ORDER_4K, p2m_map_foreign,
>                                 p2m_get_hostp2m(d)->default_access);
> @@ -2621,7 +2622,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
>       * will update the m2p table which will result in  mfn -> gpfn of dom0
>       * and not fgfn of domU.
>       */
> -    rc = set_foreign_p2m_entry(tdom, gpfn, mfn);
> +    rc = set_foreign_p2m_entry(tdom, fdom, gpfn, mfn);
>      if ( rc )
>          gdprintk(XENLOG_WARNING, "set_foreign_p2m_entry failed. "
>                   "gpfn:%lx mfn:%lx fgfn:%lx td:%d fd:%d\n",
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 3363c06..49e3001 100644
> --- 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;
>  
>      if ( copy_from_guest(&xmar, arg, 1) )
> @@ -1207,7 +1203,7 @@ static int acquire_resource(
>  
>          for ( i = 0; !rc && i < xmar.nr_frames; i++ )
>          {
> -            rc = set_foreign_p2m_entry(currd, gfn_list[i],
> +            rc = set_foreign_p2m_entry(currd, d, gfn_list[i],
>                                         _mfn(mfn_list[i]));
>              /* rc should be -EIO for any iteration other than the first */
>              if ( rc && i )
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 28ca9a8..4f8056e 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -161,6 +161,15 @@ typedef enum {
>  #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 supported on Arm.
> +     */
> +    return true;
> +}
> +
>  static inline
>  void p2m_altp2m_check(struct vcpu *v, uint16_t idx)
>  {
> @@ -392,16 +401,6 @@ static inline gfn_t gfn_next_boundary(gfn_t gfn, unsigned int order)
>      return gfn_add(gfn, 1UL << order);
>  }
>  
> -static inline int set_foreign_p2m_entry(struct domain *d, unsigned long gfn,
> -                                        mfn_t mfn)
> -{
> -    /*
> -     * NOTE: If this is implemented then proper reference counting of
> -     *       foreign entries will need to be implemented.
> -     */
> -    return -EOPNOTSUPP;
> -}
> -
>  /*
>   * A vCPU has cache enabled only when the MMU is enabled and data cache
>   * is enabled.
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index 4603560..8d2dc22 100644
> --- 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;
> +}
> +
>  /*
>   * Updates vCPU's n2pm to match its np2m_base in VMCx12 and returns that np2m.
>   */
> @@ -647,9 +660,6 @@ int p2m_finish_type_change(struct domain *d,
>  int p2m_is_logdirty_range(struct p2m_domain *, unsigned long start,
>                            unsigned long end);
>  
> -/* Set foreign entry in the p2m table (for priv-mapping) */
> -int set_foreign_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn);
> -
>  /* Set mmio addresses in the p2m table (for pass-through) */
>  int set_mmio_p2m_entry(struct domain *d, gfn_t gfn, mfn_t mfn,
>                         unsigned int order);
> diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
> index 58031a6..b4bc709 100644
> --- a/xen/include/xen/p2m-common.h
> +++ b/xen/include/xen/p2m-common.h
> @@ -3,6 +3,10 @@
>  
>  #include <xen/mm.h>
>  
> +/* Set foreign entry in the p2m table */
> +int set_foreign_p2m_entry(struct domain *d, const struct domain *fd,
> +                          unsigned long gfn, mfn_t mfn);
> +
>  /* Remove a page from a domain's p2m table */
>  int __must_check
>  guest_physmap_remove_page(struct domain *d, gfn_t gfn, mfn_t mfn,
> -- 
> 2.7.4
>

Stefano Stabellini Jan. 15, 2021, 1:18 a.m. UTC | #4

On Mon, 30 Nov 2020, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <[e-mail]>
> 
> 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 <[e-mail]>
> CC: Julien Grall <[e-mail]>

Acked-by: Stefano Stabellini <[e-mail]>


> ---
> Please note, this is a split/cleanup/hardening of Julien's PoC:
> "Add support for Guest IO forwarding to a device emulator"
> 
> Changes RFC -> V1:
>    - new patch, was split from:
>      "[RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features"
>    - rewrite a logic to handle properly reference in set_foreign_p2m_entry()
>      instead of treating foreign entries as p2m_ram_rw
> 
> Changes V1 -> V2:
>    - rebase according to the recent changes to acquire_resource()
>    - update patch description
>    - introduce arch_refcounts_p2m()
>    - add an explanation why p2m_map_foreign_rw is valid
>    - move set_foreign_p2m_entry() to p2m-common.h
>    - add const to new parameter
> 
> Changes V2 -> V3:
>    - update patch description
>    - rename arch_refcounts_p2m() to arch_acquire_resource_check()
>    - move comment to x86’s arch_acquire_resource_check()
>    - return rc in Arm's set_foreign_p2m_entry()
>    - put a respective ASSERT() into Arm's set_foreign_p2m_entry()
> ---
> ---
>  xen/arch/arm/p2m.c           | 24 ++++++++++++++++++++++++
>  xen/arch/x86/mm/p2m.c        |  5 +++--
>  xen/common/memory.c          | 10 +++-------
>  xen/include/asm-arm/p2m.h    | 19 +++++++++----------
>  xen/include/asm-x86/p2m.h    | 16 +++++++++++++---
>  xen/include/xen/p2m-common.h |  4 ++++
>  6 files changed, 56 insertions(+), 22 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 4eeb867..5b8d494 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1380,6 +1380,30 @@ int guest_physmap_remove_page(struct domain *d, gfn_t gfn, mfn_t mfn,
>      return p2m_remove_mapping(d, gfn, (1 << page_order), mfn);
>  }
>  
> +int set_foreign_p2m_entry(struct domain *d, const struct domain *fd,
> +                          unsigned long gfn, mfn_t mfn)
> +{
> +    struct page_info *page = mfn_to_page(mfn);
> +    int rc;
> +
> +    if ( !get_page(page, fd) )
> +        return -EINVAL;
> +
> +    /*
> +     * It is valid to always use p2m_map_foreign_rw here as if this gets
> +     * called then d != fd. A case when d == fd would be rejected by
> +     * rcu_lock_remote_domain_by_id() earlier. Put a respective ASSERT()
> +     * to catch incorrect usage in future.
> +     */
> +    ASSERT(d != fd);
> +
> +    rc = guest_physmap_add_entry(d, _gfn(gfn), mfn, 0, p2m_map_foreign_rw);
> +    if ( rc )
> +        put_page(page);
> +
> +    return rc;
> +}
> +
>  static struct page_info *p2m_allocate_root(void)
>  {
>      struct page_info *page;
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 7a2ba82..4772c86 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1321,7 +1321,8 @@ static int set_typed_p2m_entry(struct domain *d, unsigned long gfn_l,
>  }
>  
>  /* Set foreign mfn in the given guest's p2m table. */
> -int set_foreign_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
> +int set_foreign_p2m_entry(struct domain *d, const struct domain *fd,
> +                          unsigned long gfn, mfn_t mfn)
>  {
>      return set_typed_p2m_entry(d, gfn, mfn, PAGE_ORDER_4K, p2m_map_foreign,
>                                 p2m_get_hostp2m(d)->default_access);
> @@ -2621,7 +2622,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
>       * will update the m2p table which will result in  mfn -> gpfn of dom0
>       * and not fgfn of domU.
>       */
> -    rc = set_foreign_p2m_entry(tdom, gpfn, mfn);
> +    rc = set_foreign_p2m_entry(tdom, fdom, gpfn, mfn);
>      if ( rc )
>          gdprintk(XENLOG_WARNING, "set_foreign_p2m_entry failed. "
>                   "gpfn:%lx mfn:%lx fgfn:%lx td:%d fd:%d\n",
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 3363c06..49e3001 100644
> --- 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;
>  
>      if ( copy_from_guest(&xmar, arg, 1) )
> @@ -1207,7 +1203,7 @@ static int acquire_resource(
>  
>          for ( i = 0; !rc && i < xmar.nr_frames; i++ )
>          {
> -            rc = set_foreign_p2m_entry(currd, gfn_list[i],
> +            rc = set_foreign_p2m_entry(currd, d, gfn_list[i],
>                                         _mfn(mfn_list[i]));
>              /* rc should be -EIO for any iteration other than the first */
>              if ( rc && i )
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 28ca9a8..4f8056e 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -161,6 +161,15 @@ typedef enum {
>  #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 supported on Arm.
> +     */
> +    return true;
> +}
> +
>  static inline
>  void p2m_altp2m_check(struct vcpu *v, uint16_t idx)
>  {
> @@ -392,16 +401,6 @@ static inline gfn_t gfn_next_boundary(gfn_t gfn, unsigned int order)
>      return gfn_add(gfn, 1UL << order);
>  }
>  
> -static inline int set_foreign_p2m_entry(struct domain *d, unsigned long gfn,
> -                                        mfn_t mfn)
> -{
> -    /*
> -     * NOTE: If this is implemented then proper reference counting of
> -     *       foreign entries will need to be implemented.
> -     */
> -    return -EOPNOTSUPP;
> -}
> -
>  /*
>   * A vCPU has cache enabled only when the MMU is enabled and data cache
>   * is enabled.
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index 4603560..8d2dc22 100644
> --- 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;
> +}
> +
>  /*
>   * Updates vCPU's n2pm to match its np2m_base in VMCx12 and returns that np2m.
>   */
> @@ -647,9 +660,6 @@ int p2m_finish_type_change(struct domain *d,
>  int p2m_is_logdirty_range(struct p2m_domain *, unsigned long start,
>                            unsigned long end);
>  
> -/* Set foreign entry in the p2m table (for priv-mapping) */
> -int set_foreign_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn);
> -
>  /* Set mmio addresses in the p2m table (for pass-through) */
>  int set_mmio_p2m_entry(struct domain *d, gfn_t gfn, mfn_t mfn,
>                         unsigned int order);
> diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
> index 58031a6..b4bc709 100644
> --- a/xen/include/xen/p2m-common.h
> +++ b/xen/include/xen/p2m-common.h
> @@ -3,6 +3,10 @@
>  
>  #include <xen/mm.h>
>  
> +/* Set foreign entry in the p2m table */
> +int set_foreign_p2m_entry(struct domain *d, const struct domain *fd,
> +                          unsigned long gfn, mfn_t mfn);
> +
>  /* Remove a page from a domain's p2m table */
>  int __must_check
>  guest_physmap_remove_page(struct domain *d, gfn_t gfn, mfn_t mfn,
> -- 
> 2.7.4
>

Patch

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 4eeb867..5b8d494 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1380,6 +1380,30 @@  int guest_physmap_remove_page(struct domain *d, gfn_t gfn, mfn_t mfn,
     return p2m_remove_mapping(d, gfn, (1 << page_order), mfn);
 }
 
+int set_foreign_p2m_entry(struct domain *d, const struct domain *fd,
+                          unsigned long gfn, mfn_t mfn)
+{
+    struct page_info *page = mfn_to_page(mfn);
+    int rc;
+
+    if ( !get_page(page, fd) )
+        return -EINVAL;
+
+    /*
+     * It is valid to always use p2m_map_foreign_rw here as if this gets
+     * called then d != fd. A case when d == fd would be rejected by
+     * rcu_lock_remote_domain_by_id() earlier. Put a respective ASSERT()
+     * to catch incorrect usage in future.
+     */
+    ASSERT(d != fd);
+
+    rc = guest_physmap_add_entry(d, _gfn(gfn), mfn, 0, p2m_map_foreign_rw);
+    if ( rc )
+        put_page(page);
+
+    return rc;
+}
+
 static struct page_info *p2m_allocate_root(void)
 {
     struct page_info *page;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 7a2ba82..4772c86 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1321,7 +1321,8 @@  static int set_typed_p2m_entry(struct domain *d, unsigned long gfn_l,
 }
 
 /* Set foreign mfn in the given guest's p2m table. */
-int set_foreign_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
+int set_foreign_p2m_entry(struct domain *d, const struct domain *fd,
+                          unsigned long gfn, mfn_t mfn)
 {
     return set_typed_p2m_entry(d, gfn, mfn, PAGE_ORDER_4K, p2m_map_foreign,
                                p2m_get_hostp2m(d)->default_access);
@@ -2621,7 +2622,7 @@  int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
      * will update the m2p table which will result in  mfn -> gpfn of dom0
      * and not fgfn of domU.
      */
-    rc = set_foreign_p2m_entry(tdom, gpfn, mfn);
+    rc = set_foreign_p2m_entry(tdom, fdom, gpfn, mfn);
     if ( rc )
         gdprintk(XENLOG_WARNING, "set_foreign_p2m_entry failed. "
                  "gpfn:%lx mfn:%lx fgfn:%lx td:%d fd:%d\n",
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 3363c06..49e3001 100644
--- 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;
 
     if ( copy_from_guest(&xmar, arg, 1) )
@@ -1207,7 +1203,7 @@  static int acquire_resource(
 
         for ( i = 0; !rc && i < xmar.nr_frames; i++ )
         {
-            rc = set_foreign_p2m_entry(currd, gfn_list[i],
+            rc = set_foreign_p2m_entry(currd, d, gfn_list[i],
                                        _mfn(mfn_list[i]));
             /* rc should be -EIO for any iteration other than the first */
             if ( rc && i )
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 28ca9a8..4f8056e 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -161,6 +161,15 @@  typedef enum {
 #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 supported on Arm.
+     */
+    return true;
+}
+
 static inline
 void p2m_altp2m_check(struct vcpu *v, uint16_t idx)
 {
@@ -392,16 +401,6 @@  static inline gfn_t gfn_next_boundary(gfn_t gfn, unsigned int order)
     return gfn_add(gfn, 1UL << order);
 }
 
-static inline int set_foreign_p2m_entry(struct domain *d, unsigned long gfn,
-                                        mfn_t mfn)
-{
-    /*
-     * NOTE: If this is implemented then proper reference counting of
-     *       foreign entries will need to be implemented.
-     */
-    return -EOPNOTSUPP;
-}
-
 /*
  * A vCPU has cache enabled only when the MMU is enabled and data cache
  * is enabled.
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 4603560..8d2dc22 100644
--- 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;
+}
+
 /*
  * Updates vCPU's n2pm to match its np2m_base in VMCx12 and returns that np2m.
  */
@@ -647,9 +660,6 @@  int p2m_finish_type_change(struct domain *d,
 int p2m_is_logdirty_range(struct p2m_domain *, unsigned long start,
                           unsigned long end);
 
-/* Set foreign entry in the p2m table (for priv-mapping) */
-int set_foreign_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn);
-
 /* Set mmio addresses in the p2m table (for pass-through) */
 int set_mmio_p2m_entry(struct domain *d, gfn_t gfn, mfn_t mfn,
                        unsigned int order);
diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
index 58031a6..b4bc709 100644
--- a/xen/include/xen/p2m-common.h
+++ b/xen/include/xen/p2m-common.h
@@ -3,6 +3,10 @@ 
 
 #include <xen/mm.h>
 
+/* Set foreign entry in the p2m table */
+int set_foreign_p2m_entry(struct domain *d, const struct domain *fd,
+                          unsigned long gfn, mfn_t mfn);
+
 /* Remove a page from a domain's p2m table */
 int __must_check
 guest_physmap_remove_page(struct domain *d, gfn_t gfn, mfn_t mfn,