| Message ID | 20200322161418.31606-17-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bunch of typesafe conversion | expand |
On Sun, 2020-03-22 at 16:14 +0000, julien@xen.org wrote: > From: Julien Grall <julien.grall@arm.com> > > The first parameter of {s,g}et_gpfn_from_mfn() is an MFN, so it can > be > switched to use the typesafe. > > At the same time, replace gpfn with pfn in the helpers as they all > deal > with PFN and also turn the macros to static inline. > > Note that the return of the getter and the 2nd parameter of the > setter > have not been converted to use typesafe PFN because it was requiring > more changes than expected. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > --- > This was originally sent as part of "xen/arm: Properly disable > M2P > on Arm" [1]. > > Changes since the original version: > - mfn_to_gmfn() is still present for now so update it > - Remove stray + > - Avoid churn in set_pfn_from_mfn() by inverting mfn and mfn_ > - Remove tags > - Fix build in mem_sharing > > [1] <20190603160350.29806-1-julien.grall@arm.com> > --- > xen/arch/x86/cpu/mcheck/mcaction.c | 2 +- > xen/arch/x86/mm.c | 14 +++---- > xen/arch/x86/mm/mem_sharing.c | 20 ++++----- > xen/arch/x86/mm/p2m-pod.c | 4 +- > xen/arch/x86/mm/p2m-pt.c | 35 ++++++++-------- > xen/arch/x86/mm/p2m.c | 66 +++++++++++++++------------- > -- > xen/arch/x86/mm/paging.c | 4 +- > xen/arch/x86/pv/dom0_build.c | 6 +-- > xen/arch/x86/x86_64/traps.c | 8 ++-- > xen/common/page_alloc.c | 2 +- > xen/include/asm-arm/mm.h | 2 +- > xen/include/asm-x86/grant_table.h | 2 +- > xen/include/asm-x86/mm.h | 12 ++++-- > xen/include/asm-x86/p2m.h | 2 +- > 14 files changed, 93 insertions(+), 86 deletions(-) > > [...] > diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h > index abf4cc23e4..11614f9107 100644 > --- a/xen/include/asm-arm/mm.h > +++ b/xen/include/asm-arm/mm.h > @@ -319,7 +319,7 @@ struct page_info *get_page_from_gva(struct vcpu > *v, vaddr_t va, > #define SHARED_M2P(_e) ((_e) == SHARED_M2P_ENTRY) > > /* Xen always owns P2M on ARM */ > -#define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); > } while (0) > +static inline void set_pfn_from_mfn(mfn_t mfn, unsigned long pfn) {} > #define mfn_to_gmfn(_d, mfn) (mfn) I do not have a setup to compile and test code for Arm, but wouldn't the compiler complain about unused arguments here? The marco version explicitly silenced compiler complaints. > diff --git a/xen/include/asm-x86/grant_table.h b/xen/include/asm- > x86/grant_table.h > index 5871238f6d..b6a09c4c6c 100644 > --- a/xen/include/asm-x86/grant_table.h > +++ b/xen/include/asm-x86/grant_table.h > @@ -41,7 +41,7 @@ static inline int > replace_grant_host_mapping(uint64_t addr, mfn_t frame, > #define gnttab_get_frame_gfn(gt, st, idx) > ({ \ > mfn_t mfn_ = (st) ? gnttab_status_mfn(gt, > idx) \ > : gnttab_shared_mfn(gt, > idx); \ > - unsigned long gpfn_ = > get_gpfn_from_mfn(mfn_x(mfn_)); \ > + unsigned long gpfn_ = > get_pfn_from_mfn(mfn_); \ > VALID_M2P(gpfn_) ? _gfn(gpfn_) : > INVALID_GFN; \ > }) > > diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h > index 53f2ed7c7d..2a4f42e78f 100644 > --- a/xen/include/asm-x86/mm.h > +++ b/xen/include/asm-x86/mm.h > @@ -500,9 +500,10 @@ extern paddr_t mem_hotplug; > */ > extern bool machine_to_phys_mapping_valid; > > -static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned > long pfn) > +static inline void set_pfn_from_mfn(mfn_t mfn_, unsigned long pfn) > { > - const struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn))); > + const unsigned long mfn = mfn_x(mfn_); > + const struct domain *d = page_get_owner(mfn_to_page(mfn_)); > unsigned long entry = (d && (d == dom_cow)) ? SHARED_M2P_ENTRY : > pfn; > > if ( !machine_to_phys_mapping_valid ) > @@ -515,11 +516,14 @@ static inline void set_gpfn_from_mfn(unsigned > long mfn, unsigned long pfn) > > extern struct rangeset *mmio_ro_ranges; > > -#define get_gpfn_from_mfn(mfn) (machine_to_phys_mapping[(mfn)]) > +static inline unsigned long get_pfn_from_mfn(mfn_t mfn) > +{ > + return machine_to_phys_mapping[mfn_x(mfn)]; > +} Any specific reason this (and some other macros) are turned into static inline? I don't have a problem with them being inline functions but just wondering if there is a reason to do so. > #define mfn_to_gmfn(_d, mfn) \ > ( (paging_mode_translate(_d)) \ > - ? get_gpfn_from_mfn(mfn) \ > + ? get_pfn_from_mfn(_mfn(mfn)) \ > : (mfn) ) > > #define compat_pfn_to_cr3(pfn) (((unsigned)(pfn) << 12) | > ((unsigned)(pfn) >> 20)) > diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h > index a2c6049834..39dae242b0 100644 > --- a/xen/include/asm-x86/p2m.h > +++ b/xen/include/asm-x86/p2m.h > @@ -505,7 +505,7 @@ static inline struct page_info > *get_page_from_gfn( > static inline gfn_t mfn_to_gfn(const struct domain *d, mfn_t mfn) > { > if ( paging_mode_translate(d) ) > - return _gfn(get_gpfn_from_mfn(mfn_x(mfn))); > + return _gfn(get_pfn_from_mfn(mfn)); > else > return _gfn(mfn_x(mfn)); > } Apart from the two comments above, looks good to me. Reviewed-by: Hongyan Xia <hongyxia@amazon.com>
Hi, On 23/03/2020 12:11, Hongyan Xia wrote: > On Sun, 2020-03-22 at 16:14 +0000, julien@xen.org wrote: >> From: Julien Grall <julien.grall@arm.com> >> >> The first parameter of {s,g}et_gpfn_from_mfn() is an MFN, so it can >> be >> switched to use the typesafe. >> >> At the same time, replace gpfn with pfn in the helpers as they all >> deal >> with PFN and also turn the macros to static inline. >> >> Note that the return of the getter and the 2nd parameter of the >> setter >> have not been converted to use typesafe PFN because it was requiring >> more changes than expected. >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> >> --- >> This was originally sent as part of "xen/arm: Properly disable >> M2P >> on Arm" [1]. >> >> Changes since the original version: >> - mfn_to_gmfn() is still present for now so update it >> - Remove stray + >> - Avoid churn in set_pfn_from_mfn() by inverting mfn and mfn_ >> - Remove tags >> - Fix build in mem_sharing >> >> [1] <20190603160350.29806-1-julien.grall@arm.com> >> --- >> xen/arch/x86/cpu/mcheck/mcaction.c | 2 +- >> xen/arch/x86/mm.c | 14 +++---- >> xen/arch/x86/mm/mem_sharing.c | 20 ++++----- >> xen/arch/x86/mm/p2m-pod.c | 4 +- >> xen/arch/x86/mm/p2m-pt.c | 35 ++++++++-------- >> xen/arch/x86/mm/p2m.c | 66 +++++++++++++++------------- >> -- >> xen/arch/x86/mm/paging.c | 4 +- >> xen/arch/x86/pv/dom0_build.c | 6 +-- >> xen/arch/x86/x86_64/traps.c | 8 ++-- >> xen/common/page_alloc.c | 2 +- >> xen/include/asm-arm/mm.h | 2 +- >> xen/include/asm-x86/grant_table.h | 2 +- >> xen/include/asm-x86/mm.h | 12 ++++-- >> xen/include/asm-x86/p2m.h | 2 +- >> 14 files changed, 93 insertions(+), 86 deletions(-) >> >> > > [...] > >> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h >> index abf4cc23e4..11614f9107 100644 >> --- a/xen/include/asm-arm/mm.h >> +++ b/xen/include/asm-arm/mm.h >> @@ -319,7 +319,7 @@ struct page_info *get_page_from_gva(struct vcpu >> *v, vaddr_t va, >> #define SHARED_M2P(_e) ((_e) == SHARED_M2P_ENTRY) >> >> /* Xen always owns P2M on ARM */ >> -#define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); >> } while (0) >> +static inline void set_pfn_from_mfn(mfn_t mfn, unsigned long pfn) {} >> #define mfn_to_gmfn(_d, mfn) (mfn) > > I do not have a setup to compile and test code for Arm, but wouldn't > the compiler complain about unused arguments here? The marco version > explicitly silenced compiler complaints. The macro version does not use (void)(arg) for silencing unused parameter. It is for evaluating (mfn) but ignore the result. A compiler would warn without (void) because we build Xen with -Wall which include -Wunused-value. Xen is not used with -Wunused-parameter, so there is no concern about unused parameters. If we ever decided to turn on -Wunused-parameter (or -Wextra), then we will have quite a bit of code to modify (such as callbacks not using all the parameters) to make it compile. > >> diff --git a/xen/include/asm-x86/grant_table.h b/xen/include/asm- >> x86/grant_table.h >> index 5871238f6d..b6a09c4c6c 100644 >> --- a/xen/include/asm-x86/grant_table.h >> +++ b/xen/include/asm-x86/grant_table.h >> @@ -41,7 +41,7 @@ static inline int >> replace_grant_host_mapping(uint64_t addr, mfn_t frame, >> #define gnttab_get_frame_gfn(gt, st, idx) >> ({ \ >> mfn_t mfn_ = (st) ? gnttab_status_mfn(gt, >> idx) \ >> : gnttab_shared_mfn(gt, >> idx); \ >> - unsigned long gpfn_ = >> get_gpfn_from_mfn(mfn_x(mfn_)); \ >> + unsigned long gpfn_ = >> get_pfn_from_mfn(mfn_); \ >> VALID_M2P(gpfn_) ? _gfn(gpfn_) : >> INVALID_GFN; \ >> }) >> >> diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h >> index 53f2ed7c7d..2a4f42e78f 100644 >> --- a/xen/include/asm-x86/mm.h >> +++ b/xen/include/asm-x86/mm.h >> @@ -500,9 +500,10 @@ extern paddr_t mem_hotplug; >> */ >> extern bool machine_to_phys_mapping_valid; >> >> -static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned >> long pfn) >> +static inline void set_pfn_from_mfn(mfn_t mfn_, unsigned long pfn) >> { >> - const struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn))); >> + const unsigned long mfn = mfn_x(mfn_); >> + const struct domain *d = page_get_owner(mfn_to_page(mfn_)); >> unsigned long entry = (d && (d == dom_cow)) ? SHARED_M2P_ENTRY : >> pfn; >> >> if ( !machine_to_phys_mapping_valid ) >> @@ -515,11 +516,14 @@ static inline void set_gpfn_from_mfn(unsigned >> long mfn, unsigned long pfn) >> >> extern struct rangeset *mmio_ro_ranges; >> >> -#define get_gpfn_from_mfn(mfn) (machine_to_phys_mapping[(mfn)]) >> +static inline unsigned long get_pfn_from_mfn(mfn_t mfn) >> +{ >> + return machine_to_phys_mapping[mfn_x(mfn)]; >> +} > > Any specific reason this (and some other macros) are turned into static > inline? I don't have a problem with them being inline functions but > just wondering if there is a reason to do so. static inline provides better safety check than macro. So we tend to switch to static inline whenever the headers inter-dependency madness is not interplaying. > >> #define mfn_to_gmfn(_d, mfn) \ >> ( (paging_mode_translate(_d)) \ >> - ? get_gpfn_from_mfn(mfn) \ >> + ? get_pfn_from_mfn(_mfn(mfn)) \ >> : (mfn) ) >> >> #define compat_pfn_to_cr3(pfn) (((unsigned)(pfn) << 12) | >> ((unsigned)(pfn) >> 20)) >> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h >> index a2c6049834..39dae242b0 100644 >> --- a/xen/include/asm-x86/p2m.h >> +++ b/xen/include/asm-x86/p2m.h >> @@ -505,7 +505,7 @@ static inline struct page_info >> *get_page_from_gfn( >> static inline gfn_t mfn_to_gfn(const struct domain *d, mfn_t mfn) >> { >> if ( paging_mode_translate(d) ) >> - return _gfn(get_gpfn_from_mfn(mfn_x(mfn))); >> + return _gfn(get_pfn_from_mfn(mfn)); >> else >> return _gfn(mfn_x(mfn)); >> } > > Apart from the two comments above, looks good to me. > > Reviewed-by: Hongyan Xia <hongyxia@amazon.com> Thank you! Cheers,
On 22.03.2020 17:14, julien@xen.org wrote: > @@ -983,19 +984,20 @@ long p2m_pt_audit_p2m(struct p2m_domain *p2m) > /* check for 1GB super page */ > if ( l3e_get_flags(l3e[i3]) & _PAGE_PSE ) > { > - mfn = l3e_get_pfn(l3e[i3]); > - ASSERT(mfn_valid(_mfn(mfn))); > + mfn = l3e_get_mfn(l3e[i3]); > + ASSERT(mfn_valid(mfn)); > /* we have to cover 512x512 4K pages */ > for ( i2 = 0; > i2 < (L2_PAGETABLE_ENTRIES * L1_PAGETABLE_ENTRIES); > i2++) > { > - m2pfn = get_gpfn_from_mfn(mfn+i2); > + m2pfn = get_pfn_from_mfn(mfn_add(mfn, i2)); > if ( m2pfn != (gfn + i2) ) > { > pmbad++; > - P2M_PRINTK("mismatch: gfn %#lx -> mfn %#lx -> gfn %#lx\n", > - gfn + i2, mfn + i2, m2pfn); > + P2M_PRINTK("mismatch: gfn %#lx -> mfn %"PRI_mfn" gfn %#lx\n", > + gfn + i2, mfn_x(mfn_add(mfn, i2)), As in the earlier patch, "mfn_x(mfn) + i2" would be shorter and hence imo preferable, especially in printk() and alike invocations. I would also prefer if you left %#lx alone, with the 2nd best option being to also use PRI_gfn alongside PRI_mfn. Primarily I'd like to avoid having a mixture. Same (for both) at least one more time further down. > @@ -974,7 +974,7 @@ guest_physmap_add_entry(struct domain *d, gfn_t gfn, mfn_t mfn, > P2M_DEBUG("old gfn=%#lx -> mfn %#lx\n", > gfn_x(ogfn) , mfn_x(omfn)); > if ( mfn_eq(omfn, mfn_add(mfn, i)) ) > - p2m_remove_page(p2m, gfn_x(ogfn), mfn_x(mfn_add(mfn, i)), > + p2m_remove_page(p2m, gfn_x(ogfn), mfn_add(mfn, i), > 0); Pull this up then onto the now shorter prior line? > @@ -2843,53 +2843,53 @@ void audit_p2m(struct domain *d, > spin_lock(&d->page_alloc_lock); > page_list_for_each ( page, &d->page_list ) > { > - mfn = mfn_x(page_to_mfn(page)); > + mfn = page_to_mfn(page); > > - P2M_PRINTK("auditing guest page, mfn=%#lx\n", mfn); > + P2M_PRINTK("auditing guest page, mfn=%"PRI_mfn"\n", mfn_x(mfn)); > > od = page_get_owner(page); > > if ( od != d ) > { > - P2M_PRINTK("mfn %"PRI_mfn" owner %pd != %pd\n", mfn, od, d); > + P2M_PRINTK("mfn %"PRI_mfn" owner %pd != %pd\n", mfn_x(mfn), od, d); > continue; > } > > - gfn = get_gpfn_from_mfn(mfn); > + gfn = get_pfn_from_mfn(mfn); > if ( gfn == INVALID_M2P_ENTRY ) > { > orphans_count++; > - P2M_PRINTK("orphaned guest page: mfn=%#lx has invalid gfn\n", > - mfn); > + P2M_PRINTK("orphaned guest page: mfn=%"PRI_mfn" has invalid gfn\n", > + mfn_x(mfn)); > continue; > } > > if ( SHARED_M2P(gfn) ) > { > - P2M_PRINTK("shared mfn (%lx) on domain page list!\n", > - mfn); > + P2M_PRINTK("shared mfn (%"PRI_mfn") on domain page list!\n", > + mfn_x(mfn)); > continue; > } > > p2mfn = get_gfn_type_access(p2m, gfn, &type, &p2ma, 0, NULL); > - if ( mfn_x(p2mfn) != mfn ) > + if ( !mfn_eq(p2mfn, mfn) ) > { > mpbad++; > - P2M_PRINTK("map mismatch mfn %#lx -> gfn %#lx -> mfn %#lx" > + P2M_PRINTK("map mismatch mfn %"PRI_mfn" -> gfn %#lx -> mfn %"PRI_mfn"" > " (-> gfn %#lx)\n", > - mfn, gfn, mfn_x(p2mfn), > + mfn_x(mfn), gfn, mfn_x(p2mfn), > (mfn_valid(p2mfn) > - ? get_gpfn_from_mfn(mfn_x(p2mfn)) > + ? get_pfn_from_mfn(p2mfn) > : -1u)); I realize this is an entirely unrelated change, but the -1u here is standing out too much to not mention it: Could I talk you into making this gfn_x(INVALID_GFN) at this occasion? > --- a/xen/include/asm-x86/mm.h > +++ b/xen/include/asm-x86/mm.h > @@ -500,9 +500,10 @@ extern paddr_t mem_hotplug; > */ > extern bool machine_to_phys_mapping_valid; > > -static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn) > +static inline void set_pfn_from_mfn(mfn_t mfn_, unsigned long pfn) > { > - const struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn))); > + const unsigned long mfn = mfn_x(mfn_); I think it would be better overall if the parameter was named "mfn" and there was no local variable altogether. This would bring things in line with ... > @@ -515,11 +516,14 @@ static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn) > > extern struct rangeset *mmio_ro_ranges; > > -#define get_gpfn_from_mfn(mfn) (machine_to_phys_mapping[(mfn)]) > +static inline unsigned long get_pfn_from_mfn(mfn_t mfn) > +{ > + return machine_to_phys_mapping[mfn_x(mfn)]; > +} ... this. Jan
Hi, On 27/03/2020 13:15, Jan Beulich wrote: > On 22.03.2020 17:14, julien@xen.org wrote: >> @@ -983,19 +984,20 @@ long p2m_pt_audit_p2m(struct p2m_domain *p2m) >> /* check for 1GB super page */ >> if ( l3e_get_flags(l3e[i3]) & _PAGE_PSE ) >> { >> - mfn = l3e_get_pfn(l3e[i3]); >> - ASSERT(mfn_valid(_mfn(mfn))); >> + mfn = l3e_get_mfn(l3e[i3]); >> + ASSERT(mfn_valid(mfn)); >> /* we have to cover 512x512 4K pages */ >> for ( i2 = 0; >> i2 < (L2_PAGETABLE_ENTRIES * L1_PAGETABLE_ENTRIES); >> i2++) >> { >> - m2pfn = get_gpfn_from_mfn(mfn+i2); >> + m2pfn = get_pfn_from_mfn(mfn_add(mfn, i2)); >> if ( m2pfn != (gfn + i2) ) >> { >> pmbad++; >> - P2M_PRINTK("mismatch: gfn %#lx -> mfn %#lx -> gfn %#lx\n", >> - gfn + i2, mfn + i2, m2pfn); >> + P2M_PRINTK("mismatch: gfn %#lx -> mfn %"PRI_mfn" gfn %#lx\n", >> + gfn + i2, mfn_x(mfn_add(mfn, i2)), > > As in the earlier patch, "mfn_x(mfn) + i2" would be shorter and > hence imo preferable, especially in printk() and alike invocations. The goal of using typesafe is to make the code safer not try to open-code everything because it might be shorter to write. > > I would also prefer if you left %#lx alone, with the 2nd best > option being to also use PRI_gfn alongside PRI_mfn. Primarily > I'd like to avoid having a mixture. The two options would be wrong: * gfn is an unsigned long and not gfn_t, so using PRI_gfn would be incorrect * mfn is now an mfn_t so using %lx would be incorrect So the format string used in the patch is correct based on the types used. This... > > Same (for both) at least one more time further down. ... would likely be applicable for all the other uses. >> @@ -974,7 +974,7 @@ guest_physmap_add_entry(struct domain *d, gfn_t gfn, mfn_t mfn, >> P2M_DEBUG("old gfn=%#lx -> mfn %#lx\n", >> gfn_x(ogfn) , mfn_x(omfn)); >> if ( mfn_eq(omfn, mfn_add(mfn, i)) ) >> - p2m_remove_page(p2m, gfn_x(ogfn), mfn_x(mfn_add(mfn, i)), >> + p2m_remove_page(p2m, gfn_x(ogfn), mfn_add(mfn, i), >> 0); > > Pull this up then onto the now shorter prior line? Ok. > >> @@ -2843,53 +2843,53 @@ void audit_p2m(struct domain *d, >> spin_lock(&d->page_alloc_lock); >> page_list_for_each ( page, &d->page_list ) >> { >> - mfn = mfn_x(page_to_mfn(page)); >> + mfn = page_to_mfn(page); >> >> - P2M_PRINTK("auditing guest page, mfn=%#lx\n", mfn); >> + P2M_PRINTK("auditing guest page, mfn=%"PRI_mfn"\n", mfn_x(mfn)); >> >> od = page_get_owner(page); >> >> if ( od != d ) >> { >> - P2M_PRINTK("mfn %"PRI_mfn" owner %pd != %pd\n", mfn, od, d); >> + P2M_PRINTK("mfn %"PRI_mfn" owner %pd != %pd\n", mfn_x(mfn), od, d); >> continue; >> } >> >> - gfn = get_gpfn_from_mfn(mfn); >> + gfn = get_pfn_from_mfn(mfn); >> if ( gfn == INVALID_M2P_ENTRY ) >> { >> orphans_count++; >> - P2M_PRINTK("orphaned guest page: mfn=%#lx has invalid gfn\n", >> - mfn); >> + P2M_PRINTK("orphaned guest page: mfn=%"PRI_mfn" has invalid gfn\n", >> + mfn_x(mfn)); >> continue; >> } >> >> if ( SHARED_M2P(gfn) ) >> { >> - P2M_PRINTK("shared mfn (%lx) on domain page list!\n", >> - mfn); >> + P2M_PRINTK("shared mfn (%"PRI_mfn") on domain page list!\n", >> + mfn_x(mfn)); >> continue; >> } >> >> p2mfn = get_gfn_type_access(p2m, gfn, &type, &p2ma, 0, NULL); >> - if ( mfn_x(p2mfn) != mfn ) >> + if ( !mfn_eq(p2mfn, mfn) ) >> { >> mpbad++; >> - P2M_PRINTK("map mismatch mfn %#lx -> gfn %#lx -> mfn %#lx" >> + P2M_PRINTK("map mismatch mfn %"PRI_mfn" -> gfn %#lx -> mfn %"PRI_mfn"" >> " (-> gfn %#lx)\n", >> - mfn, gfn, mfn_x(p2mfn), >> + mfn_x(mfn), gfn, mfn_x(p2mfn), >> (mfn_valid(p2mfn) >> - ? get_gpfn_from_mfn(mfn_x(p2mfn)) >> + ? get_pfn_from_mfn(p2mfn) >> : -1u)); > > I realize this is an entirely unrelated change, but the -1u here > is standing out too much to not mention it: Could I talk you into > making this gfn_x(INVALID_GFN) at this occasion? Hmmm, I am not sure why I missed this one. I will use gfn_x(INVALID_GFN). > >> --- a/xen/include/asm-x86/mm.h >> +++ b/xen/include/asm-x86/mm.h >> @@ -500,9 +500,10 @@ extern paddr_t mem_hotplug; >> */ >> extern bool machine_to_phys_mapping_valid; >> >> -static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn) >> +static inline void set_pfn_from_mfn(mfn_t mfn_, unsigned long pfn) >> { >> - const struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn))); >> + const unsigned long mfn = mfn_x(mfn_); > > I think it would be better overall if the parameter was named > "mfn" and there was no local variable altogether. This would > bring things in line with ... You asked for this approach on the previous version [1]: "Btw, the cheaper (in terms of code churn) change here would seem to be to name the function parameter mfn_, and the local variable mfn. That'll also reduce the number of uses of the unfortunate trailing- underscore-name." So can you pick a side and stick with it? > >> @@ -515,11 +516,14 @@ static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn) >> >> extern struct rangeset *mmio_ro_ranges; >> >> -#define get_gpfn_from_mfn(mfn) (machine_to_phys_mapping[(mfn)]) >> +static inline unsigned long get_pfn_from_mfn(mfn_t mfn) >> +{ >> + return machine_to_phys_mapping[mfn_x(mfn)]; >> +} > > ... this. > > Jan > Cheers, [1] <5CF7A1090200007800235782@prv1-mh.provo.novell.com>
On 28.03.2020 12:14, Julien Grall wrote: > On 27/03/2020 13:15, Jan Beulich wrote: >> On 22.03.2020 17:14, julien@xen.org wrote: >>> @@ -983,19 +984,20 @@ long p2m_pt_audit_p2m(struct p2m_domain *p2m) >>> /* check for 1GB super page */ >>> if ( l3e_get_flags(l3e[i3]) & _PAGE_PSE ) >>> { >>> - mfn = l3e_get_pfn(l3e[i3]); >>> - ASSERT(mfn_valid(_mfn(mfn))); >>> + mfn = l3e_get_mfn(l3e[i3]); >>> + ASSERT(mfn_valid(mfn)); >>> /* we have to cover 512x512 4K pages */ >>> for ( i2 = 0; >>> i2 < (L2_PAGETABLE_ENTRIES * L1_PAGETABLE_ENTRIES); >>> i2++) >>> { >>> - m2pfn = get_gpfn_from_mfn(mfn+i2); >>> + m2pfn = get_pfn_from_mfn(mfn_add(mfn, i2)); >>> if ( m2pfn != (gfn + i2) ) >>> { >>> pmbad++; >>> - P2M_PRINTK("mismatch: gfn %#lx -> mfn %#lx -> gfn %#lx\n", >>> - gfn + i2, mfn + i2, m2pfn); >>> + P2M_PRINTK("mismatch: gfn %#lx -> mfn %"PRI_mfn" gfn %#lx\n", >>> + gfn + i2, mfn_x(mfn_add(mfn, i2)), >> >> As in the earlier patch, "mfn_x(mfn) + i2" would be shorter and >> hence imo preferable, especially in printk() and alike invocations. > > The goal of using typesafe is to make the code safer not try to > open-code everything because it might be shorter to write. I'm not talking about "everything". As soon as you use mfn_x() _anywhere_, type-safety is gone. Since in printk() and alike you unavoidably have to use it (at least for now), there's no win from using e.g. mfn_add() as you do here, imo. And hence the readability aspect gets even higher significance. >> I would also prefer if you left %#lx alone, with the 2nd best >> option being to also use PRI_gfn alongside PRI_mfn. Primarily >> I'd like to avoid having a mixture. > The two options would be wrong: > * gfn is an unsigned long and not gfn_t, so using PRI_gfn would be incorrect > * mfn is now an mfn_t so using %lx would be incorrect > > So the format string used in the patch is correct based on the types used. Hmm, xen/mm.h suggests a partial connection between e.g. mfn_t and PRI_mfn, yes, but I think this is unhelpful as long as mfn_x() needs to be explicitly used when specifying the printk() arguments. Instead I view PRI_mfn and alike as a more general format usable also for MFNs stored in unsigned long rather than mfn_t. I agree though that views here may differ. Hence wider agreement on what the intentions are (also mid/long term), and hence how well formed code ought to look like, would seem necessary here. > This... > >> >> Same (for both) at least one more time further down. > > ... would likely be applicable for all the other uses. Agreed. >>> --- a/xen/include/asm-x86/mm.h >>> +++ b/xen/include/asm-x86/mm.h >>> @@ -500,9 +500,10 @@ extern paddr_t mem_hotplug; >>> */ >>> extern bool machine_to_phys_mapping_valid; >>> -static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn) >>> +static inline void set_pfn_from_mfn(mfn_t mfn_, unsigned long pfn) >>> { >>> - const struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn))); >>> + const unsigned long mfn = mfn_x(mfn_); >> >> I think it would be better overall if the parameter was named >> "mfn" and there was no local variable altogether. This would >> bring things in line with ... > > You asked for this approach on the previous version [1]: > > "Btw, the cheaper (in terms of code churn) change here would seem to > be to name the function parameter mfn_, and the local variable mfn. > That'll also reduce the number of uses of the unfortunate trailing- > underscore-name." > > So can you pick a side and stick with it? Well, things like this happen when you see the final result, sorry. And indeed I recalled commenting on this before, but upon searching I didn't manage to find the earlier reply, to better justify what I also suspected might have been a change of mind. Jan
diff --git a/xen/arch/x86/cpu/mcheck/mcaction.c b/xen/arch/x86/cpu/mcheck/mcaction.c index 69332fb84d..5e78fb7703 100644 --- a/xen/arch/x86/cpu/mcheck/mcaction.c +++ b/xen/arch/x86/cpu/mcheck/mcaction.c @@ -89,7 +89,7 @@ mc_memerr_dhandler(struct mca_binfo *binfo, { d = get_domain_by_id(bank->mc_domid); ASSERT(d); - gfn = get_gpfn_from_mfn((bank->mc_addr) >> PAGE_SHIFT); + gfn = get_pfn_from_mfn(maddr_to_mfn(bank->mc_addr)); if ( unmmap_broken_page(d, mfn, gfn) ) { diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 2516548e49..2feb7a5993 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -476,7 +476,7 @@ void share_xen_page_with_guest(struct page_info *page, struct domain *d, if ( page_get_owner(page) == d ) return; - set_gpfn_from_mfn(mfn_x(page_to_mfn(page)), INVALID_M2P_ENTRY); + set_pfn_from_mfn(page_to_mfn(page), INVALID_M2P_ENTRY); spin_lock(&d->page_alloc_lock); @@ -1040,7 +1040,7 @@ get_page_from_l1e( gdprintk(XENLOG_WARNING, "Error updating mappings for mfn %" PRI_mfn " (pfn %" PRI_pfn ", from L1 entry %" PRIpte ") for d%d\n", - mfn, get_gpfn_from_mfn(mfn), + mfn, get_pfn_from_mfn(_mfn(mfn)), l1e_get_intpte(l1e), l1e_owner->domain_id); return err; } @@ -1051,7 +1051,7 @@ get_page_from_l1e( could_not_pin: gdprintk(XENLOG_WARNING, "Error getting mfn %" PRI_mfn " (pfn %" PRI_pfn ") from L1 entry %" PRIpte " for l1e_owner d%d, pg_owner d%d\n", - mfn, get_gpfn_from_mfn(mfn), + mfn, get_pfn_from_mfn(_mfn(mfn)), l1e_get_intpte(l1e), l1e_owner->domain_id, pg_owner->domain_id); if ( real_pg_owner != NULL ) put_page(page); @@ -2636,7 +2636,7 @@ static int validate_page(struct page_info *page, unsigned long type, " (pfn %" PRI_pfn ") for type %" PRtype_info ": caf=%08lx taf=%" PRtype_info "\n", mfn_x(page_to_mfn(page)), - get_gpfn_from_mfn(mfn_x(page_to_mfn(page))), + get_pfn_from_mfn(page_to_mfn(page)), type, page->count_info, page->u.inuse.type_info); if ( page != current->arch.old_guest_table ) page->u.inuse.type_info = 0; @@ -2946,7 +2946,7 @@ static int _get_page_type(struct page_info *page, unsigned long type, "Bad type (saw %" PRtype_info " != exp %" PRtype_info ") " "for mfn %" PRI_mfn " (pfn %" PRI_pfn ")\n", x, type, mfn_x(page_to_mfn(page)), - get_gpfn_from_mfn(mfn_x(page_to_mfn(page)))); + get_pfn_from_mfn(page_to_mfn(page))); return -EINVAL; } else if ( unlikely(!(x & PGT_validated)) ) @@ -4106,7 +4106,7 @@ long do_mmu_update( break; } - set_gpfn_from_mfn(mfn_x(mfn), gpfn); + set_pfn_from_mfn(mfn, gpfn); paging_mark_pfn_dirty(pg_owner, _pfn(gpfn)); put_page(page); @@ -4590,7 +4590,7 @@ int xenmem_add_to_physmap_one( goto put_both; /* Unmap from old location, if any. */ - old_gpfn = get_gpfn_from_mfn(mfn_x(mfn)); + old_gpfn = get_pfn_from_mfn(mfn); ASSERT(!SHARED_M2P(old_gpfn)); if ( space == XENMAPSPACE_gmfn && old_gpfn != gfn ) { diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c index 3835bc928f..018beec10f 100644 --- a/xen/arch/x86/mm/mem_sharing.c +++ b/xen/arch/x86/mm/mem_sharing.c @@ -426,15 +426,15 @@ static void mem_sharing_gfn_destroy(struct page_info *page, struct domain *d, xfree(gfn_info); } -static struct page_info *mem_sharing_lookup(unsigned long mfn) +static struct page_info *mem_sharing_lookup(mfn_t mfn) { struct page_info *page; unsigned long t; - if ( !mfn_valid(_mfn(mfn)) ) + if ( !mfn_valid(mfn) ) return NULL; - page = mfn_to_page(_mfn(mfn)); + page = mfn_to_page(mfn); if ( page_get_owner(page) != dom_cow ) return NULL; @@ -446,7 +446,7 @@ static struct page_info *mem_sharing_lookup(unsigned long mfn) t = read_atomic(&page->u.inuse.type_info); ASSERT((t & PGT_type_mask) == PGT_shared_page); ASSERT((t & PGT_count_mask) >= 2); - ASSERT(SHARED_M2P(get_gpfn_from_mfn(mfn))); + ASSERT(SHARED_M2P(get_pfn_from_mfn(mfn))); return page; } @@ -505,10 +505,10 @@ static int audit(void) } /* Check the m2p entry */ - if ( !SHARED_M2P(get_gpfn_from_mfn(mfn_x(mfn))) ) + if ( !SHARED_M2P(get_pfn_from_mfn(mfn)) ) { - gdprintk(XENLOG_ERR, "mfn %lx shared, but wrong m2p entry (%lx)!\n", - mfn_x(mfn), get_gpfn_from_mfn(mfn_x(mfn))); + gdprintk(XENLOG_ERR, "mfn %"PRI_mfn" shared, but wrong m2p entry (%lx)!\n", + mfn_x(mfn), get_pfn_from_mfn(mfn)); errors++; } @@ -736,7 +736,7 @@ static struct page_info *__grab_shared_page(mfn_t mfn) if ( !mem_sharing_page_lock(pg) ) return NULL; - if ( mem_sharing_lookup(mfn_x(mfn)) == NULL ) + if ( mem_sharing_lookup(mfn) == NULL ) { mem_sharing_page_unlock(pg); return NULL; @@ -918,7 +918,7 @@ static int nominate_page(struct domain *d, gfn_t gfn, atomic_inc(&nr_shared_mfns); /* Update m2p entry to SHARED_M2P_ENTRY */ - set_gpfn_from_mfn(mfn_x(mfn), SHARED_M2P_ENTRY); + set_pfn_from_mfn(mfn, SHARED_M2P_ENTRY); *phandle = page->sharing->handle; audit_add_list(page); @@ -1306,7 +1306,7 @@ int __mem_sharing_unshare_page(struct domain *d, } /* Update m2p entry */ - set_gpfn_from_mfn(mfn_x(page_to_mfn(page)), gfn); + set_pfn_from_mfn(page_to_mfn(page), gfn); /* * Now that the gfn<->mfn map is properly established, diff --git a/xen/arch/x86/mm/p2m-pod.c b/xen/arch/x86/mm/p2m-pod.c index 2a7b8c117b..a9ac44a65c 100644 --- a/xen/arch/x86/mm/p2m-pod.c +++ b/xen/arch/x86/mm/p2m-pod.c @@ -644,7 +644,7 @@ p2m_pod_decrease_reservation(struct domain *d, gfn_t gfn, unsigned int order) } p2m_tlb_flush_sync(p2m); for ( j = 0; j < n; ++j ) - set_gpfn_from_mfn(mfn_x(mfn), INVALID_M2P_ENTRY); + set_pfn_from_mfn(mfn, INVALID_M2P_ENTRY); p2m_pod_cache_add(p2m, page, cur_order); steal_for_cache = ( p2m->pod.entry_count > p2m->pod.count ); @@ -1194,7 +1194,7 @@ p2m_pod_demand_populate(struct p2m_domain *p2m, gfn_t gfn, for( i = 0; i < (1UL << order); i++ ) { - set_gpfn_from_mfn(mfn_x(mfn) + i, gfn_x(gfn_aligned) + i); + set_pfn_from_mfn(mfn_add(mfn, i), gfn_x(gfn_aligned) + i); paging_mark_pfn_dirty(d, _pfn(gfn_x(gfn_aligned) + i)); } diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c index e9da34d668..1601e9e5e9 100644 --- a/xen/arch/x86/mm/p2m-pt.c +++ b/xen/arch/x86/mm/p2m-pt.c @@ -944,7 +944,8 @@ static int p2m_pt_change_entry_type_range(struct p2m_domain *p2m, long p2m_pt_audit_p2m(struct p2m_domain *p2m) { unsigned long entry_count = 0, pmbad = 0; - unsigned long mfn, gfn, m2pfn; + unsigned long gfn, m2pfn; + mfn_t mfn; ASSERT(p2m_locked_by_me(p2m)); ASSERT(pod_locked_by_me(p2m)); @@ -983,19 +984,20 @@ long p2m_pt_audit_p2m(struct p2m_domain *p2m) /* check for 1GB super page */ if ( l3e_get_flags(l3e[i3]) & _PAGE_PSE ) { - mfn = l3e_get_pfn(l3e[i3]); - ASSERT(mfn_valid(_mfn(mfn))); + mfn = l3e_get_mfn(l3e[i3]); + ASSERT(mfn_valid(mfn)); /* we have to cover 512x512 4K pages */ for ( i2 = 0; i2 < (L2_PAGETABLE_ENTRIES * L1_PAGETABLE_ENTRIES); i2++) { - m2pfn = get_gpfn_from_mfn(mfn+i2); + m2pfn = get_pfn_from_mfn(mfn_add(mfn, i2)); if ( m2pfn != (gfn + i2) ) { pmbad++; - P2M_PRINTK("mismatch: gfn %#lx -> mfn %#lx -> gfn %#lx\n", - gfn + i2, mfn + i2, m2pfn); + P2M_PRINTK("mismatch: gfn %#lx -> mfn %"PRI_mfn" gfn %#lx\n", + gfn + i2, mfn_x(mfn_add(mfn, i2)), + m2pfn); BUG(); } gfn += 1 << (L3_PAGETABLE_SHIFT - PAGE_SHIFT); @@ -1019,17 +1021,18 @@ long p2m_pt_audit_p2m(struct p2m_domain *p2m) /* check for super page */ if ( l2e_get_flags(l2e[i2]) & _PAGE_PSE ) { - mfn = l2e_get_pfn(l2e[i2]); - ASSERT(mfn_valid(_mfn(mfn))); + mfn = l2e_get_mfn(l2e[i2]); + ASSERT(mfn_valid(mfn)); for ( i1 = 0; i1 < L1_PAGETABLE_ENTRIES; i1++) { - m2pfn = get_gpfn_from_mfn(mfn+i1); + m2pfn = get_pfn_from_mfn(mfn_add(mfn, i1)); /* Allow shared M2Ps */ if ( (m2pfn != (gfn + i1)) && !SHARED_M2P(m2pfn) ) { pmbad++; - P2M_PRINTK("mismatch: gfn %#lx -> mfn %#lx -> gfn %#lx\n", - gfn + i1, mfn + i1, m2pfn); + P2M_PRINTK("mismatch: gfn %#lx -> mfn %"PRI_mfn" -> gfn %#lx\n", + gfn + i1, mfn_x(mfn_add(mfn, i1)), + m2pfn); BUG(); } } @@ -1050,17 +1053,17 @@ long p2m_pt_audit_p2m(struct p2m_domain *p2m) entry_count++; continue; } - mfn = l1e_get_pfn(l1e[i1]); - ASSERT(mfn_valid(_mfn(mfn))); - m2pfn = get_gpfn_from_mfn(mfn); + mfn = l1e_get_mfn(l1e[i1]); + ASSERT(mfn_valid(mfn)); + m2pfn = get_pfn_from_mfn(mfn); if ( m2pfn != gfn && type != p2m_mmio_direct && !p2m_is_grant(type) && !p2m_is_shared(type) ) { pmbad++; - P2M_PRINTK("mismatch: gfn %#lx -> mfn %#lx -> gfn %#lx\n", - gfn, mfn, m2pfn); + P2M_PRINTK("mismatch: gfn %#lx -> mfn %"PRI_mfn" -> gfn %#lx\n", + gfn, mfn_x(mfn), m2pfn); BUG(); } } diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index b6b01a71c8..587c062481 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -769,7 +769,7 @@ void p2m_final_teardown(struct domain *d) static int -p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn_l, unsigned long mfn, +p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn_l, mfn_t mfn, unsigned int page_order) { unsigned long i; @@ -783,17 +783,17 @@ p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn_l, unsigned long mfn, return 0; ASSERT(gfn_locked_by_me(p2m, gfn)); - P2M_DEBUG("removing gfn=%#lx mfn=%#lx\n", gfn_l, mfn); + P2M_DEBUG("removing gfn=%#lx mfn=%"PRI_mfn"\n", gfn_l, mfn_x(mfn)); - if ( mfn_valid(_mfn(mfn)) ) + if ( mfn_valid(mfn) ) { for ( i = 0; i < (1UL << page_order); i++ ) { mfn_return = p2m->get_entry(p2m, gfn_add(gfn, i), &t, &a, 0, NULL, NULL); if ( !p2m_is_grant(t) && !p2m_is_shared(t) && !p2m_is_foreign(t) ) - set_gpfn_from_mfn(mfn+i, INVALID_M2P_ENTRY); - ASSERT( !p2m_is_valid(t) || mfn + i == mfn_x(mfn_return) ); + set_pfn_from_mfn(mfn_add(mfn, i), INVALID_M2P_ENTRY); + ASSERT( !p2m_is_valid(t) || mfn_eq(mfn_add(mfn, i), mfn_return) ); } } return p2m_set_entry(p2m, gfn, INVALID_MFN, page_order, p2m_invalid, @@ -807,7 +807,7 @@ guest_physmap_remove_page(struct domain *d, gfn_t gfn, struct p2m_domain *p2m = p2m_get_hostp2m(d); int rc; gfn_lock(p2m, gfn, page_order); - rc = p2m_remove_page(p2m, gfn_x(gfn), mfn_x(mfn), page_order); + rc = p2m_remove_page(p2m, gfn_x(gfn), mfn, page_order); gfn_unlock(p2m, gfn, page_order); return rc; } @@ -842,7 +842,7 @@ guest_physmap_add_page(struct domain *d, gfn_t gfn, mfn_t mfn, else return -EINVAL; - set_gpfn_from_mfn(mfn_x(mfn) + i, gfn_x(gfn) + i); + set_pfn_from_mfn(mfn_add(mfn, i), gfn_x(gfn) + i); } return 0; @@ -930,7 +930,7 @@ guest_physmap_add_entry(struct domain *d, gfn_t gfn, mfn_t mfn, else if ( p2m_is_ram(ot) && !p2m_is_paged(ot) ) { ASSERT(mfn_valid(omfn)); - set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY); + set_pfn_from_mfn(omfn, INVALID_M2P_ENTRY); } else if ( ot == p2m_populate_on_demand ) { @@ -974,7 +974,7 @@ guest_physmap_add_entry(struct domain *d, gfn_t gfn, mfn_t mfn, P2M_DEBUG("old gfn=%#lx -> mfn %#lx\n", gfn_x(ogfn) , mfn_x(omfn)); if ( mfn_eq(omfn, mfn_add(mfn, i)) ) - p2m_remove_page(p2m, gfn_x(ogfn), mfn_x(mfn_add(mfn, i)), + p2m_remove_page(p2m, gfn_x(ogfn), mfn_add(mfn, i), 0); } } @@ -992,8 +992,8 @@ guest_physmap_add_entry(struct domain *d, gfn_t gfn, mfn_t mfn, if ( !p2m_is_grant(t) ) { for ( i = 0; i < (1UL << page_order); i++ ) - set_gpfn_from_mfn(mfn_x(mfn_add(mfn, i)), - gfn_x(gfn_add(gfn, i))); + set_pfn_from_mfn(mfn_add(mfn, i), + gfn_x(gfn_add(gfn, i))); } } @@ -1279,7 +1279,7 @@ static int set_typed_p2m_entry(struct domain *d, unsigned long gfn_l, for ( i = 0; i < (1UL << order); ++i ) { ASSERT(mfn_valid(mfn_add(omfn, i))); - set_gpfn_from_mfn(mfn_x(omfn) + i, INVALID_M2P_ENTRY); + set_pfn_from_mfn(mfn_add(omfn, i), INVALID_M2P_ENTRY); } } @@ -1475,7 +1475,7 @@ int set_shared_p2m_entry(struct domain *d, unsigned long gfn_l, mfn_t mfn) pg_type = read_atomic(&(mfn_to_page(omfn)->u.inuse.type_info)); if ( (pg_type & PGT_count_mask) == 0 || (pg_type & PGT_type_mask) != PGT_shared_page ) - set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY); + set_pfn_from_mfn(omfn, INVALID_M2P_ENTRY); P2M_DEBUG("set shared %lx %lx\n", gfn_l, mfn_x(mfn)); rc = p2m_set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2m_ram_shared, @@ -1829,7 +1829,7 @@ int p2m_mem_paging_prep(struct domain *d, unsigned long gfn_l, uint64_t buffer) ret = p2m_set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, paging_mode_log_dirty(d) ? p2m_ram_logdirty : p2m_ram_rw, a); - set_gpfn_from_mfn(mfn_x(mfn), gfn_l); + set_pfn_from_mfn(mfn, gfn_l); if ( !page_extant ) atomic_dec(&d->paged_pages); @@ -1880,7 +1880,7 @@ void p2m_mem_paging_resume(struct domain *d, vm_event_response_t *rsp) p2m_ram_rw, a); if ( !rc ) - set_gpfn_from_mfn(mfn_x(mfn), gfn_x(gfn)); + set_pfn_from_mfn(mfn, gfn_x(gfn)); } gfn_unlock(p2m, gfn, 0); } @@ -2706,7 +2706,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx, { mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL); if ( mfn_valid(mfn) ) - p2m_remove_page(ap2m, gfn_x(old_gfn), mfn_x(mfn), PAGE_ORDER_4K); + p2m_remove_page(ap2m, gfn_x(old_gfn), mfn, PAGE_ORDER_4K); rc = 0; goto out; } @@ -2820,8 +2820,8 @@ void audit_p2m(struct domain *d, { struct page_info *page; struct domain *od; - unsigned long mfn, gfn; - mfn_t p2mfn; + unsigned long gfn; + mfn_t p2mfn, mfn; unsigned long orphans_count = 0, mpbad = 0, pmbad = 0; p2m_access_t p2ma; p2m_type_t type; @@ -2843,53 +2843,53 @@ void audit_p2m(struct domain *d, spin_lock(&d->page_alloc_lock); page_list_for_each ( page, &d->page_list ) { - mfn = mfn_x(page_to_mfn(page)); + mfn = page_to_mfn(page); - P2M_PRINTK("auditing guest page, mfn=%#lx\n", mfn); + P2M_PRINTK("auditing guest page, mfn=%"PRI_mfn"\n", mfn_x(mfn)); od = page_get_owner(page); if ( od != d ) { - P2M_PRINTK("mfn %"PRI_mfn" owner %pd != %pd\n", mfn, od, d); + P2M_PRINTK("mfn %"PRI_mfn" owner %pd != %pd\n", mfn_x(mfn), od, d); continue; } - gfn = get_gpfn_from_mfn(mfn); + gfn = get_pfn_from_mfn(mfn); if ( gfn == INVALID_M2P_ENTRY ) { orphans_count++; - P2M_PRINTK("orphaned guest page: mfn=%#lx has invalid gfn\n", - mfn); + P2M_PRINTK("orphaned guest page: mfn=%"PRI_mfn" has invalid gfn\n", + mfn_x(mfn)); continue; } if ( SHARED_M2P(gfn) ) { - P2M_PRINTK("shared mfn (%lx) on domain page list!\n", - mfn); + P2M_PRINTK("shared mfn (%"PRI_mfn") on domain page list!\n", + mfn_x(mfn)); continue; } p2mfn = get_gfn_type_access(p2m, gfn, &type, &p2ma, 0, NULL); - if ( mfn_x(p2mfn) != mfn ) + if ( !mfn_eq(p2mfn, mfn) ) { mpbad++; - P2M_PRINTK("map mismatch mfn %#lx -> gfn %#lx -> mfn %#lx" + P2M_PRINTK("map mismatch mfn %"PRI_mfn" -> gfn %#lx -> mfn %"PRI_mfn"" " (-> gfn %#lx)\n", - mfn, gfn, mfn_x(p2mfn), + mfn_x(mfn), gfn, mfn_x(p2mfn), (mfn_valid(p2mfn) - ? get_gpfn_from_mfn(mfn_x(p2mfn)) + ? get_pfn_from_mfn(p2mfn) : -1u)); /* This m2p entry is stale: the domain has another frame in * this physical slot. No great disaster, but for neatness, * blow away the m2p entry. */ - set_gpfn_from_mfn(mfn, INVALID_M2P_ENTRY); + set_pfn_from_mfn(mfn, INVALID_M2P_ENTRY); } __put_gfn(p2m, gfn); - P2M_PRINTK("OK: mfn=%#lx, gfn=%#lx, p2mfn=%#lx\n", - mfn, gfn, mfn_x(p2mfn)); + P2M_PRINTK("OK: mfn=%"PRI_mfn", gfn=%#lx, p2mfn=%"PRI_mfn"\n", + mfn_x(mfn), gfn, mfn_x(p2mfn)); } spin_unlock(&d->page_alloc_lock); diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c index 469bb76429..2f6df74135 100644 --- a/xen/arch/x86/mm/paging.c +++ b/xen/arch/x86/mm/paging.c @@ -344,7 +344,7 @@ void paging_mark_dirty(struct domain *d, mfn_t gmfn) return; /* We /really/ mean PFN here, even for non-translated guests. */ - pfn = _pfn(get_gpfn_from_mfn(mfn_x(gmfn))); + pfn = _pfn(get_pfn_from_mfn(gmfn)); paging_mark_pfn_dirty(d, pfn); } @@ -362,7 +362,7 @@ int paging_mfn_is_dirty(struct domain *d, mfn_t gmfn) ASSERT(paging_mode_log_dirty(d)); /* We /really/ mean PFN here, even for non-translated guests. */ - pfn = _pfn(get_gpfn_from_mfn(mfn_x(gmfn))); + pfn = _pfn(get_pfn_from_mfn(gmfn)); /* Invalid pages can't be dirty. */ if ( unlikely(!VALID_M2P(pfn_x(pfn))) ) return 0; diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c index 8abd5d255c..9f558b2932 100644 --- a/xen/arch/x86/pv/dom0_build.c +++ b/xen/arch/x86/pv/dom0_build.c @@ -39,7 +39,7 @@ void __init dom0_update_physmap(struct domain *d, unsigned long pfn, else ((unsigned int *)vphysmap_s)[pfn] = mfn; - set_gpfn_from_mfn(mfn, pfn); + set_pfn_from_mfn(_mfn(mfn), pfn); } static __init void mark_pv_pt_pages_rdonly(struct domain *d, @@ -789,8 +789,8 @@ int __init dom0_construct_pv(struct domain *d, page_list_for_each ( page, &d->page_list ) { mfn = mfn_x(page_to_mfn(page)); - BUG_ON(SHARED_M2P(get_gpfn_from_mfn(mfn))); - if ( get_gpfn_from_mfn(mfn) >= count ) + BUG_ON(SHARED_M2P(get_pfn_from_mfn(_mfn(mfn)))); + if ( get_pfn_from_mfn(_mfn(mfn)) >= count ) { BUG_ON(is_pv_32bit_domain(d)); if ( !page->u.inuse.type_info && diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c index 811c2cb37b..bf5c2060e7 100644 --- a/xen/arch/x86/x86_64/traps.c +++ b/xen/arch/x86/x86_64/traps.c @@ -200,7 +200,7 @@ void show_page_walk(unsigned long addr) unmap_domain_page(l4t); mfn = l4e_get_mfn(l4e); pfn = mfn_valid(mfn) && machine_to_phys_mapping_valid ? - get_gpfn_from_mfn(mfn_x(mfn)) : INVALID_M2P_ENTRY; + get_pfn_from_mfn(mfn) : INVALID_M2P_ENTRY; printk(" L4[0x%03lx] = %"PRIpte" %016lx\n", l4_table_offset(addr), l4e_get_intpte(l4e), pfn); if ( !(l4e_get_flags(l4e) & _PAGE_PRESENT) || !mfn_valid(mfn) ) @@ -211,7 +211,7 @@ void show_page_walk(unsigned long addr) unmap_domain_page(l3t); mfn = l3e_get_mfn(l3e); pfn = mfn_valid(mfn) && machine_to_phys_mapping_valid ? - get_gpfn_from_mfn(mfn_x(mfn)) : INVALID_M2P_ENTRY; + get_pfn_from_mfn(mfn) : INVALID_M2P_ENTRY; printk(" L3[0x%03lx] = %"PRIpte" %016lx%s\n", l3_table_offset(addr), l3e_get_intpte(l3e), pfn, (l3e_get_flags(l3e) & _PAGE_PSE) ? " (PSE)" : ""); @@ -225,7 +225,7 @@ void show_page_walk(unsigned long addr) unmap_domain_page(l2t); mfn = l2e_get_mfn(l2e); pfn = mfn_valid(mfn) && machine_to_phys_mapping_valid ? - get_gpfn_from_mfn(mfn_x(mfn)) : INVALID_M2P_ENTRY; + get_pfn_from_mfn(mfn) : INVALID_M2P_ENTRY; printk(" L2[0x%03lx] = %"PRIpte" %016lx%s\n", l2_table_offset(addr), l2e_get_intpte(l2e), pfn, (l2e_get_flags(l2e) & _PAGE_PSE) ? " (PSE)" : ""); @@ -239,7 +239,7 @@ void show_page_walk(unsigned long addr) unmap_domain_page(l1t); mfn = l1e_get_mfn(l1e); pfn = mfn_valid(mfn) && machine_to_phys_mapping_valid ? - get_gpfn_from_mfn(mfn_x(mfn)) : INVALID_M2P_ENTRY; + get_pfn_from_mfn(mfn) : INVALID_M2P_ENTRY; printk(" L1[0x%03lx] = %"PRIpte" %016lx\n", l1_table_offset(addr), l1e_get_intpte(l1e), pfn); } diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index 41e4fa899d..239aac18dd 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -1430,7 +1430,7 @@ static void free_heap_pages( /* This page is not a guest frame any more. */ page_set_owner(&pg[i], NULL); /* set_gpfn_from_mfn snoops pg owner */ - set_gpfn_from_mfn(mfn_x(mfn) + i, INVALID_M2P_ENTRY); + set_pfn_from_mfn(mfn_add(mfn, i), INVALID_M2P_ENTRY); if ( need_scrub ) { diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h index abf4cc23e4..11614f9107 100644 --- a/xen/include/asm-arm/mm.h +++ b/xen/include/asm-arm/mm.h @@ -319,7 +319,7 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va, #define SHARED_M2P(_e) ((_e) == SHARED_M2P_ENTRY) /* Xen always owns P2M on ARM */ -#define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0) +static inline void set_pfn_from_mfn(mfn_t mfn, unsigned long pfn) {} #define mfn_to_gmfn(_d, mfn) (mfn) diff --git a/xen/include/asm-x86/grant_table.h b/xen/include/asm-x86/grant_table.h index 5871238f6d..b6a09c4c6c 100644 --- a/xen/include/asm-x86/grant_table.h +++ b/xen/include/asm-x86/grant_table.h @@ -41,7 +41,7 @@ static inline int replace_grant_host_mapping(uint64_t addr, mfn_t frame, #define gnttab_get_frame_gfn(gt, st, idx) ({ \ mfn_t mfn_ = (st) ? gnttab_status_mfn(gt, idx) \ : gnttab_shared_mfn(gt, idx); \ - unsigned long gpfn_ = get_gpfn_from_mfn(mfn_x(mfn_)); \ + unsigned long gpfn_ = get_pfn_from_mfn(mfn_); \ VALID_M2P(gpfn_) ? _gfn(gpfn_) : INVALID_GFN; \ }) diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h index 53f2ed7c7d..2a4f42e78f 100644 --- a/xen/include/asm-x86/mm.h +++ b/xen/include/asm-x86/mm.h @@ -500,9 +500,10 @@ extern paddr_t mem_hotplug; */ extern bool machine_to_phys_mapping_valid; -static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn) +static inline void set_pfn_from_mfn(mfn_t mfn_, unsigned long pfn) { - const struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn))); + const unsigned long mfn = mfn_x(mfn_); + const struct domain *d = page_get_owner(mfn_to_page(mfn_)); unsigned long entry = (d && (d == dom_cow)) ? SHARED_M2P_ENTRY : pfn; if ( !machine_to_phys_mapping_valid ) @@ -515,11 +516,14 @@ static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn) extern struct rangeset *mmio_ro_ranges; -#define get_gpfn_from_mfn(mfn) (machine_to_phys_mapping[(mfn)]) +static inline unsigned long get_pfn_from_mfn(mfn_t mfn) +{ + return machine_to_phys_mapping[mfn_x(mfn)]; +} #define mfn_to_gmfn(_d, mfn) \ ( (paging_mode_translate(_d)) \ - ? get_gpfn_from_mfn(mfn) \ + ? get_pfn_from_mfn(_mfn(mfn)) \ : (mfn) ) #define compat_pfn_to_cr3(pfn) (((unsigned)(pfn) << 12) | ((unsigned)(pfn) >> 20)) diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h index a2c6049834..39dae242b0 100644 --- a/xen/include/asm-x86/p2m.h +++ b/xen/include/asm-x86/p2m.h @@ -505,7 +505,7 @@ static inline struct page_info *get_page_from_gfn( static inline gfn_t mfn_to_gfn(const struct domain *d, mfn_t mfn) { if ( paging_mode_translate(d) ) - return _gfn(get_gpfn_from_mfn(mfn_x(mfn))); + return _gfn(get_pfn_from_mfn(mfn)); else return _gfn(mfn_x(mfn)); }