Fix some bad naming and setting of max_pfn related variables.
Juergen Gross (2):
xen: fix setting of max_pfn in shared_info
xen: rename wrong named pfn related variables
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(-)
--
2.26.2
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 <[email protected]>
---
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