[Xen-devel] [PATCH 11/14] xen/x86: mm: Re-implement set_gpfn_from_mfn() as a static inline function

Julien Grall posted 14 patches 10 weeks ago

[Xen-devel] [PATCH 11/14] xen/x86: mm: Re-implement set_gpfn_from_mfn() as a static inline function

Posted by Julien Grall 10 weeks ago
set_gpfn_from_mfn() is currently implement in a 2 part macros. The
second macro is only called within the first macro, so they can be
folded together.

Furthermore, this is now converted to a static inline making the code
more readable and safer.

As set_gpfn_from_mfn is now a static inline function, the extern
variable dom_cow should be defined earlier on. For convenience, the
definition of all dom_* variables are moved earlier on.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---
    Changes in v2:
        - Patch added
---
 xen/include/asm-x86/mm.h | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 717378730b..4721725c60 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -442,6 +442,8 @@ int check_descriptor(const struct domain *d, seg_desc_t *desc);
 
 extern paddr_t mem_hotplug;
 
+extern struct domain *dom_xen, *dom_io, *dom_cow;	/* for vmcoreinfo */
+
 /******************************************************************************
  * With shadow pagetables, the different kinds of address start
  * to get get confusing.
@@ -483,24 +485,25 @@ extern paddr_t mem_hotplug;
 #define SHARED_M2P(_e)           ((_e) == SHARED_M2P_ENTRY)
 
 #define compat_machine_to_phys_mapping ((unsigned int *)RDWR_COMPAT_MPT_VIRT_START)
-#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);                              \
-    ((void)((mfn) >= (RDWR_COMPAT_MPT_VIRT_END - RDWR_COMPAT_MPT_VIRT_START) / 4 || \
-            (compat_machine_to_phys_mapping[(mfn)] = (unsigned int)(entry))), \
-     machine_to_phys_mapping[(mfn)] = (entry));                \
-    })
 
 /*
  * Disable some users of set_gpfn_from_mfn() (e.g., free_heap_pages()) until
  * the machine_to_phys_mapping is actually set up.
  */
 extern bool machine_to_phys_mapping_valid;
-#define set_gpfn_from_mfn(mfn, pfn) do {        \
-    if ( machine_to_phys_mapping_valid )        \
-        _set_gpfn_from_mfn(mfn, pfn);           \
-} while (0)
+
+static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn)
+{
+    struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn)));
+    unsigned long entry = (d && (d == dom_cow)) ? SHARED_M2P_ENTRY : pfn;
+
+    if (!machine_to_phys_mapping_valid)
+        return;
+
+    if ( mfn >= (RDWR_COMPAT_MPT_VIRT_END - RDWR_COMPAT_MPT_VIRT_START) / 4 )
+        compat_machine_to_phys_mapping[mfn] = entry;
+    machine_to_phys_mapping[mfn] = entry;
+}
 
 extern struct rangeset *mmio_ro_ranges;
 
@@ -592,8 +595,6 @@ unsigned int domain_clamp_alloc_bitsize(struct domain *d, unsigned int bits);
 
 unsigned long domain_get_maximum_gpfn(struct domain *d);
 
-extern struct domain *dom_xen, *dom_io, *dom_cow;	/* for vmcoreinfo */
-
 /* Definition of an mm lock: spinlock with extra fields for debugging */
 typedef struct mm_lock {
     spinlock_t         lock;
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 11/14] xen/x86: mm: Re-implement set_gpfn_from_mfn() as a static inline function

Posted by Jan Beulich 10 weeks ago
>>> On 07.05.19 at 17:14, <julien.grall@arm.com> wrote:
> +static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn)
> +{
> +    struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn)));
> +    unsigned long entry = (d && (d == dom_cow)) ? SHARED_M2P_ENTRY : pfn;

The && here looks, ehm, funny, but I guess it's needed for early boot?
But that's perhaps a separate thing to clean up. However, looking at
this - why is Arm setting up dom_cow in the first place?

> +    if (!machine_to_phys_mapping_valid)

Please add the missing blanks.

> +        return;
> +
> +    if ( mfn >= (RDWR_COMPAT_MPT_VIRT_END - RDWR_COMPAT_MPT_VIRT_START) / 4 )

You've inverted the original condition (by re-using it verbatim) - I'm pretty
sure this is going to crash.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 11/14] xen/x86: mm: Re-implement set_gpfn_from_mfn() as a static inline function

Posted by Julien Grall 10 weeks ago
Hi,

On 10/05/2019 13:43, Jan Beulich wrote:
>>>> On 07.05.19 at 17:14, <julien.grall@arm.com> wrote:
>> +static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn)
>> +{
>> +    struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn)));
>> +    unsigned long entry = (d && (d == dom_cow)) ? SHARED_M2P_ENTRY : pfn;
> 
> The && here looks, ehm, funny, but I guess it's needed for early boot?

I have no idea, this is x86 not Arm...

> But that's perhaps a separate thing to clean up. However, looking at
> this - why is Arm setting up dom_cow in the first place?

Common code is using dom_cow, so I don't think we want it to be NULL on Arm to 
avoid weird issues.

> 
>> +    if (!machine_to_phys_mapping_valid)
> 
> Please add the missing blanks.

Ok.

> 
>> +        return;
>> +
>> +    if ( mfn >= (RDWR_COMPAT_MPT_VIRT_END - RDWR_COMPAT_MPT_VIRT_START) / 4 )
> 
> You've inverted the original condition (by re-using it verbatim) - I'm pretty
> sure this is going to crash.

Good point, I will update the patch.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 11/14] xen/x86: mm: Re-implement set_gpfn_from_mfn() as a static inline function

Posted by Jan Beulich 10 weeks ago
>>> On 10.05.19 at 15:27, <julien.grall@arm.com> wrote:
> On 10/05/2019 13:43, Jan Beulich wrote:
>>>>> On 07.05.19 at 17:14, <julien.grall@arm.com> wrote:
>>> +static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn)
>>> +{
>>> +    struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn)));
>>> +    unsigned long entry = (d && (d == dom_cow)) ? SHARED_M2P_ENTRY : pfn;
>> 
>> The && here looks, ehm, funny, but I guess it's needed for early boot?
> 
> I have no idea, this is x86 not Arm...
> 
>> But that's perhaps a separate thing to clean up. However, looking at
>> this - why is Arm setting up dom_cow in the first place?
> 
> Common code is using dom_cow, so I don't think we want it to be NULL on Arm to 
> avoid weird issues.

I didn't mean it to remain NULL. Common code doesn't dereference it
(and isn't supposed to), so I'd consider initializing it to some known
faulting non-NULL address, if there is such on Arm.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 11/14] xen/x86: mm: Re-implement set_gpfn_from_mfn() as a static inline function

Posted by Julien Grall 10 weeks ago
On 10/05/2019 14:35, Jan Beulich wrote:
>>>> On 10.05.19 at 15:27, <julien.grall@arm.com> wrote:
>> On 10/05/2019 13:43, Jan Beulich wrote:
>>>>>> On 07.05.19 at 17:14, <julien.grall@arm.com> wrote:
>>>> +static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn)
>>>> +{
>>>> +    struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn)));
>>>> +    unsigned long entry = (d && (d == dom_cow)) ? SHARED_M2P_ENTRY : pfn;
>>>
>>> The && here looks, ehm, funny, but I guess it's needed for early boot?
>>
>> I have no idea, this is x86 not Arm...
>>
>>> But that's perhaps a separate thing to clean up. However, looking at
>>> this - why is Arm setting up dom_cow in the first place?
>>
>> Common code is using dom_cow, so I don't think we want it to be NULL on Arm to
>> avoid weird issues.
> 
> I didn't mean it to remain NULL. Common code doesn't dereference it
> (and isn't supposed to), so I'd consider initializing it to some known
> faulting non-NULL address, if there is such on Arm.

Patches are welcomed ;).

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 11/14] xen/x86: mm: Re-implement set_gpfn_from_mfn() as a static inline function

Posted by Jan Beulich 10 weeks ago
>>> On 10.05.19 at 15:41, <julien.grall@arm.com> wrote:
> On 10/05/2019 14:35, Jan Beulich wrote:
>>>>> On 10.05.19 at 15:27, <julien.grall@arm.com> wrote:
>>> On 10/05/2019 13:43, Jan Beulich wrote:
>>>>>>> On 07.05.19 at 17:14, <julien.grall@arm.com> wrote:
>>>>> +static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn)
>>>>> +{
>>>>> +    struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn)));
>>>>> +    unsigned long entry = (d && (d == dom_cow)) ? SHARED_M2P_ENTRY : pfn;
>>>>
>>>> The && here looks, ehm, funny, but I guess it's needed for early boot?
>>>
>>> I have no idea, this is x86 not Arm...
>>>
>>>> But that's perhaps a separate thing to clean up. However, looking at
>>>> this - why is Arm setting up dom_cow in the first place?
>>>
>>> Common code is using dom_cow, so I don't think we want it to be NULL on Arm to
>>> avoid weird issues.
>> 
>> I didn't mean it to remain NULL. Common code doesn't dereference it
>> (and isn't supposed to), so I'd consider initializing it to some known
>> faulting non-NULL address, if there is such on Arm.
> 
> Patches are welcomed ;).

So is there such an address on Arm?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 11/14] xen/x86: mm: Re-implement set_gpfn_from_mfn() as a static inline function

Posted by Julien Grall 10 weeks ago
Hi,

On 10/05/2019 14:48, Jan Beulich wrote:
>>>> On 10.05.19 at 15:41, <julien.grall@arm.com> wrote:
>> On 10/05/2019 14:35, Jan Beulich wrote:
>>>>>> On 10.05.19 at 15:27, <julien.grall@arm.com> wrote:
>>>> On 10/05/2019 13:43, Jan Beulich wrote:
>>>>>>>> On 07.05.19 at 17:14, <julien.grall@arm.com> wrote:
>>>>>> +static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn)
>>>>>> +{
>>>>>> +    struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn)));
>>>>>> +    unsigned long entry = (d && (d == dom_cow)) ? SHARED_M2P_ENTRY : pfn;
>>>>>
>>>>> The && here looks, ehm, funny, but I guess it's needed for early boot?
>>>>
>>>> I have no idea, this is x86 not Arm...
>>>>
>>>>> But that's perhaps a separate thing to clean up. However, looking at
>>>>> this - why is Arm setting up dom_cow in the first place?
>>>>
>>>> Common code is using dom_cow, so I don't think we want it to be NULL on Arm to
>>>> avoid weird issues.
>>>
>>> I didn't mean it to remain NULL. Common code doesn't dereference it
>>> (and isn't supposed to), so I'd consider initializing it to some known
>>> faulting non-NULL address, if there is such on Arm.
>>
>> Patches are welcomed ;).
> 
> So is there such an address on Arm?

0 - 2MB is unmapped so far. I don't know whether this will still be the case (at 
least for the range 4KB - 2MB) with the rework I am attempting.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 11/14] xen/x86: mm: Re-implement set_gpfn_from_mfn() as a static inline function

Posted by Jan Beulich 10 weeks ago
>>> On 10.05.19 at 16:05, <julien.grall@arm.com> wrote:
> On 10/05/2019 14:48, Jan Beulich wrote:
>>>>> On 10.05.19 at 15:41, <julien.grall@arm.com> wrote:
>>> On 10/05/2019 14:35, Jan Beulich wrote:
>>>> I didn't mean it to remain NULL. Common code doesn't dereference it
>>>> (and isn't supposed to), so I'd consider initializing it to some known
>>>> faulting non-NULL address, if there is such on Arm.
>>>
>>> Patches are welcomed ;).
>> 
>> So is there such an address on Arm?
> 
> 0 - 2MB is unmapped so far. I don't know whether this will still be the case (at 
> least for the range 4KB - 2MB) with the rework I am attempting.

Hmm, I was hoping for an architecturally faulting address, like
the non-canonical ones we have on x86-64.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 11/14] xen/x86: mm: Re-implement set_gpfn_from_mfn() as a static inline function

Posted by Julien Grall 10 weeks ago
Hi,

On 10/05/2019 15:21, Jan Beulich wrote:
>>>> On 10.05.19 at 16:05, <julien.grall@arm.com> wrote:
>> On 10/05/2019 14:48, Jan Beulich wrote:
>>>>>> On 10.05.19 at 15:41, <julien.grall@arm.com> wrote:
>>>> On 10/05/2019 14:35, Jan Beulich wrote:
>>>>> I didn't mean it to remain NULL. Common code doesn't dereference it
>>>>> (and isn't supposed to), so I'd consider initializing it to some known
>>>>> faulting non-NULL address, if there is such on Arm.
>>>>
>>>> Patches are welcomed ;).
>>>
>>> So is there such an address on Arm?
>>
>> 0 - 2MB is unmapped so far. I don't know whether this will still be the case (at
>> least for the range 4KB - 2MB) with the rework I am attempting.
> 
> Hmm, I was hoping for an architecturally faulting address, like
> the non-canonical ones we have on x86-64.

Nothing we can reliably use across Armv7 and Armv8 (and future extension).

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel