diff mbox series

[2/2] xen: rename wrong named pfn related variables

Message ID 20210616073007.5215-3-jgross@suse.com (mailing list archive)
State New
Headers show
Series xen: fix max_pfn handling for pv guests | expand

Commit Message

Juergen Gross June 16, 2021, 7:30 a.m. UTC
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(-)

Comments

Jan Beulich June 16, 2021, 9:56 a.m. UTC | #1
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
Juergen Gross June 16, 2021, 10:43 a.m. UTC | #2
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
Jan Beulich June 16, 2021, 11:01 a.m. UTC | #3
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
diff mbox series

Patch

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;
 		}