| Message ID | 20190507151458.29350-12-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Properly disable M2P on Arm. | expand |
>>> 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
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,
>>> 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
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,
>>> 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
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,
>>> 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
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,
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;
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(-)