Re: [PATCH for-4.19 v2 3/3] xen/x86: remove foreign mappings from the p2m on teardown

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, May 08, 2024 at 01:23:23PM +0200, Roger Pau Monne wrote:
> Iterate over the p2m up to the maximum recorded gfn and remove any foreign
> mappings, in order to drop the underlying page references and thus don't keep
> extra page references if a domain is destroyed while still having foreign
> mappings on it's p2m.
> 
> The logic is similar to the one used on Arm.
> 
> Note that foreign mappings cannot be created by guests that have altp2m or
> nested HVM enabled, as p2ms different than the host one are not currently
> scrubbed when destroyed in order to drop references to any foreign maps.
> 
> It's unclear whether the right solution is to take an extra reference when
> foreign maps are added to p2ms different than the host one, or just rely on 
> the
> host p2m already having a reference.  The mapping being removed from the host
> p2m should cause it to be dropped on all domain p2ms.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Changes since v1:
>  - Use existing p2m max_mapped_pfn field.
>  - Prevent creating foreign mappings by guests that have altp2m or nestedhvm
>    enabled.
> ---
>  CHANGELOG.md                   |  1 +
>  xen/arch/x86/domain.c          |  8 +++-
>  xen/arch/x86/include/asm/p2m.h | 26 +++++++------
>  xen/arch/x86/mm/p2m-basic.c    | 17 +++++++++
>  xen/arch/x86/mm/p2m.c          | 68 ++++++++++++++++++++++++++++++++--
>  5 files changed, 103 insertions(+), 17 deletions(-)
> 
> diff --git a/CHANGELOG.md b/CHANGELOG.md
> index 8041cfb7d243..09bdb9b97578 100644
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -14,6 +14,7 @@ The format is based on [Keep a 
> Changelog](https://keepachangelog.com/en/1.0.0/)
>     - HVM PIRQs are disabled by default.
>     - Reduce IOMMU setup time for hardware domain.
>   - xl/libxl configures vkb=[] for HVM domains with priority over vkb_device.
> + - Allow HVM/PVH domains to map foreign pages.
>  
>  ### Added
>   - On x86:
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index dff790060605..1cb3ccddab00 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -718,7 +718,7 @@ int arch_sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>          return -EINVAL;
>      }
>  
> -    if ( altp2m && (altp2m & (altp2m - 1)) )
> +    if ( altp2m & (altp2m - 1) )
>      {
>          dprintk(XENLOG_INFO, "Multiple altp2m options selected in flags: 
> %#x\n",
>                  config->flags);
> @@ -2387,6 +2387,7 @@ int domain_relinquish_resources(struct domain *d)
>          enum {
>              PROG_iommu_pagetables = 1,
>              PROG_shared,
> +            PROG_mappings,
>              PROG_paging,
>              PROG_vcpu_pagetables,
>              PROG_xen,
> @@ -2435,6 +2436,11 @@ int domain_relinquish_resources(struct domain *d)
>          }
>  #endif
>  
> +    PROGRESS(mappings):
> +        ret = relinquish_p2m_mapping(d);
> +        if ( ret )
> +            return ret;
> +
>      PROGRESS(paging):
>  
>          /* Tear down paging-assistance stuff. */
> diff --git a/xen/arch/x86/include/asm/p2m.h b/xen/arch/x86/include/asm/p2m.h
> index 107b9f260848..c1478ffc3647 100644
> --- a/xen/arch/x86/include/asm/p2m.h
> +++ b/xen/arch/x86/include/asm/p2m.h
> @@ -383,6 +383,8 @@ struct p2m_domain {
>  
>      /* Number of foreign mappings. */
>      unsigned long      nr_foreign;
> +    /* Cursor for iterating over the p2m on teardown. */
> +    unsigned long      teardown_gfn;
>  #endif /* CONFIG_HVM */
>  };
>  
> @@ -395,16 +397,7 @@ struct p2m_domain {
>  #endif
>  #include <xen/p2m-common.h>
>  
> -static inline bool arch_acquire_resource_check(struct domain *d)
> -{
> -    /*
> -     * 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
> -     * (see set_foreign_p2m_entry()).
> -     */
> -    return !paging_mode_translate(d) || is_hardware_domain(d);

This must be:

    return is_pv_domain(d) ||
           (d->arch.hvm.params[HVM_PARAM_ALTP2M] == XEN_ALTP2M_disabled &&
            !nestedhvm_enabled(d));

Sorry.