Re: [PATCH 3/3] x86: don't override INVALID_M2P_ENTRY with SHARED_M2P_ENTRY

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 06/08/2020 10:29, Jan Beulich wrote:
> While in most cases code ahead of the invocation of set_gpfn_from_mfn()
> deals with shared pages, at least in set_typed_p2m_entry() I can't spot
> such handling (it's entirely possible there's code missing there). Let's
> try to play safe and add an extra check.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -525,9 +525,14 @@ extern const unsigned int *const compat_
>  #endif /* CONFIG_PV32 */
>  
>  #define _set_gpfn_from_mfn(mfn, pfn) ({                        \
> -    struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn))); \
> -    unsigned long entry = (d && (d == dom_cow)) ?              \
> -        SHARED_M2P_ENTRY : (pfn);                              \
> +    unsigned long entry = (pfn);                               \
> +    if ( entry != INVALID_M2P_ENTRY )                          \
> +    {                                                          \
> +        const struct domain *d;                                \
> +        d = page_get_owner(mfn_to_page(_mfn(mfn)));            \
> +        if ( d && (d == dom_cow) )                             \
> +            entry = SHARED_M2P_ENTRY;                          \
> +    }                                                          \
>      set_compat_m2p(mfn, (unsigned int)(entry));                \
>      machine_to_phys_mapping[mfn] = (entry);                    \
>  })
>

Hmm - we already have a lot of callers, and this is already too
complicated to be a define.

We have x86 which uses M2P, and ARM which doesn't.  We have two more
architectures on the way which probably won't want M2P, and certainly
won't in the beginning.

Can we introduce CONFIG_M2P which is selected by x86, rename this
infrastructure to set_m2p() or something, provide a no-op fallback in
common code, and move this implementation into x86/mm.c ?

In particular, silently clobbering pfn to SHARED_M2P_ENTRY is rude
behaviour.  It would be better to ASSERT() the right one is passed in,
which also simplifies release builds.

~Andrew