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>>> 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
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
>>> 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
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
>>> 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
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
>>> 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
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
© 2016 - 2026 Red Hat, Inc.