[3/3] x86: don't override INVALID_M2P_ENTRY with SHARED_M2P_ENTRY
diff mbox series

Message ID 1d83fd35-6ea5-289c-d8db-029c50957f85@suse.com
State Superseded
Headers show
Series
  • x86: M2P maintenance adjustments (step 1)
Related show

Commit Message

Jan Beulich Aug. 6, 2020, 9:29 a.m. UTC
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@suse.com>

Comments

Andrew Cooper Aug. 10, 2020, 4:42 p.m. UTC | #1
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@suse.com>
>
> --- 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
Jan Beulich Aug. 17, 2020, 4:11 p.m. UTC | #2
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@suse.com>
>>
>> --- 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

Patch
diff mbox series

--- 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);                    \
 })