Re: [PATCH v4 2/4] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 21/09/2020 19:02, Julien Grall wrote:
> From: Julien Grall <julien.grall@xxxxxxx>
>
> While Arm never had a M2P, the implementation of mfn_to_gmfn is pretty
> bogus as we directly return the MFN passed in parameter.
>
> Thankfully, the use of mfn_to_gmfn is pretty limited on Arm today. There
> are only 2 callers in the common code:
>     - memory_exchange: Arm cannot not use it because steal_page is not
>     implemented. Note this is already protected by !CONFIG_PV.
>     - getdomaininfo: Toolstack may map the shared page. It looks like
>     this is mostly used for mapping the P2M of PV guest. Therefore the
>     issue might be minor.
>
> Implementing the M2P on Arm is not planned. The M2P would require significant
> amount of VA address (very tough on 32-bit) that can hardly be justified with
> the current use of mfn_to_gmfn.

If anyone cares, access to the shared info page should be wired up
through acquire_resource, not through this foreign mapping bodge,
because ...

>     - memory_exchange: This does not work and I haven't seen any
>     request for it so far.
>     - getdomaininfo: The structure on Arm does not seem to contain a lot
>     of useful information on Arm. It is unclear whether we want to
>     allow the toolstack mapping it on Arm.
>
> This patch introduces a config option HAS_M2P to tell whether an
> architecture implements the M2P.
>     - memory_exchange: This is PV only (not supported on Arm) but take
>     the opportunity to gate with HAS_M2P as well in case someone decide
>     to implement PV without M2P.
>     - getdomaininfo: A new helper is introduced to wrap the call to
>     mfn_to_gfn/mfn_to_gmfn. For Arm, a fixed value will be provided that will
>     fail on mapping if used.
>
> Signed-off-by Julien Grall <julien.grall@xxxxxxx>

... possibly the single best reason for returning -1 on ARM is that this
is already the behaviour for x86 HVM guests, until the guest has mapped
the shared info frame for the first time.

(XEN) *** d0v6 getdomaininfo(d1) d->shared_info ffff83007cffe000,
shared_info_frame 0x7cffe
(XEN) *** d0v6 getdomaininfo(d2) d->shared_info ffff83007a401000,
shared_info_frame 0xffffffffffffffff

(d1 is PV, d2 is HVM.  This is the result of creating domains at the
python shell, then querying them for their info, without any further
construction or execution).

Furthermore, both PV and HVM guests can invalidate the M2P mappings for
their shared_info page, which in the HVM case would cause -1 to be
returned again.

> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 5ac6e9c5cafa..2bf3e6659018 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -68,6 +68,7 @@ void getdomaininfo(struct domain *d, struct 
> xen_domctl_getdomaininfo *info)
>      u64 cpu_time = 0;
>      int flags = XEN_DOMINF_blocked;
>      struct vcpu_runstate_info runstate;
> +    gfn_t shared_info_frame;
>  
>      info->domain = d->domain_id;
>      info->max_vcpu_id = XEN_INVALID_MAX_VCPU_ID;
> @@ -111,8 +112,12 @@ void getdomaininfo(struct domain *d, struct 
> xen_domctl_getdomaininfo *info)
>      info->outstanding_pages = d->outstanding_pages;
>      info->shr_pages         = atomic_read(&d->shr_pages);
>      info->paged_pages       = atomic_read(&d->paged_pages);
> -    info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
> -    BUG_ON(SHARED_M2P(info->shared_info_frame));
> +
> +    shared_info_frame = domain_shared_info_gfn(d);
> +    if ( gfn_eq(shared_info_frame, INVALID_GFN) )
> +        info->shared_info_frame = XEN_INVALID_SHARED_INFO_FRAME;
> +    else
> +        info->shared_info_frame = gfn_x(shared_info_frame);

This can be simplified to just  info->shared_info_frame =
gfn_x(arch_shared_info_gfn(d)) based on the suggestion at the bottom.

>  
>      info->cpupool = cpupool_get_id(d);
>  
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 9300104943b0..c698e6872596 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -504,7 +504,7 @@ static bool propagate_node(unsigned int xmf, unsigned int 
> *memflags)
>  
>  static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) 
> arg)
>  {
> -#ifdef CONFIG_PV
> +#if defined(CONFIG_PV) && defined(CONFIG_M2P)

There is no such thing as PV && !M2P.  The M2P table is part of the PV
ABI with guests.

These two hunks should be dropped.

>      struct xen_memory_exchange exch;
>      PAGE_LIST_HEAD(in_chunk_list);
>      PAGE_LIST_HEAD(out_chunk_list);
> @@ -801,7 +801,7 @@ static long 
> memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>      if ( __copy_field_to_guest(arg, &exch, nr_exchanged) )
>          rc = -EFAULT;
>      return rc;
> -#else /* !CONFIG_PV */
> +#else /* !(CONFIG_PV && CONFIG_M2P) */
>      return -EOPNOTSUPP;
>  #endif
>  }
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 6819a3bf382f..90161dd079a1 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -262,6 +262,11 @@ static inline void arch_vcpu_block(struct vcpu *v) {}
>  
>  #define arch_vm_assist_valid_mask(d) (1UL << 
> VMASST_TYPE_runstate_update_flag)
>  
> +static inline gfn_t domain_shared_info_gfn(struct domain *d)
> +{
> +    return INVALID_GFN;
> +}

We don't want every every architecture to provide a stub.  Instead, ...
(bottom reply)

> +
>  #endif /* __ASM_DOMAIN_H__ */
>  
>  /*
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 5c5e55ebcb76..7564df5e8374 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -136,6 +136,12 @@ struct xen_domctl_getdomaininfo {
>      uint64_aligned_t outstanding_pages;
>      uint64_aligned_t shr_pages;
>      uint64_aligned_t paged_pages;
> +#define XEN_INVALID_SHARED_INFO_FRAME (~(uint64_t)0)

We've already got INVALID_GFN as a constant used in the interface.  Lets
not proliferate more.

> +    /*
> +     * GFN of shared_info struct. Some architectures (e.g Arm) may not
> +     * provide a mappable address in the field. In that case, the field
> +     * will be set to XEN_INVALID_SHARED_INFO_FRAME.
> +     */
>      uint64_aligned_t shared_info_frame; /* GMFN of shared_info struct */

Delete this comment, especially as it is using obsolete naming terminology.

>      uint64_aligned_t cpu_time;
>      uint32_t nr_online_vcpus;    /* Number of VCPUs currently online. */
> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> index cde0d9c7fe63..7281eb7b36c7 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -131,4 +131,16 @@ void vnuma_destroy(struct vnuma_info *vnuma);
>  static inline void vnuma_destroy(struct vnuma_info *vnuma) { ASSERT(!vnuma); 
> }
>  #endif
>  
> +#ifdef CONFIG_HAS_M2P
> +#define domain_shared_info_gfn(d) ({                            \
> +    const struct domain *d_ = (d);                              \
> +    gfn_t gfn_;                                                 \
> +                                                                \
> +    gfn_ = mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info)));\
> +    BUG_ON(SHARED_M2P(gfn_x(gfn_)));                            \
> +                                                                \
> +    gfn_;                                                       \
> +})

... this wants to be

#ifndef arch_shared_info_gfn
static inline gfn_t arch_shared_info_gfn(const struct domain *d) {
return INVALID_GFN; }
#endif

with

gfn_t arch_shared_info_gfn(const struct domain *d);
#define arch_shared_info_gfn arch_shared_info_gfn

in asm-x86/domain.h

and the actual implementation in arch/x86/domain.c

~Andrew