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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.08.2020 18:42, Andrew Cooper wrote:
> 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.

I did consider moving this into an out-of-line function, yes.

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

We can, sure. Question is whether this isn't more scope creep than
is acceptable considering the purpose of this change.

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

Now this is, irrespective of me agreeing with the point you make,
a change I'm not going to make: There's no way I could guarantee
I wouldn't break mem-sharing. A change like this can imo only
possibly be done by someone actively working on and with
mem-sharing.

Jan