[PATCH 0/2] xen: fix regressions regarding memory hotplug in pv domains

[PATCH 0/2] xen: fix regressions regarding memory hotplug in pv domains

[Juergen Gross]

Fix some regressions introduced in 3.16 and 3.19.

Juergen Gross (2):
  xen: prepare p2m list for memory hotplug
  xen: before ballooning hotplugged memory, set frames to invalid

 arch/x86/xen/p2m.c    | 13 ++++++++++++-
 drivers/xen/Kconfig   | 13 +++++++++++++
 drivers/xen/balloon.c | 13 +++++++++++--
 3 files changed, 36 insertions(+), 3 deletions(-)

-- 
2.1.4

[PATCH 1/2] xen: prepare p2m list for memory hotplug

[Juergen Gross]

Commit 054954eb051f35e74b75a566a96fe756015352c8 ("xen: switch to linear
virtual mapped sparse p2m list") introduced a regression regarding to
memory hotplug for a pv-domain: as the virtual space for the p2m list
is allocated for the to be expected memory size of the domain only,
hotplugged memory above that size will not be usable by the domain.

Correct this by using a configurable size for the p2m list in case of
memory hotplug enabled (default supported memory size is 512 GB for
64 bit domains and 4 GB for 32 bit domains).

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/xen/p2m.c  | 13 ++++++++++++-
 drivers/xen/Kconfig | 13 +++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
index 9f93af5..30e84ae 100644
--- a/arch/x86/xen/p2m.c
+++ b/arch/x86/xen/p2m.c
@@ -91,6 +91,17 @@ EXPORT_SYMBOL_GPL(xen_p2m_size);
 unsigned long xen_max_p2m_pfn __read_mostly;
 EXPORT_SYMBOL_GPL(xen_max_p2m_pfn);
 
+#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
+#ifdef CONFIG_X86_32
+BUILD_BUG_ON_MSG(CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT > 64)
+#endif
+#define P2M_LIMIT max(xen_max_p2m_pfn,					\
+	((unsigned long)((u64)CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT *	\
+	1024 * 1024 * 1024 / PAGE_SIZE)))
+#else
+#define P2M_LIMIT xen_max_p2m_pfn
+#endif
+
 static DEFINE_SPINLOCK(p2m_update_lock);
 
 static unsigned long *p2m_mid_missing_mfn;
@@ -387,7 +398,7 @@ void __init xen_vmalloc_p2m_tree(void)
 	static struct vm_struct vm;
 
 	vm.flags = VM_ALLOC;
-	vm.size = ALIGN(sizeof(unsigned long) * xen_max_p2m_pfn,
+	vm.size = ALIGN(sizeof(unsigned long) * 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);
diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index b812462..0a61ddf 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -55,6 +55,19 @@ config XEN_BALLOON_MEMORY_HOTPLUG
 
 	  In that case step 3 should be omitted.
 
+config XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
+	int
+	default 512 if X86_64
+	default 4 if X86_32
+	depends on XEN_HAVE_PVMMU
+	depends on XEN_BALLOON_MEMORY_HOTPLUG
+	help
+	  Upper limit in GBs a pv domain can be expanded to using memory
+	  hotplug.
+
+	  This value is used to allocate enough space in internal tables needed
+	  for physical memory administration.
+
 config XEN_SCRUB_PAGES
 	bool "Scrub pages before returning them to system"
 	depends on XEN_BALLOON
-- 
2.1.4

Re: [PATCH 1/2] xen: prepare p2m list for memory hotplug

[Boris Ostrovsky]

On 03/19/2015 10:31 AM, Juergen Gross wrote:
> Commit 054954eb051f35e74b75a566a96fe756015352c8 ("xen: switch to linear
> virtual mapped sparse p2m list") introduced a regression regarding to
> memory hotplug for a pv-domain: as the virtual space for the p2m list
> is allocated for the to be expected memory size of the domain only,
> hotplugged memory above that size will not be usable by the domain.
>
> Correct this by using a configurable size for the p2m list in case of
> memory hotplug enabled (default supported memory size is 512 GB for
> 64 bit domains and 4 GB for 32 bit domains).
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>   arch/x86/xen/p2m.c  | 13 ++++++++++++-
>   drivers/xen/Kconfig | 13 +++++++++++++
>   2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
> index 9f93af5..30e84ae 100644
> --- a/arch/x86/xen/p2m.c
> +++ b/arch/x86/xen/p2m.c
> @@ -91,6 +91,17 @@ EXPORT_SYMBOL_GPL(xen_p2m_size);
>   unsigned long xen_max_p2m_pfn __read_mostly;
>   EXPORT_SYMBOL_GPL(xen_max_p2m_pfn);
>   
> +#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
> +#ifdef CONFIG_X86_32
> +BUILD_BUG_ON_MSG(CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT > 64)

Will this work in Kconfig instead?

     range 0 64 if CONFIG_X86_32

-boris

> +#endif
> +#define P2M_LIMIT max(xen_max_p2m_pfn,					\
> +	((unsigned long)((u64)CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT *	\
> +	1024 * 1024 * 1024 / PAGE_SIZE)))
> +#else
> +#define P2M_LIMIT xen_max_p2m_pfn
> +#endif
> +
>   static DEFINE_SPINLOCK(p2m_update_lock);
>   
>   static unsigned long *p2m_mid_missing_mfn;
> @@ -387,7 +398,7 @@ void __init xen_vmalloc_p2m_tree(void)
>   	static struct vm_struct vm;
>   
>   	vm.flags = VM_ALLOC;
> -	vm.size = ALIGN(sizeof(unsigned long) * xen_max_p2m_pfn,
> +	vm.size = ALIGN(sizeof(unsigned long) * 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);
> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index b812462..0a61ddf 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -55,6 +55,19 @@ config XEN_BALLOON_MEMORY_HOTPLUG
>   
>   	  In that case step 3 should be omitted.
>   
> +config XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
> +	int
> +	default 512 if X86_64
> +	default 4 if X86_32
> +	depends on XEN_HAVE_PVMMU
> +	depends on XEN_BALLOON_MEMORY_HOTPLUG
> +	help
> +	  Upper limit in GBs a pv domain can be expanded to using memory
> +	  hotplug.
> +
> +	  This value is used to allocate enough space in internal tables needed
> +	  for physical memory administration.
> +
>   config XEN_SCRUB_PAGES
>   	bool "Scrub pages before returning them to system"
>   	depends on XEN_BALLOON

Re: [PATCH 1/2] xen: prepare p2m list for memory hotplug

[Juergen Gross]

On 03/19/2015 04:53 PM, Boris Ostrovsky wrote:
> On 03/19/2015 10:31 AM, Juergen Gross wrote:
>> Commit 054954eb051f35e74b75a566a96fe756015352c8 ("xen: switch to linear
>> virtual mapped sparse p2m list") introduced a regression regarding to
>> memory hotplug for a pv-domain: as the virtual space for the p2m list
>> is allocated for the to be expected memory size of the domain only,
>> hotplugged memory above that size will not be usable by the domain.
>>
>> Correct this by using a configurable size for the p2m list in case of
>> memory hotplug enabled (default supported memory size is 512 GB for
>> 64 bit domains and 4 GB for 32 bit domains).
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   arch/x86/xen/p2m.c  | 13 ++++++++++++-
>>   drivers/xen/Kconfig | 13 +++++++++++++
>>   2 files changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
>> index 9f93af5..30e84ae 100644
>> --- a/arch/x86/xen/p2m.c
>> +++ b/arch/x86/xen/p2m.c
>> @@ -91,6 +91,17 @@ EXPORT_SYMBOL_GPL(xen_p2m_size);
>>   unsigned long xen_max_p2m_pfn __read_mostly;
>>   EXPORT_SYMBOL_GPL(xen_max_p2m_pfn);
>> +#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
>> +#ifdef CONFIG_X86_32
>> +BUILD_BUG_ON_MSG(CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT > 64)
>
> Will this work in Kconfig instead?
>
>      range 0 64 if CONFIG_X86_32

I'll give it a try.

Juergen

>
> -boris
>
>> +#endif
>> +#define P2M_LIMIT max(xen_max_p2m_pfn,                    \
>> +    ((unsigned long)((u64)CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT *    \
>> +    1024 * 1024 * 1024 / PAGE_SIZE)))
>> +#else
>> +#define P2M_LIMIT xen_max_p2m_pfn
>> +#endif
>> +
>>   static DEFINE_SPINLOCK(p2m_update_lock);
>>   static unsigned long *p2m_mid_missing_mfn;
>> @@ -387,7 +398,7 @@ void __init xen_vmalloc_p2m_tree(void)
>>       static struct vm_struct vm;
>>       vm.flags = VM_ALLOC;
>> -    vm.size = ALIGN(sizeof(unsigned long) * xen_max_p2m_pfn,
>> +    vm.size = ALIGN(sizeof(unsigned long) * 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);
>> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
>> index b812462..0a61ddf 100644
>> --- a/drivers/xen/Kconfig
>> +++ b/drivers/xen/Kconfig
>> @@ -55,6 +55,19 @@ config XEN_BALLOON_MEMORY_HOTPLUG
>>         In that case step 3 should be omitted.
>> +config XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
>> +    int
>> +    default 512 if X86_64
>> +    default 4 if X86_32
>> +    depends on XEN_HAVE_PVMMU
>> +    depends on XEN_BALLOON_MEMORY_HOTPLUG
>> +    help
>> +      Upper limit in GBs a pv domain can be expanded to using memory
>> +      hotplug.
>> +
>> +      This value is used to allocate enough space in internal tables
>> needed
>> +      for physical memory administration.
>> +
>>   config XEN_SCRUB_PAGES
>>       bool "Scrub pages before returning them to system"
>>       depends on XEN_BALLOON
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
>

Re: [PATCH 1/2] xen: prepare p2m list for memory hotplug

[Daniel Kiper]

On Thu, Mar 19, 2015 at 03:31:01PM +0100, Juergen Gross wrote:
> Commit 054954eb051f35e74b75a566a96fe756015352c8 ("xen: switch to linear
> virtual mapped sparse p2m list") introduced a regression regarding to
> memory hotplug for a pv-domain: as the virtual space for the p2m list
> is allocated for the to be expected memory size of the domain only,
> hotplugged memory above that size will not be usable by the domain.
>
> Correct this by using a configurable size for the p2m list in case of
> memory hotplug enabled (default supported memory size is 512 GB for
> 64 bit domains and 4 GB for 32 bit domains).
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  arch/x86/xen/p2m.c  | 13 ++++++++++++-
>  drivers/xen/Kconfig | 13 +++++++++++++
>  2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
> index 9f93af5..30e84ae 100644
> --- a/arch/x86/xen/p2m.c
> +++ b/arch/x86/xen/p2m.c
> @@ -91,6 +91,17 @@ EXPORT_SYMBOL_GPL(xen_p2m_size);
>  unsigned long xen_max_p2m_pfn __read_mostly;
>  EXPORT_SYMBOL_GPL(xen_max_p2m_pfn);
>
> +#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
> +#ifdef CONFIG_X86_32
> +BUILD_BUG_ON_MSG(CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT > 64)
> +#endif
> +#define P2M_LIMIT max(xen_max_p2m_pfn,					\
> +	((unsigned long)((u64)CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT *	\
> +	1024 * 1024 * 1024 / PAGE_SIZE)))
> +#else
> +#define P2M_LIMIT xen_max_p2m_pfn
> +#endif
> +
>  static DEFINE_SPINLOCK(p2m_update_lock);
>
>  static unsigned long *p2m_mid_missing_mfn;
> @@ -387,7 +398,7 @@ void __init xen_vmalloc_p2m_tree(void)
>  	static struct vm_struct vm;
>
>  	vm.flags = VM_ALLOC;
> -	vm.size = ALIGN(sizeof(unsigned long) * xen_max_p2m_pfn,
> +	vm.size = ALIGN(sizeof(unsigned long) * P2M_LIMIT,
>  			PMD_SIZE * PMDS_PER_MID_PAGE);

What happens when somebody will allocate more memory for guest than
XEN_BALLOON_MEMORY_HOTPLUG_LIMIT? Additionally, I think that we do
not need allocate extra virtual address space if memory hotplug is
disabled.

>  	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);
> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index b812462..0a61ddf 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -55,6 +55,19 @@ config XEN_BALLOON_MEMORY_HOTPLUG
>
>  	  In that case step 3 should be omitted.
>
> +config XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
> +	int
> +	default 512 if X86_64
> +	default 4 if X86_32
> +	depends on XEN_HAVE_PVMMU

Do we need dependency on XEN_HAVE_PVMMU?
If yes than maybe "depends on XEN_HAVE_PVMMU && XEN_BALLOON_MEMORY_HOTPLUG".

> +	depends on XEN_BALLOON_MEMORY_HOTPLUG

Should not this option be available even when memory hotplug is disabled?

Daniel

Re: [PATCH 1/2] xen: prepare p2m list for memory hotplug

[Juergen Gross]

On 03/19/2015 05:14 PM, Daniel Kiper wrote:
> On Thu, Mar 19, 2015 at 03:31:01PM +0100, Juergen Gross wrote:
>> Commit 054954eb051f35e74b75a566a96fe756015352c8 ("xen: switch to linear
>> virtual mapped sparse p2m list") introduced a regression regarding to
>> memory hotplug for a pv-domain: as the virtual space for the p2m list
>> is allocated for the to be expected memory size of the domain only,
>> hotplugged memory above that size will not be usable by the domain.
>>
>> Correct this by using a configurable size for the p2m list in case of
>> memory hotplug enabled (default supported memory size is 512 GB for
>> 64 bit domains and 4 GB for 32 bit domains).
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   arch/x86/xen/p2m.c  | 13 ++++++++++++-
>>   drivers/xen/Kconfig | 13 +++++++++++++
>>   2 files changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
>> index 9f93af5..30e84ae 100644
>> --- a/arch/x86/xen/p2m.c
>> +++ b/arch/x86/xen/p2m.c
>> @@ -91,6 +91,17 @@ EXPORT_SYMBOL_GPL(xen_p2m_size);
>>   unsigned long xen_max_p2m_pfn __read_mostly;
>>   EXPORT_SYMBOL_GPL(xen_max_p2m_pfn);
>>
>> +#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
>> +#ifdef CONFIG_X86_32
>> +BUILD_BUG_ON_MSG(CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT > 64)
>> +#endif
>> +#define P2M_LIMIT max(xen_max_p2m_pfn,					\
>> +	((unsigned long)((u64)CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT *	\
>> +	1024 * 1024 * 1024 / PAGE_SIZE)))
>> +#else
>> +#define P2M_LIMIT xen_max_p2m_pfn
>> +#endif
>> +
>>   static DEFINE_SPINLOCK(p2m_update_lock);
>>
>>   static unsigned long *p2m_mid_missing_mfn;
>> @@ -387,7 +398,7 @@ void __init xen_vmalloc_p2m_tree(void)
>>   	static struct vm_struct vm;
>>
>>   	vm.flags = VM_ALLOC;
>> -	vm.size = ALIGN(sizeof(unsigned long) * xen_max_p2m_pfn,
>> +	vm.size = ALIGN(sizeof(unsigned long) * P2M_LIMIT,
>>   			PMD_SIZE * PMDS_PER_MID_PAGE);
>
> What happens when somebody will allocate more memory for guest than
> XEN_BALLOON_MEMORY_HOTPLUG_LIMIT? Additionally, I think that we do

It will fail to add like it does without this patch.

> not need allocate extra virtual address space if memory hotplug is
> disabled.

As XEN_BALLOON_MEMORY_HOTPLUG_LIMIT depends on
XEN_BALLOON_MEMORY_HOTPLUG which in turn depends on MEMORY_HOTPLUG,
this is already the case.

>
>>   	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);
>> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
>> index b812462..0a61ddf 100644
>> --- a/drivers/xen/Kconfig
>> +++ b/drivers/xen/Kconfig
>> @@ -55,6 +55,19 @@ config XEN_BALLOON_MEMORY_HOTPLUG
>>
>>   	  In that case step 3 should be omitted.
>>
>> +config XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
>> +	int
>> +	default 512 if X86_64
>> +	default 4 if X86_32
>> +	depends on XEN_HAVE_PVMMU
>
> Do we need dependency on XEN_HAVE_PVMMU?

Yes. Needed only for pv domains.

> If yes than maybe "depends on XEN_HAVE_PVMMU && XEN_BALLOON_MEMORY_HOTPLUG".

Matter of taste, I guess.

>
>> +	depends on XEN_BALLOON_MEMORY_HOTPLUG
>
> Should not this option be available even when memory hotplug is disabled?

Why?


Juergen

Re: [Xen-devel] [PATCH 1/2] xen: prepare p2m list for memory hotplug

[David Vrabel]

On 19/03/15 14:31, Juergen Gross wrote:
> Commit 054954eb051f35e74b75a566a96fe756015352c8 ("xen: switch to linear
> virtual mapped sparse p2m list") introduced a regression regarding to
> memory hotplug for a pv-domain: as the virtual space for the p2m list
> is allocated for the to be expected memory size of the domain only,
> hotplugged memory above that size will not be usable by the domain.
> 
> Correct this by using a configurable size for the p2m list in case of
> memory hotplug enabled (default supported memory size is 512 GB for
> 64 bit domains and 4 GB for 32 bit domains).
[...]
> --- a/arch/x86/xen/p2m.c
> +++ b/arch/x86/xen/p2m.c
> @@ -91,6 +91,17 @@ EXPORT_SYMBOL_GPL(xen_p2m_size);
>  unsigned long xen_max_p2m_pfn __read_mostly;
>  EXPORT_SYMBOL_GPL(xen_max_p2m_pfn);
>  
> +#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
> +#ifdef CONFIG_X86_32
> +BUILD_BUG_ON_MSG(CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT > 64)
> +#endif
> +#define P2M_LIMIT max(xen_max_p2m_pfn,					\
> +	((unsigned long)((u64)CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT *	\
> +	1024 * 1024 * 1024 / PAGE_SIZE)))
> +#else
> +#define P2M_LIMIT xen_max_p2m_pfn
> +#endif

Can you arrange the #ifdef's to set xen_max_p2m_pfn to the right value
instead of introducing P2M_LIMIT?

David

Re: [Xen-devel] [PATCH 1/2] xen: prepare p2m list for memory hotplug

[Juergen Gross]

On 03/19/2015 05:18 PM, David Vrabel wrote:
> On 19/03/15 14:31, Juergen Gross wrote:
>> Commit 054954eb051f35e74b75a566a96fe756015352c8 ("xen: switch to linear
>> virtual mapped sparse p2m list") introduced a regression regarding to
>> memory hotplug for a pv-domain: as the virtual space for the p2m list
>> is allocated for the to be expected memory size of the domain only,
>> hotplugged memory above that size will not be usable by the domain.
>>
>> Correct this by using a configurable size for the p2m list in case of
>> memory hotplug enabled (default supported memory size is 512 GB for
>> 64 bit domains and 4 GB for 32 bit domains).
> [...]
>> --- a/arch/x86/xen/p2m.c
>> +++ b/arch/x86/xen/p2m.c
>> @@ -91,6 +91,17 @@ EXPORT_SYMBOL_GPL(xen_p2m_size);
>>   unsigned long xen_max_p2m_pfn __read_mostly;
>>   EXPORT_SYMBOL_GPL(xen_max_p2m_pfn);
>>
>> +#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
>> +#ifdef CONFIG_X86_32
>> +BUILD_BUG_ON_MSG(CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT > 64)
>> +#endif
>> +#define P2M_LIMIT max(xen_max_p2m_pfn,					\
>> +	((unsigned long)((u64)CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT *	\
>> +	1024 * 1024 * 1024 / PAGE_SIZE)))
>> +#else
>> +#define P2M_LIMIT xen_max_p2m_pfn
>> +#endif
>
> Can you arrange the #ifdef's to set xen_max_p2m_pfn to the right value
> instead of introducing P2M_LIMIT?

Hmm, this would require additional checks in setup.c. What about:

#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
#define P2M_LIMIT CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
#else
#define P2M_LIMIT 0
#endif

and do the max(...) calculation in xen_vmalloc_p2m_tree()?


Juergen

Re: [Xen-devel] [PATCH 1/2] xen: prepare p2m list for memory hotplug

[David Vrabel]

On 19/03/15 17:16, Juergen Gross wrote:
> On 03/19/2015 05:18 PM, David Vrabel wrote:
>> On 19/03/15 14:31, Juergen Gross wrote:
>>> Commit 054954eb051f35e74b75a566a96fe756015352c8 ("xen: switch to linear
>>> virtual mapped sparse p2m list") introduced a regression regarding to
>>> memory hotplug for a pv-domain: as the virtual space for the p2m list
>>> is allocated for the to be expected memory size of the domain only,
>>> hotplugged memory above that size will not be usable by the domain.
>>>
>>> Correct this by using a configurable size for the p2m list in case of
>>> memory hotplug enabled (default supported memory size is 512 GB for
>>> 64 bit domains and 4 GB for 32 bit domains).
>> [...]
>>> --- a/arch/x86/xen/p2m.c
>>> +++ b/arch/x86/xen/p2m.c
>>> @@ -91,6 +91,17 @@ EXPORT_SYMBOL_GPL(xen_p2m_size);
>>>   unsigned long xen_max_p2m_pfn __read_mostly;
>>>   EXPORT_SYMBOL_GPL(xen_max_p2m_pfn);
>>>
>>> +#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
>>> +#ifdef CONFIG_X86_32
>>> +BUILD_BUG_ON_MSG(CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT > 64)
>>> +#endif
>>> +#define P2M_LIMIT max(xen_max_p2m_pfn,                    \
>>> +    ((unsigned long)((u64)CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
>>> *    \
>>> +    1024 * 1024 * 1024 / PAGE_SIZE)))
>>> +#else
>>> +#define P2M_LIMIT xen_max_p2m_pfn
>>> +#endif
>>
>> Can you arrange the #ifdef's to set xen_max_p2m_pfn to the right value
>> instead of introducing P2M_LIMIT?
> 
> Hmm, this would require additional checks in setup.c. What about:
> 
> #ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
> #define P2M_LIMIT CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
> #else
> #define P2M_LIMIT 0
> #endif
> 
> and do the max(...) calculation in xen_vmalloc_p2m_tree()?

Yes, this is fine.

David

Re: [PATCH 1/2] xen: prepare p2m list for memory hotplug

[Paul Bolle]

On Thu, 2015-03-19 at 15:31 +0100, Juergen Gross wrote:
> --- a/arch/x86/xen/p2m.c
> +++ b/arch/x86/xen/p2m.c

> +#ifdef CONFIG_X86_32
> +BUILD_BUG_ON_MSG(CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT > 64)
> +#endif

I assume BUILD_BUG_ON_MSG() aborts the build. 

> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig

> +config XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
> +	int
> +	default 512 if X86_64
> +	default 4 if X86_32
> +	depends on XEN_HAVE_PVMMU
> +	depends on XEN_BALLOON_MEMORY_HOTPLUG
> +	help
> +	  Upper limit in GBs a pv domain can be expanded to using memory
> +	  hotplug.
> +
> +	  This value is used to allocate enough space in internal tables needed
> +	  for physical memory administration.
> +

I think adding a
	range 1 64 if X86_32

would allow to drop the BUILD_BUG_ON_MSG(). (I haven't tested this so
you're allowed to bark at me if this ends up wasting your time.)


Paul Bolle

[PATCH 2/2] xen: before ballooning hotplugged memory, set frames to invalid

[Juergen Gross]

Commit 25b884a83d487fd62c3de7ac1ab5549979188482 ("x86/xen: set
regions above the end of RAM as 1:1") introduced a regression.

To be able to add memory pages which were added via memory hotplug to
a pv domain, the pages must be "invalid" instead of "identity" in the
p2m list before they can be added.

Suggested-by: David Vrabel <david.vrabel@citrix.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/balloon.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index 0b52d92..52e331f 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -221,15 +221,24 @@ static bool balloon_is_inflated(void)
 
 static enum bp_state reserve_additional_memory(long credit)
 {
-	int nid, rc;
+	int nid, rc = 0;
 	u64 hotplug_start_paddr;
 	unsigned long balloon_hotplug = credit;
+	unsigned long pfn;
 
 	hotplug_start_paddr = PFN_PHYS(SECTION_ALIGN_UP(max_pfn));
 	balloon_hotplug = round_up(balloon_hotplug, PAGES_PER_SECTION);
 	nid = memory_add_physaddr_to_nid(hotplug_start_paddr);
 
-	rc = add_memory(nid, hotplug_start_paddr, balloon_hotplug << PAGE_SHIFT);
+	for (pfn = PFN_DOWN(hotplug_start_paddr);
+	     !rc && pfn < PFN_DOWN(hotplug_start_paddr) + balloon_hotplug;
+	     pfn++)
+		if (!set_phys_to_machine(pfn, INVALID_P2M_ENTRY))
+			rc = 1;
+
+	if (!rc)
+		rc = add_memory(nid, hotplug_start_paddr,
+				balloon_hotplug << PAGE_SHIFT);
 
 	if (rc) {
 		pr_warn("Cannot add additional memory (%i)\n", rc);
-- 
2.1.4

Re: [Xen-devel] [PATCH 2/2] xen: before ballooning hotplugged memory, set frames to invalid

[David Vrabel]

On 19/03/15 14:31, Juergen Gross wrote:
> Commit 25b884a83d487fd62c3de7ac1ab5549979188482 ("x86/xen: set
> regions above the end of RAM as 1:1") introduced a regression.
> 
> To be able to add memory pages which were added via memory hotplug to
> a pv domain, the pages must be "invalid" instead of "identity" in the
> p2m list before they can be added.
[...]
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -221,15 +221,24 @@ static bool balloon_is_inflated(void)
>  
>  static enum bp_state reserve_additional_memory(long credit)
>  {
> -	int nid, rc;
> +	int nid, rc = 0;
>  	u64 hotplug_start_paddr;
>  	unsigned long balloon_hotplug = credit;
> +	unsigned long pfn;
>  
>  	hotplug_start_paddr = PFN_PHYS(SECTION_ALIGN_UP(max_pfn));
>  	balloon_hotplug = round_up(balloon_hotplug, PAGES_PER_SECTION);
>  	nid = memory_add_physaddr_to_nid(hotplug_start_paddr);
>  
> -	rc = add_memory(nid, hotplug_start_paddr, balloon_hotplug << PAGE_SHIFT);
> +	for (pfn = PFN_DOWN(hotplug_start_paddr);
> +	     !rc && pfn < PFN_DOWN(hotplug_start_paddr) + balloon_hotplug;
> +	     pfn++)
> +		if (!set_phys_to_machine(pfn, INVALID_P2M_ENTRY))
> +			rc = 1;

    rc = -ENOMEM;
    break;

> +
> +	if (!rc)
> +		rc = add_memory(nid, hotplug_start_paddr,
> +				balloon_hotplug << PAGE_SHIFT);

Use else here.

>  	if (rc) {
>  		pr_warn("Cannot add additional memory (%i)\n", rc);
> 

Re: [Xen-devel] [PATCH 2/2] xen: before ballooning hotplugged memory, set frames to invalid

[Juergen Gross]

On 03/19/2015 05:18 PM, David Vrabel wrote:
> On 19/03/15 14:31, Juergen Gross wrote:
>> Commit 25b884a83d487fd62c3de7ac1ab5549979188482 ("x86/xen: set
>> regions above the end of RAM as 1:1") introduced a regression.
>>
>> To be able to add memory pages which were added via memory hotplug to
>> a pv domain, the pages must be "invalid" instead of "identity" in the
>> p2m list before they can be added.
> [...]
>> --- a/drivers/xen/balloon.c
>> +++ b/drivers/xen/balloon.c
>> @@ -221,15 +221,24 @@ static bool balloon_is_inflated(void)
>>
>>   static enum bp_state reserve_additional_memory(long credit)
>>   {
>> -	int nid, rc;
>> +	int nid, rc = 0;
>>   	u64 hotplug_start_paddr;
>>   	unsigned long balloon_hotplug = credit;
>> +	unsigned long pfn;
>>
>>   	hotplug_start_paddr = PFN_PHYS(SECTION_ALIGN_UP(max_pfn));
>>   	balloon_hotplug = round_up(balloon_hotplug, PAGES_PER_SECTION);
>>   	nid = memory_add_physaddr_to_nid(hotplug_start_paddr);
>>
>> -	rc = add_memory(nid, hotplug_start_paddr, balloon_hotplug << PAGE_SHIFT);
>> +	for (pfn = PFN_DOWN(hotplug_start_paddr);
>> +	     !rc && pfn < PFN_DOWN(hotplug_start_paddr) + balloon_hotplug;
>> +	     pfn++)
>> +		if (!set_phys_to_machine(pfn, INVALID_P2M_ENTRY))
>> +			rc = 1;
>
>      rc = -ENOMEM;

I used the value 1 on purpose to be able to identify the case by the
value printed in the warning below.

>      break;

Why? !rc is already tested in the for() clause.

>
>> +
>> +	if (!rc)
>> +		rc = add_memory(nid, hotplug_start_paddr,
>> +				balloon_hotplug << PAGE_SHIFT);
>
> Use else here.

Huh? I want the message to be printed if either set_phys_to_machine()
or add_memory() failed.

>
>>   	if (rc) {
>>   		pr_warn("Cannot add additional memory (%i)\n", rc);
>>


Juergen

Re: [Xen-devel] [PATCH 2/2] xen: before ballooning hotplugged memory, set frames to invalid

[David Vrabel]

On 19/03/15 17:19, Juergen Gross wrote:
> On 03/19/2015 05:18 PM, David Vrabel wrote:
>> On 19/03/15 14:31, Juergen Gross wrote:
>>> Commit 25b884a83d487fd62c3de7ac1ab5549979188482 ("x86/xen: set
>>> regions above the end of RAM as 1:1") introduced a regression.
>>>
>>> To be able to add memory pages which were added via memory hotplug to
>>> a pv domain, the pages must be "invalid" instead of "identity" in the
>>> p2m list before they can be added.
>> [...]
>>> --- a/drivers/xen/balloon.c
>>> +++ b/drivers/xen/balloon.c
>>> @@ -221,15 +221,24 @@ static bool balloon_is_inflated(void)
>>>
>>>   static enum bp_state reserve_additional_memory(long credit)
>>>   {
>>> -    int nid, rc;
>>> +    int nid, rc = 0;
>>>       u64 hotplug_start_paddr;
>>>       unsigned long balloon_hotplug = credit;
>>> +    unsigned long pfn;
>>>
>>>       hotplug_start_paddr = PFN_PHYS(SECTION_ALIGN_UP(max_pfn));
>>>       balloon_hotplug = round_up(balloon_hotplug, PAGES_PER_SECTION);
>>>       nid = memory_add_physaddr_to_nid(hotplug_start_paddr);
>>>
>>> -    rc = add_memory(nid, hotplug_start_paddr, balloon_hotplug <<
>>> PAGE_SHIFT);
>>> +    for (pfn = PFN_DOWN(hotplug_start_paddr);
>>> +         !rc && pfn < PFN_DOWN(hotplug_start_paddr) + balloon_hotplug;
>>> +         pfn++)
>>> +        if (!set_phys_to_machine(pfn, INVALID_P2M_ENTRY))
>>> +            rc = 1;
>>
>>      rc = -ENOMEM;
> 
> I used the value 1 on purpose to be able to identify the case by the
> value printed in the warning below.

Ok.

> 
>>      break;
> 
> Why? !rc is already tested in the for() clause.

I prefer an explicit break.

>>> +
>>> +    if (!rc)
>>> +        rc = add_memory(nid, hotplug_start_paddr,
>>> +                balloon_hotplug << PAGE_SHIFT);
>>
>> Use else here.
> 
> Huh? I want the message to be printed if either set_phys_to_machine()
> or add_memory() failed.

Ok.

David

Re: [PATCH 2/2] xen: before ballooning hotplugged memory, set frames to invalid

[Daniel Kiper]

On Thu, Mar 19, 2015 at 03:31:02PM +0100, Juergen Gross wrote:
> Commit 25b884a83d487fd62c3de7ac1ab5549979188482 ("x86/xen: set
> regions above the end of RAM as 1:1") introduced a regression.
>
> To be able to add memory pages which were added via memory hotplug to
> a pv domain, the pages must be "invalid" instead of "identity" in the
> p2m list before they can be added.
>
> Suggested-by: David Vrabel <david.vrabel@citrix.com>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  drivers/xen/balloon.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 0b52d92..52e331f 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -221,15 +221,24 @@ static bool balloon_is_inflated(void)
>
>  static enum bp_state reserve_additional_memory(long credit)
>  {
> -	int nid, rc;
> +	int nid, rc = 0;
>  	u64 hotplug_start_paddr;
>  	unsigned long balloon_hotplug = credit;
> +	unsigned long pfn;
>
>  	hotplug_start_paddr = PFN_PHYS(SECTION_ALIGN_UP(max_pfn));
>  	balloon_hotplug = round_up(balloon_hotplug, PAGES_PER_SECTION);
>  	nid = memory_add_physaddr_to_nid(hotplug_start_paddr);
>
> -	rc = add_memory(nid, hotplug_start_paddr, balloon_hotplug << PAGE_SHIFT);
> +	for (pfn = PFN_DOWN(hotplug_start_paddr);
> +	     !rc && pfn < PFN_DOWN(hotplug_start_paddr) + balloon_hotplug;
> +	     pfn++)
> +		if (!set_phys_to_machine(pfn, INVALID_P2M_ENTRY))

rc = set_phys_to_machine(pfn, INVALID_P2M_ENTRY)?

> +			rc = 1;

I do not think that this stuff is needed for HVM or PVH guests.

> +	if (!rc)
> +		rc = add_memory(nid, hotplug_start_paddr,
> +				balloon_hotplug << PAGE_SHIFT);
>
>  	if (rc) {
>  		pr_warn("Cannot add additional memory (%i)\n", rc);

It will be nice to know what part of infrastructure failed.
Could you create separate pr_warn() message for set_phys_to_machine()?

Daniel

Re: [PATCH 2/2] xen: before ballooning hotplugged memory, set frames to invalid

[Juergen Gross]

On 03/19/2015 05:21 PM, Daniel Kiper wrote:
> On Thu, Mar 19, 2015 at 03:31:02PM +0100, Juergen Gross wrote:
>> Commit 25b884a83d487fd62c3de7ac1ab5549979188482 ("x86/xen: set
>> regions above the end of RAM as 1:1") introduced a regression.
>>
>> To be able to add memory pages which were added via memory hotplug to
>> a pv domain, the pages must be "invalid" instead of "identity" in the
>> p2m list before they can be added.
>>
>> Suggested-by: David Vrabel <david.vrabel@citrix.com>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   drivers/xen/balloon.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
>> index 0b52d92..52e331f 100644
>> --- a/drivers/xen/balloon.c
>> +++ b/drivers/xen/balloon.c
>> @@ -221,15 +221,24 @@ static bool balloon_is_inflated(void)
>>
>>   static enum bp_state reserve_additional_memory(long credit)
>>   {
>> -	int nid, rc;
>> +	int nid, rc = 0;
>>   	u64 hotplug_start_paddr;
>>   	unsigned long balloon_hotplug = credit;
>> +	unsigned long pfn;
>>
>>   	hotplug_start_paddr = PFN_PHYS(SECTION_ALIGN_UP(max_pfn));
>>   	balloon_hotplug = round_up(balloon_hotplug, PAGES_PER_SECTION);
>>   	nid = memory_add_physaddr_to_nid(hotplug_start_paddr);
>>
>> -	rc = add_memory(nid, hotplug_start_paddr, balloon_hotplug << PAGE_SHIFT);
>> +	for (pfn = PFN_DOWN(hotplug_start_paddr);
>> +	     !rc && pfn < PFN_DOWN(hotplug_start_paddr) + balloon_hotplug;
>> +	     pfn++)
>> +		if (!set_phys_to_machine(pfn, INVALID_P2M_ENTRY))
>
> rc = set_phys_to_machine(pfn, INVALID_P2M_ENTRY)?

Not really. set_phys_to_machine returns false on failure...

>
>> +			rc = 1;
>
> I do not think that this stuff is needed for HVM or PVH guests.

True.

>
>> +	if (!rc)
>> +		rc = add_memory(nid, hotplug_start_paddr,
>> +				balloon_hotplug << PAGE_SHIFT);
>>
>>   	if (rc) {
>>   		pr_warn("Cannot add additional memory (%i)\n", rc);
>
> It will be nice to know what part of infrastructure failed.
> Could you create separate pr_warn() message for set_phys_to_machine()?

Value 1 for rc is the indicator for that case.


Juergen

Re: [PATCH 2/2] xen: before ballooning hotplugged memory, set frames to invalid

[Daniel Kiper]

On Thu, Mar 19, 2015 at 06:22:06PM +0100, Juergen Gross wrote:
> On 03/19/2015 05:21 PM, Daniel Kiper wrote:
> >On Thu, Mar 19, 2015 at 03:31:02PM +0100, Juergen Gross wrote:
> >>Commit 25b884a83d487fd62c3de7ac1ab5549979188482 ("x86/xen: set
> >>regions above the end of RAM as 1:1") introduced a regression.
> >>
> >>To be able to add memory pages which were added via memory hotplug to
> >>a pv domain, the pages must be "invalid" instead of "identity" in the
> >>p2m list before they can be added.
> >>
> >>Suggested-by: David Vrabel <david.vrabel@citrix.com>
> >>Signed-off-by: Juergen Gross <jgross@suse.com>
> >>---
> >>  drivers/xen/balloon.c | 13 +++++++++++--
> >>  1 file changed, 11 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> >>index 0b52d92..52e331f 100644
> >>--- a/drivers/xen/balloon.c
> >>+++ b/drivers/xen/balloon.c
> >>@@ -221,15 +221,24 @@ static bool balloon_is_inflated(void)
> >>
> >>  static enum bp_state reserve_additional_memory(long credit)
> >>  {
> >>-	int nid, rc;
> >>+	int nid, rc = 0;
> >>  	u64 hotplug_start_paddr;
> >>  	unsigned long balloon_hotplug = credit;
> >>+	unsigned long pfn;
> >>
> >>  	hotplug_start_paddr = PFN_PHYS(SECTION_ALIGN_UP(max_pfn));
> >>  	balloon_hotplug = round_up(balloon_hotplug, PAGES_PER_SECTION);
> >>  	nid = memory_add_physaddr_to_nid(hotplug_start_paddr);
> >>
> >>-	rc = add_memory(nid, hotplug_start_paddr, balloon_hotplug << PAGE_SHIFT);
> >>+	for (pfn = PFN_DOWN(hotplug_start_paddr);
> >>+	     !rc && pfn < PFN_DOWN(hotplug_start_paddr) + balloon_hotplug;
> >>+	     pfn++)
> >>+		if (!set_phys_to_machine(pfn, INVALID_P2M_ENTRY))
> >
> >rc = set_phys_to_machine(pfn, INVALID_P2M_ENTRY)?
>
> Not really. set_phys_to_machine returns false on failure...
>
> >
> >>+			rc = 1;
> >
> >I do not think that this stuff is needed for HVM or PVH guests.
>
> True.
>
> >
> >>+	if (!rc)
> >>+		rc = add_memory(nid, hotplug_start_paddr,
> >>+				balloon_hotplug << PAGE_SHIFT);
> >>
> >>  	if (rc) {
> >>  		pr_warn("Cannot add additional memory (%i)\n", rc);
> >
> >It will be nice to know what part of infrastructure failed.
> >Could you create separate pr_warn() message for set_phys_to_machine()?
>
> Value 1 for rc is the indicator for that case.

Well... Personally I prefer explicit messages telling what happened than
something which requires digging in a code to understand a problem.
Additionally, I think that we should use negative numbers (as David
suggested) to signal an error. Most of kernel stuff work in that way.

Daniel