linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] xen: fix max_pfn handling for pv guests
@ 2021-06-16  7:30 Juergen Gross
  2021-06-16  7:30 ` [PATCH 1/2] xen: fix setting of max_pfn in shared_info Juergen Gross
  2021-06-16  7:30 ` [PATCH 2/2] xen: rename wrong named pfn related variables Juergen Gross
  0 siblings, 2 replies; 14+ messages in thread
From: Juergen Gross @ 2021-06-16  7:30 UTC (permalink / raw)
  To: xen-devel, x86, linux-kernel
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
	stable

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/2] xen: fix setting of max_pfn in shared_info
  2021-06-16  7:30 [PATCH 0/2] xen: fix max_pfn handling for pv guests Juergen Gross
@ 2021-06-16  7:30 ` Juergen Gross
  2021-06-16  9:52   ` Jan Beulich
  2021-06-16  7:30 ` [PATCH 2/2] xen: rename wrong named pfn related variables Juergen Gross
  1 sibling, 1 reply; 14+ messages in thread
From: Juergen Gross @ 2021-06-16  7:30 UTC (permalink / raw)
  To: xen-devel, x86, linux-kernel
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
	stable

Xen PV guests are specifying the highest used PFN via the max_pfn
field in shared_info. This value is used by the Xen tools when saving
or migrating the guest.

Unfortunately this field is misnamed, as in reality it is specifying
the number of pages (including any memory holes) of the guest, so it
is the highest used PFN + 1. Renaming isn't possible, as this is a
public Xen hypervisor interface which needs to be kept stable.

The kernel will set the value correctly initially at boot time, but
when adding more pages (e.g. due to memory hotplug or ballooning) a
real PFN number is stored in max_pfn. This is done when expanding the
p2m array, and the PFN stored there is even possibly wrong, as it
should be the last possible PFN of the just added P2M frame, and not
one which led to the P2M expansion.

Fix that by setting shared_info->max_pfn to the last possible PFN + 1.

Fixes: 98dd166ea3a3c3 ("x86/xen/p2m: hint at the last populated P2M entry")
Cc: stable@vger.kernel.org
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/xen/p2m.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
index ac06ca32e9ef..5e6e236977c7 100644
--- a/arch/x86/xen/p2m.c
+++ b/arch/x86/xen/p2m.c
@@ -618,8 +618,8 @@ int xen_alloc_p2m_entry(unsigned long pfn)
 	}
 
 	/* Expanded the p2m? */
-	if (pfn > xen_p2m_last_pfn) {
-		xen_p2m_last_pfn = pfn;
+	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;
 	}
 
-- 
2.26.2


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 2/2] xen: rename wrong named pfn related variables
  2021-06-16  7:30 [PATCH 0/2] xen: fix max_pfn handling for pv guests Juergen Gross
  2021-06-16  7:30 ` [PATCH 1/2] xen: fix setting of max_pfn in shared_info Juergen Gross
@ 2021-06-16  7:30 ` Juergen Gross
  2021-06-16  9:56   ` Jan Beulich
  1 sibling, 1 reply; 14+ messages in thread
From: Juergen Gross @ 2021-06-16  7:30 UTC (permalink / raw)
  To: xen-devel, x86, linux-kernel
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] xen: fix setting of max_pfn in shared_info
  2021-06-16  7:30 ` [PATCH 1/2] xen: fix setting of max_pfn in shared_info Juergen Gross
@ 2021-06-16  9:52   ` Jan Beulich
  2021-06-16 10:37     ` Juergen Gross
  0 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2021-06-16  9:52 UTC (permalink / raw)
  To: Juergen Gross
  Cc: Boris Ostrovsky, Stefano Stabellini, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, H. Peter Anvin, stable, xen-devel,
	linux-kernel, x86

On 16.06.2021 09:30, Juergen Gross wrote:
> Xen PV guests are specifying the highest used PFN via the max_pfn
> field in shared_info. This value is used by the Xen tools when saving
> or migrating the guest.
> 
> Unfortunately this field is misnamed, as in reality it is specifying
> the number of pages (including any memory holes) of the guest, so it
> is the highest used PFN + 1. Renaming isn't possible, as this is a
> public Xen hypervisor interface which needs to be kept stable.
> 
> The kernel will set the value correctly initially at boot time, but
> when adding more pages (e.g. due to memory hotplug or ballooning) a
> real PFN number is stored in max_pfn. This is done when expanding the
> p2m array, and the PFN stored there is even possibly wrong, as it
> should be the last possible PFN of the just added P2M frame, and not
> one which led to the P2M expansion.
> 
> Fix that by setting shared_info->max_pfn to the last possible PFN + 1.
> 
> Fixes: 98dd166ea3a3c3 ("x86/xen/p2m: hint at the last populated P2M entry")
> Cc: stable@vger.kernel.org
> Signed-off-by: Juergen Gross <jgross@suse.com>

The code change is fine, so
Reviewed-by: Jan Beulich <jbeulich@suse.com>

But I think even before the rename you would want to clarify the comment
next to the variable's definition, to make clear what it really holds.

Jan


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] xen: rename wrong named pfn related variables
  2021-06-16  7:30 ` [PATCH 2/2] xen: rename wrong named pfn related variables Juergen Gross
@ 2021-06-16  9:56   ` Jan Beulich
  2021-06-16 10:43     ` Juergen Gross
  0 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2021-06-16  9:56 UTC (permalink / raw)
  To: Juergen Gross
  Cc: Boris Ostrovsky, Stefano Stabellini, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, H. Peter Anvin, xen-devel,
	linux-kernel, x86

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] xen: fix setting of max_pfn in shared_info
  2021-06-16  9:52   ` Jan Beulich
@ 2021-06-16 10:37     ` Juergen Gross
  2021-06-16 10:56       ` Jan Beulich
  0 siblings, 1 reply; 14+ messages in thread
From: Juergen Gross @ 2021-06-16 10:37 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Boris Ostrovsky, Stefano Stabellini, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, H. Peter Anvin, stable, xen-devel,
	linux-kernel, x86


[-- Attachment #1.1.1: Type: text/plain, Size: 1590 bytes --]

On 16.06.21 11:52, Jan Beulich wrote:
> On 16.06.2021 09:30, Juergen Gross wrote:
>> Xen PV guests are specifying the highest used PFN via the max_pfn
>> field in shared_info. This value is used by the Xen tools when saving
>> or migrating the guest.
>>
>> Unfortunately this field is misnamed, as in reality it is specifying
>> the number of pages (including any memory holes) of the guest, so it
>> is the highest used PFN + 1. Renaming isn't possible, as this is a
>> public Xen hypervisor interface which needs to be kept stable.
>>
>> The kernel will set the value correctly initially at boot time, but
>> when adding more pages (e.g. due to memory hotplug or ballooning) a
>> real PFN number is stored in max_pfn. This is done when expanding the
>> p2m array, and the PFN stored there is even possibly wrong, as it
>> should be the last possible PFN of the just added P2M frame, and not
>> one which led to the P2M expansion.
>>
>> Fix that by setting shared_info->max_pfn to the last possible PFN + 1.
>>
>> Fixes: 98dd166ea3a3c3 ("x86/xen/p2m: hint at the last populated P2M entry")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> The code change is fine, so
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> But I think even before the rename you would want to clarify the comment
> next to the variable's definition, to make clear what it really holds.

It already says: "Number of valid entries in the p2m table(s) ..."
What do you think is unclear about that? Or do you mean another
variable?


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] xen: rename wrong named pfn related variables
  2021-06-16  9:56   ` Jan Beulich
@ 2021-06-16 10:43     ` Juergen Gross
  2021-06-16 11:01       ` Jan Beulich
  2021-07-30  9:00       ` Juergen Gross
  0 siblings, 2 replies; 14+ messages in thread
From: Juergen Gross @ 2021-06-16 10:43 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Boris Ostrovsky, Stefano Stabellini, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, H. Peter Anvin, xen-devel,
	linux-kernel, x86


[-- Attachment #1.1.1: Type: text/plain, Size: 1576 bytes --]

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

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] xen: fix setting of max_pfn in shared_info
  2021-06-16 10:37     ` Juergen Gross
@ 2021-06-16 10:56       ` Jan Beulich
  2021-06-16 11:18         ` Juergen Gross
  0 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2021-06-16 10:56 UTC (permalink / raw)
  To: Juergen Gross
  Cc: Boris Ostrovsky, Stefano Stabellini, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, H. Peter Anvin, stable, xen-devel,
	linux-kernel, x86

On 16.06.2021 12:37, Juergen Gross wrote:
> On 16.06.21 11:52, Jan Beulich wrote:
>> On 16.06.2021 09:30, Juergen Gross wrote:
>>> Xen PV guests are specifying the highest used PFN via the max_pfn
>>> field in shared_info. This value is used by the Xen tools when saving
>>> or migrating the guest.
>>>
>>> Unfortunately this field is misnamed, as in reality it is specifying
>>> the number of pages (including any memory holes) of the guest, so it
>>> is the highest used PFN + 1. Renaming isn't possible, as this is a
>>> public Xen hypervisor interface which needs to be kept stable.
>>>
>>> The kernel will set the value correctly initially at boot time, but
>>> when adding more pages (e.g. due to memory hotplug or ballooning) a
>>> real PFN number is stored in max_pfn. This is done when expanding the
>>> p2m array, and the PFN stored there is even possibly wrong, as it
>>> should be the last possible PFN of the just added P2M frame, and not
>>> one which led to the P2M expansion.
>>>
>>> Fix that by setting shared_info->max_pfn to the last possible PFN + 1.
>>>
>>> Fixes: 98dd166ea3a3c3 ("x86/xen/p2m: hint at the last populated P2M entry")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>
>> The code change is fine, so
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>> But I think even before the rename you would want to clarify the comment
>> next to the variable's definition, to make clear what it really holds.
> 
> It already says: "Number of valid entries in the p2m table(s) ..."
> What do you think is unclear about that? Or do you mean another
> variable?

I mean the variable the value of which the patch corrects, i.e.
xen_p2m_last_pfn. What I see in current source is

/*
 * Hint at last populated PFN.
 *
 * Used to set HYPERVISOR_shared_info->arch.max_pfn so the toolstack
 * can avoid scanning the whole P2M (which may be sized to account for
 * hotplugged memory).
 */
static unsigned long xen_p2m_last_pfn;

Jan


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] xen: rename wrong named pfn related variables
  2021-06-16 10:43     ` Juergen Gross
@ 2021-06-16 11:01       ` Jan Beulich
  2021-07-30  9:00       ` Juergen Gross
  1 sibling, 0 replies; 14+ messages in thread
From: Jan Beulich @ 2021-06-16 11:01 UTC (permalink / raw)
  To: Juergen Gross
  Cc: Boris Ostrovsky, Stefano Stabellini, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, H. Peter Anvin, xen-devel,
	linux-kernel, x86

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] xen: fix setting of max_pfn in shared_info
  2021-06-16 10:56       ` Jan Beulich
@ 2021-06-16 11:18         ` Juergen Gross
  0 siblings, 0 replies; 14+ messages in thread
From: Juergen Gross @ 2021-06-16 11:18 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Boris Ostrovsky, Stefano Stabellini, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, H. Peter Anvin, stable, xen-devel,
	linux-kernel, x86


[-- Attachment #1.1.1: Type: text/plain, Size: 2381 bytes --]

On 16.06.21 12:56, Jan Beulich wrote:
> On 16.06.2021 12:37, Juergen Gross wrote:
>> On 16.06.21 11:52, Jan Beulich wrote:
>>> On 16.06.2021 09:30, Juergen Gross wrote:
>>>> Xen PV guests are specifying the highest used PFN via the max_pfn
>>>> field in shared_info. This value is used by the Xen tools when saving
>>>> or migrating the guest.
>>>>
>>>> Unfortunately this field is misnamed, as in reality it is specifying
>>>> the number of pages (including any memory holes) of the guest, so it
>>>> is the highest used PFN + 1. Renaming isn't possible, as this is a
>>>> public Xen hypervisor interface which needs to be kept stable.
>>>>
>>>> The kernel will set the value correctly initially at boot time, but
>>>> when adding more pages (e.g. due to memory hotplug or ballooning) a
>>>> real PFN number is stored in max_pfn. This is done when expanding the
>>>> p2m array, and the PFN stored there is even possibly wrong, as it
>>>> should be the last possible PFN of the just added P2M frame, and not
>>>> one which led to the P2M expansion.
>>>>
>>>> Fix that by setting shared_info->max_pfn to the last possible PFN + 1.
>>>>
>>>> Fixes: 98dd166ea3a3c3 ("x86/xen/p2m: hint at the last populated P2M entry")
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>
>>> The code change is fine, so
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> But I think even before the rename you would want to clarify the comment
>>> next to the variable's definition, to make clear what it really holds.
>>
>> It already says: "Number of valid entries in the p2m table(s) ..."
>> What do you think is unclear about that? Or do you mean another
>> variable?
> 
> I mean the variable the value of which the patch corrects, i.e.
> xen_p2m_last_pfn. What I see in current source is
> 
> /*
>   * Hint at last populated PFN.
>   *
>   * Used to set HYPERVISOR_shared_info->arch.max_pfn so the toolstack
>   * can avoid scanning the whole P2M (which may be sized to account for
>   * hotplugged memory).
>   */
> static unsigned long xen_p2m_last_pfn;

Ah, okay.

I think only changing the comment without renaming the variable isn't
the way to go.

In order to keep the to be backported patch small I'd rather do the
comment adjustment and variable renaming in the followup patch.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] xen: rename wrong named pfn related variables
  2021-06-16 10:43     ` Juergen Gross
  2021-06-16 11:01       ` Jan Beulich
@ 2021-07-30  9:00       ` Juergen Gross
  2021-08-03 10:42         ` Jan Beulich
  1 sibling, 1 reply; 14+ messages in thread
From: Juergen Gross @ 2021-07-30  9:00 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Boris Ostrovsky, Stefano Stabellini, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, H. Peter Anvin, xen-devel,
	linux-kernel, x86


[-- Attachment #1.1.1: Type: text/plain, Size: 1432 bytes --]

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

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] xen: rename wrong named pfn related variables
  2021-07-30  9:00       ` Juergen Gross
@ 2021-08-03 10:42         ` Jan Beulich
  2021-08-16  5:25           ` Juergen Gross
  0 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2021-08-03 10:42 UTC (permalink / raw)
  To: Juergen Gross
  Cc: Boris Ostrovsky, Stefano Stabellini, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, H. Peter Anvin, xen-devel,
	linux-kernel, x86

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] xen: rename wrong named pfn related variables
  2021-08-03 10:42         ` Jan Beulich
@ 2021-08-16  5:25           ` Juergen Gross
  2021-08-16 12:57             ` Jan Beulich
  0 siblings, 1 reply; 14+ messages in thread
From: Juergen Gross @ 2021-08-16  5:25 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Boris Ostrovsky, Stefano Stabellini, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, H. Peter Anvin, xen-devel,
	linux-kernel, x86


[-- Attachment #1.1.1: Type: text/plain, Size: 2178 bytes --]

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

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] xen: rename wrong named pfn related variables
  2021-08-16  5:25           ` Juergen Gross
@ 2021-08-16 12:57             ` Jan Beulich
  0 siblings, 0 replies; 14+ messages in thread
From: Jan Beulich @ 2021-08-16 12:57 UTC (permalink / raw)
  To: Juergen Gross
  Cc: Boris Ostrovsky, Stefano Stabellini, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, H. Peter Anvin, xen-devel,
	linux-kernel, x86

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2021-08-16 12:58 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-16  7:30 [PATCH 0/2] xen: fix max_pfn handling for pv guests Juergen Gross
2021-06-16  7:30 ` [PATCH 1/2] xen: fix setting of max_pfn in shared_info Juergen Gross
2021-06-16  9:52   ` Jan Beulich
2021-06-16 10:37     ` Juergen Gross
2021-06-16 10:56       ` Jan Beulich
2021-06-16 11:18         ` Juergen Gross
2021-06-16  7:30 ` [PATCH 2/2] xen: rename wrong named pfn related variables Juergen Gross
2021-06-16  9:56   ` Jan Beulich
2021-06-16 10:43     ` Juergen Gross
2021-06-16 11:01       ` Jan Beulich
2021-07-30  9:00       ` Juergen Gross
2021-08-03 10:42         ` Jan Beulich
2021-08-16  5:25           ` Juergen Gross
2021-08-16 12:57             ` Jan Beulich

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).