| Message ID | 20210616073007.5215-3-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: fix max_pfn handling for pv guests | expand |
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
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; }
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(-)