There are some variables in Xen specific coding which names imply them
holding a PFN, while in reality they are containing numbers of PFNs.
Rename them accordingly.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
arch/x86/include/asm/xen/page.h | 4 ++--
arch/x86/xen/p2m.c | 31 ++++++++++++++++---------------
arch/x86/xen/setup.c | 2 +-
3 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/arch/x86/include/asm/xen/page.h b/arch/x86/include/asm/xen/page.h
index 1a162e559753..3590d6bf42c7 100644
--- a/arch/x86/include/asm/xen/page.h
+++ b/arch/x86/include/asm/xen/page.h
@@ -51,7 +51,7 @@ extern unsigned long *machine_to_phys_mapping;
extern unsigned long machine_to_phys_nr;
extern unsigned long *xen_p2m_addr;
extern unsigned long xen_p2m_size;
-extern unsigned long xen_max_p2m_pfn;
+extern unsigned long xen_p2m_max_size;
extern int xen_alloc_p2m_entry(unsigned long pfn);
@@ -144,7 +144,7 @@ static inline unsigned long __pfn_to_mfn(unsigned long pfn)
if (pfn < xen_p2m_size)
mfn = xen_p2m_addr[pfn];
- else if (unlikely(pfn < xen_max_p2m_pfn))
+ else if (unlikely(pfn < xen_p2m_max_size))
return get_phys_to_machine(pfn);
else
return IDENTITY_FRAME(pfn);
diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
index 5e6e236977c7..babdc18dbef7 100644
--- a/arch/x86/xen/p2m.c
+++ b/arch/x86/xen/p2m.c
@@ -95,8 +95,8 @@ unsigned long *xen_p2m_addr __read_mostly;
EXPORT_SYMBOL_GPL(xen_p2m_addr);
unsigned long xen_p2m_size __read_mostly;
EXPORT_SYMBOL_GPL(xen_p2m_size);
-unsigned long xen_max_p2m_pfn __read_mostly;
-EXPORT_SYMBOL_GPL(xen_max_p2m_pfn);
+unsigned long xen_p2m_max_size __read_mostly;
+EXPORT_SYMBOL_GPL(xen_p2m_max_size);
#ifdef CONFIG_XEN_MEMORY_HOTPLUG_LIMIT
#define P2M_LIMIT CONFIG_XEN_MEMORY_HOTPLUG_LIMIT
@@ -121,7 +121,7 @@ static pte_t *p2m_identity_pte;
* can avoid scanning the whole P2M (which may be sized to account for
* hotplugged memory).
*/
-static unsigned long xen_p2m_last_pfn;
+static unsigned long xen_p2m_pfn_limit;
static inline unsigned p2m_top_index(unsigned long pfn)
{
@@ -239,7 +239,7 @@ void __ref xen_build_mfn_list_list(void)
p2m_mid_mfn_init(p2m_mid_missing_mfn, p2m_missing);
}
- for (pfn = 0; pfn < xen_max_p2m_pfn && pfn < MAX_P2M_PFN;
+ for (pfn = 0; pfn < xen_p2m_max_size && pfn < MAX_P2M_PFN;
pfn += P2M_PER_PAGE) {
topidx = p2m_top_index(pfn);
mididx = p2m_mid_index(pfn);
@@ -284,7 +284,7 @@ void xen_setup_mfn_list_list(void)
else
HYPERVISOR_shared_info->arch.pfn_to_mfn_frame_list_list =
virt_to_mfn(p2m_top_mfn);
- HYPERVISOR_shared_info->arch.max_pfn = xen_p2m_last_pfn;
+ HYPERVISOR_shared_info->arch.max_pfn = xen_p2m_pfn_limit;
HYPERVISOR_shared_info->arch.p2m_generation = 0;
HYPERVISOR_shared_info->arch.p2m_vaddr = (unsigned long)xen_p2m_addr;
HYPERVISOR_shared_info->arch.p2m_cr3 =
@@ -302,7 +302,7 @@ void __init xen_build_dynamic_phys_to_machine(void)
for (pfn = xen_start_info->nr_pages; pfn < xen_p2m_size; pfn++)
xen_p2m_addr[pfn] = INVALID_P2M_ENTRY;
- xen_max_p2m_pfn = xen_p2m_size;
+ xen_p2m_max_size = xen_p2m_size;
}
#define P2M_TYPE_IDENTITY 0
@@ -353,7 +353,7 @@ static void __init xen_rebuild_p2m_list(unsigned long *p2m)
pfn_pte(PFN_DOWN(__pa(p2m_identity)), PAGE_KERNEL_RO));
}
- for (pfn = 0; pfn < xen_max_p2m_pfn; pfn += chunk) {
+ for (pfn = 0; pfn < xen_p2m_max_size; pfn += chunk) {
/*
* Try to map missing/identity PMDs or p2m-pages if possible.
* We have to respect the structure of the mfn_list_list
@@ -413,21 +413,22 @@ void __init xen_vmalloc_p2m_tree(void)
static struct vm_struct vm;
unsigned long p2m_limit;
- xen_p2m_last_pfn = xen_max_p2m_pfn;
+ xen_p2m_pfn_limit = xen_p2m_max_size;
p2m_limit = (phys_addr_t)P2M_LIMIT * 1024 * 1024 * 1024 / PAGE_SIZE;
vm.flags = VM_ALLOC;
- vm.size = ALIGN(sizeof(unsigned long) * max(xen_max_p2m_pfn, p2m_limit),
+ vm.size = ALIGN(sizeof(unsigned long) *
+ max(xen_p2m_max_size, p2m_limit),
PMD_SIZE * PMDS_PER_MID_PAGE);
vm_area_register_early(&vm, PMD_SIZE * PMDS_PER_MID_PAGE);
pr_notice("p2m virtual area at %p, size is %lx\n", vm.addr, vm.size);
- xen_max_p2m_pfn = vm.size / sizeof(unsigned long);
+ xen_p2m_max_size = vm.size / sizeof(unsigned long);
xen_rebuild_p2m_list(vm.addr);
xen_p2m_addr = vm.addr;
- xen_p2m_size = xen_max_p2m_pfn;
+ xen_p2m_size = xen_p2m_max_size;
xen_inv_extra_mem();
}
@@ -438,7 +439,7 @@ unsigned long get_phys_to_machine(unsigned long pfn)
unsigned int level;
if (unlikely(pfn >= xen_p2m_size)) {
- if (pfn < xen_max_p2m_pfn)
+ if (pfn < xen_p2m_max_size)
return xen_chk_extra_mem(pfn);
return IDENTITY_FRAME(pfn);
@@ -618,9 +619,9 @@ int xen_alloc_p2m_entry(unsigned long pfn)
}
/* Expanded the p2m? */
- if (pfn >= xen_p2m_last_pfn) {
- xen_p2m_last_pfn = ALIGN(pfn + 1, P2M_PER_PAGE);
- HYPERVISOR_shared_info->arch.max_pfn = xen_p2m_last_pfn;
+ if (pfn >= xen_p2m_pfn_limit) {
+ xen_p2m_pfn_limit = ALIGN(pfn + 1, P2M_PER_PAGE);
+ HYPERVISOR_shared_info->arch.max_pfn = xen_p2m_pfn_limit;
}
return 0;
diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
index 8bfc10330107..1caddd3fa0e1 100644
--- a/arch/x86/xen/setup.c
+++ b/arch/x86/xen/setup.c
@@ -816,7 +816,7 @@ char * __init xen_memory_setup(void)
n_pfns = PFN_DOWN(addr + chunk_size) - pfn_s;
extra_pages -= n_pfns;
xen_add_extra_mem(pfn_s, n_pfns);
- xen_max_p2m_pfn = pfn_s + n_pfns;
+ xen_p2m_max_size = pfn_s + n_pfns;
} else
discard = true;
}
--
2.26.2
On 16.06.2021 09:30, Juergen Gross wrote: > --- a/arch/x86/xen/p2m.c > +++ b/arch/x86/xen/p2m.c > @@ -95,8 +95,8 @@ unsigned long *xen_p2m_addr __read_mostly; > EXPORT_SYMBOL_GPL(xen_p2m_addr); > unsigned long xen_p2m_size __read_mostly; > EXPORT_SYMBOL_GPL(xen_p2m_size); > -unsigned long xen_max_p2m_pfn __read_mostly; > -EXPORT_SYMBOL_GPL(xen_max_p2m_pfn); > +unsigned long xen_p2m_max_size __read_mostly; > +EXPORT_SYMBOL_GPL(xen_p2m_max_size); Instead of renaming the exported variable (which will break consumers anyway), how about dropping the apparently unneeded export at this occasion? Further it looks to me as if xen_p2m_size and this variable were actually always kept in sync, so I'd like to put up the question of dropping one of the two. > @@ -121,7 +121,7 @@ static pte_t *p2m_identity_pte; > * can avoid scanning the whole P2M (which may be sized to account for > * hotplugged memory). > */ > -static unsigned long xen_p2m_last_pfn; > +static unsigned long xen_p2m_pfn_limit; As to the comment remark in patch 1: You don't alter the comment here either, and "limit" still doesn't make clear whether that's an inclusive or exclusive limit. Jan
On 16.06.21 11:56, Jan Beulich wrote: > On 16.06.2021 09:30, Juergen Gross wrote: >> --- a/arch/x86/xen/p2m.c >> +++ b/arch/x86/xen/p2m.c >> @@ -95,8 +95,8 @@ unsigned long *xen_p2m_addr __read_mostly; >> EXPORT_SYMBOL_GPL(xen_p2m_addr); >> unsigned long xen_p2m_size __read_mostly; >> EXPORT_SYMBOL_GPL(xen_p2m_size); >> -unsigned long xen_max_p2m_pfn __read_mostly; >> -EXPORT_SYMBOL_GPL(xen_max_p2m_pfn); >> +unsigned long xen_p2m_max_size __read_mostly; >> +EXPORT_SYMBOL_GPL(xen_p2m_max_size); > > Instead of renaming the exported variable (which will break consumers > anyway), how about dropping the apparently unneeded export at this > occasion? Why do you think it isn't needed? It is being referenced via the inline function __pfn_to_mfn() in arch/x86/include/asm/xen/page.h. And __pfn_to_mfn() is used via lots of other inline functions and macros. > Further it looks to me as if xen_p2m_size and this variable > were actually always kept in sync, so I'd like to put up the question > of dropping one of the two. Hmm, should be possible, yes. > >> @@ -121,7 +121,7 @@ static pte_t *p2m_identity_pte; >> * can avoid scanning the whole P2M (which may be sized to account for >> * hotplugged memory). >> */ >> -static unsigned long xen_p2m_last_pfn; >> +static unsigned long xen_p2m_pfn_limit; > > As to the comment remark in patch 1: You don't alter the comment > here either, and "limit" still doesn't make clear whether that's an > inclusive or exclusive limit. Oh, indeed. Will fix that. Juergen
On 16.06.2021 12:43, Juergen Gross wrote: > On 16.06.21 11:56, Jan Beulich wrote: >> On 16.06.2021 09:30, Juergen Gross wrote: >>> --- a/arch/x86/xen/p2m.c >>> +++ b/arch/x86/xen/p2m.c >>> @@ -95,8 +95,8 @@ unsigned long *xen_p2m_addr __read_mostly; >>> EXPORT_SYMBOL_GPL(xen_p2m_addr); >>> unsigned long xen_p2m_size __read_mostly; >>> EXPORT_SYMBOL_GPL(xen_p2m_size); >>> -unsigned long xen_max_p2m_pfn __read_mostly; >>> -EXPORT_SYMBOL_GPL(xen_max_p2m_pfn); >>> +unsigned long xen_p2m_max_size __read_mostly; >>> +EXPORT_SYMBOL_GPL(xen_p2m_max_size); >> >> Instead of renaming the exported variable (which will break consumers >> anyway), how about dropping the apparently unneeded export at this >> occasion? > > Why do you think it isn't needed? It is being referenced via the inline > function __pfn_to_mfn() in arch/x86/include/asm/xen/page.h. And > __pfn_to_mfn() is used via lots of other inline functions and macros. Oh, sorry. Not working that much with the Linux sources anymore, I didn't pay attention to include/ changes living ahead of *.c ones, and inferred from the last file touched being *.c that no headers were getting changed by the patch. Jan
On 16.06.21 12:43, Juergen Gross wrote: > On 16.06.21 11:56, Jan Beulich wrote: >> On 16.06.2021 09:30, Juergen Gross wrote: >>> --- a/arch/x86/xen/p2m.c >>> +++ b/arch/x86/xen/p2m.c >>> @@ -95,8 +95,8 @@ unsigned long *xen_p2m_addr __read_mostly; >>> EXPORT_SYMBOL_GPL(xen_p2m_addr); >>> unsigned long xen_p2m_size __read_mostly; >>> EXPORT_SYMBOL_GPL(xen_p2m_size); >>> -unsigned long xen_max_p2m_pfn __read_mostly; >>> -EXPORT_SYMBOL_GPL(xen_max_p2m_pfn); >>> +unsigned long xen_p2m_max_size __read_mostly; >>> +EXPORT_SYMBOL_GPL(xen_p2m_max_size); >> >> Instead of renaming the exported variable (which will break consumers >> anyway), how about dropping the apparently unneeded export at this >> occasion? > > Why do you think it isn't needed? It is being referenced via the inline > function __pfn_to_mfn() in arch/x86/include/asm/xen/page.h. And > __pfn_to_mfn() is used via lots of other inline functions and macros. > >> Further it looks to me as if xen_p2m_size and this variable >> were actually always kept in sync, so I'd like to put up the question >> of dropping one of the two. > > Hmm, should be possible, yes. Looking into this it seems this is not possible. xen_p2m_size always holds the number of p2m entries in the p2m table, including invalid ones at the end. xen_p2m_pfn_limit however contains the (rounded up) index after the last valid p2m entry. Juergen
On 30.07.2021 11:00, Juergen Gross wrote: > On 16.06.21 12:43, Juergen Gross wrote: >> On 16.06.21 11:56, Jan Beulich wrote: >>> On 16.06.2021 09:30, Juergen Gross wrote: >>>> --- a/arch/x86/xen/p2m.c >>>> +++ b/arch/x86/xen/p2m.c >>>> @@ -95,8 +95,8 @@ unsigned long *xen_p2m_addr __read_mostly; >>>> EXPORT_SYMBOL_GPL(xen_p2m_addr); >>>> unsigned long xen_p2m_size __read_mostly; >>>> EXPORT_SYMBOL_GPL(xen_p2m_size); >>>> -unsigned long xen_max_p2m_pfn __read_mostly; >>>> -EXPORT_SYMBOL_GPL(xen_max_p2m_pfn); >>>> +unsigned long xen_p2m_max_size __read_mostly; >>>> +EXPORT_SYMBOL_GPL(xen_p2m_max_size); >>> >>> Instead of renaming the exported variable (which will break consumers >>> anyway), how about dropping the apparently unneeded export at this >>> occasion? >> >> Why do you think it isn't needed? It is being referenced via the inline >> function __pfn_to_mfn() in arch/x86/include/asm/xen/page.h. And >> __pfn_to_mfn() is used via lots of other inline functions and macros. >> >>> Further it looks to me as if xen_p2m_size and this variable >>> were actually always kept in sync, so I'd like to put up the question >>> of dropping one of the two. >> >> Hmm, should be possible, yes. > > Looking into this it seems this is not possible. > > xen_p2m_size always holds the number of p2m entries in the p2m table, > including invalid ones at the end. xen_p2m_pfn_limit however contains > the (rounded up) index after the last valid p2m entry. I'm afraid I can't follow: xen_build_dynamic_phys_to_machine() sets xen_p2m_size and then syncs its value to what so far has been xen_max_p2m_pfn. xen_vmalloc_p2m_tree() sets xen_max_p2m_pfn and then syncs its value to xen_p2m_size. I therefore can't see how the two values would hold different values, except for the brief periods between updating one and then the other. Jan
On 03.08.21 12:42, Jan Beulich wrote: > On 30.07.2021 11:00, Juergen Gross wrote: >> On 16.06.21 12:43, Juergen Gross wrote: >>> On 16.06.21 11:56, Jan Beulich wrote: >>>> On 16.06.2021 09:30, Juergen Gross wrote: >>>>> --- a/arch/x86/xen/p2m.c >>>>> +++ b/arch/x86/xen/p2m.c >>>>> @@ -95,8 +95,8 @@ unsigned long *xen_p2m_addr __read_mostly; >>>>> EXPORT_SYMBOL_GPL(xen_p2m_addr); >>>>> unsigned long xen_p2m_size __read_mostly; >>>>> EXPORT_SYMBOL_GPL(xen_p2m_size); >>>>> -unsigned long xen_max_p2m_pfn __read_mostly; >>>>> -EXPORT_SYMBOL_GPL(xen_max_p2m_pfn); >>>>> +unsigned long xen_p2m_max_size __read_mostly; >>>>> +EXPORT_SYMBOL_GPL(xen_p2m_max_size); >>>> >>>> Instead of renaming the exported variable (which will break consumers >>>> anyway), how about dropping the apparently unneeded export at this >>>> occasion? >>> >>> Why do you think it isn't needed? It is being referenced via the inline >>> function __pfn_to_mfn() in arch/x86/include/asm/xen/page.h. And >>> __pfn_to_mfn() is used via lots of other inline functions and macros. >>> >>>> Further it looks to me as if xen_p2m_size and this variable >>>> were actually always kept in sync, so I'd like to put up the question >>>> of dropping one of the two. >>> >>> Hmm, should be possible, yes. >> >> Looking into this it seems this is not possible. >> >> xen_p2m_size always holds the number of p2m entries in the p2m table, >> including invalid ones at the end. xen_p2m_pfn_limit however contains >> the (rounded up) index after the last valid p2m entry. > > I'm afraid I can't follow: > > xen_build_dynamic_phys_to_machine() sets xen_p2m_size and then syncs > its value to what so far has been xen_max_p2m_pfn. > > xen_vmalloc_p2m_tree() sets xen_max_p2m_pfn and then syncs its value > to xen_p2m_size. > > I therefore can't see how the two values would hold different values, > except for the brief periods between updating one and then the other. The brief period in xen_vmalloc_p2m_tree() is the problematic one. The different values are especially important for the calls of __pfn_to_mfn() during xen_rebuild_p2m_list(). Juergen
On 16.08.2021 07:25, Juergen Gross wrote: > On 03.08.21 12:42, Jan Beulich wrote: >> On 30.07.2021 11:00, Juergen Gross wrote: >>> On 16.06.21 12:43, Juergen Gross wrote: >>>> On 16.06.21 11:56, Jan Beulich wrote: >>>>> On 16.06.2021 09:30, Juergen Gross wrote: >>>>>> --- a/arch/x86/xen/p2m.c >>>>>> +++ b/arch/x86/xen/p2m.c >>>>>> @@ -95,8 +95,8 @@ unsigned long *xen_p2m_addr __read_mostly; >>>>>> EXPORT_SYMBOL_GPL(xen_p2m_addr); >>>>>> unsigned long xen_p2m_size __read_mostly; >>>>>> EXPORT_SYMBOL_GPL(xen_p2m_size); >>>>>> -unsigned long xen_max_p2m_pfn __read_mostly; >>>>>> -EXPORT_SYMBOL_GPL(xen_max_p2m_pfn); >>>>>> +unsigned long xen_p2m_max_size __read_mostly; >>>>>> +EXPORT_SYMBOL_GPL(xen_p2m_max_size); >>>>> >>>>> Instead of renaming the exported variable (which will break consumers >>>>> anyway), how about dropping the apparently unneeded export at this >>>>> occasion? >>>> >>>> Why do you think it isn't needed? It is being referenced via the inline >>>> function __pfn_to_mfn() in arch/x86/include/asm/xen/page.h. And >>>> __pfn_to_mfn() is used via lots of other inline functions and macros. >>>> >>>>> Further it looks to me as if xen_p2m_size and this variable >>>>> were actually always kept in sync, so I'd like to put up the question >>>>> of dropping one of the two. >>>> >>>> Hmm, should be possible, yes. >>> >>> Looking into this it seems this is not possible. >>> >>> xen_p2m_size always holds the number of p2m entries in the p2m table, >>> including invalid ones at the end. xen_p2m_pfn_limit however contains >>> the (rounded up) index after the last valid p2m entry. >> >> I'm afraid I can't follow: >> >> xen_build_dynamic_phys_to_machine() sets xen_p2m_size and then syncs >> its value to what so far has been xen_max_p2m_pfn. >> >> xen_vmalloc_p2m_tree() sets xen_max_p2m_pfn and then syncs its value >> to xen_p2m_size. >> >> I therefore can't see how the two values would hold different values, >> except for the brief periods between updating one and then the other. > > The brief period in xen_vmalloc_p2m_tree() is the problematic one. The > different values are especially important for the calls of > __pfn_to_mfn() during xen_rebuild_p2m_list(). I'm still in trouble: Such __pfn_to_mfn() invocations would, afaics, occur only in the context of page directory entry manipulation. Isn't it the case that all valid RAM is below xen_p2m_size, and hence no use of else if (unlikely(pfn < xen_max_p2m_pfn)) return get_phys_to_machine(pfn); would occur during that time window? (We're still way ahead of SMP init, so what other CPUs might do does not look to be of concern here.) Jan
© 2016 - 2026 Red Hat, Inc.