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

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

[Juergen Gross]

Fix some regressions introduced in 3.16 and 3.19.

Changes in V2:
- use "range" in Kconfig instead of BUILD_BUG_ON_MSG() as suggested by
  Boris Ostrovsky and Paul Bolle
- simplify ifdeffery regarding P2M_LIMIT in patch 1 as requested by David Vrabel
- changed test for failure of set_phys_to_machine() in patch 2 as requested
  by David Vrabel and Daniel Kiper
- only call set_phys_to_machine() for pv domains as requested by Daniel Kiper

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

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

-- 
2.1.4

[Patch V2 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  | 10 +++++++++-
 drivers/xen/Kconfig | 14 ++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
index 9f93af5..b47124d 100644
--- a/arch/x86/xen/p2m.c
+++ b/arch/x86/xen/p2m.c
@@ -91,6 +91,12 @@ 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
+#define P2M_LIMIT CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
+#else
+#define P2M_LIMIT 0
+#endif
+
 static DEFINE_SPINLOCK(p2m_update_lock);
 
 static unsigned long *p2m_mid_missing_mfn;
@@ -385,9 +391,11 @@ static void __init xen_rebuild_p2m_list(unsigned long *p2m)
 void __init xen_vmalloc_p2m_tree(void)
 {
 	static struct vm_struct vm;
+	unsigned long p2m_limit;
 
+	p2m_limit = (phys_addr_t)P2M_LIMIT * 1024 * 1024 * 1024 / PAGE_SIZE;
 	vm.flags = VM_ALLOC;
-	vm.size = ALIGN(sizeof(unsigned long) * xen_max_p2m_pfn,
+	vm.size = ALIGN(sizeof(unsigned long) * max(xen_max_p2m_pfn, 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..0f1b509 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -55,6 +55,20 @@ 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
+	range 0 64 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 V2 1/2] xen: prepare p2m list for memory hotplug

[Daniel Kiper]

On Fri, Mar 20, 2015 at 01:55:38PM +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>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

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

[David Vrabel]

On 20/03/15 12:55, 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  | 10 +++++++++-
>  drivers/xen/Kconfig | 14 ++++++++++++++
>  2 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
> index 9f93af5..b47124d 100644
> --- a/arch/x86/xen/p2m.c
> +++ b/arch/x86/xen/p2m.c
> @@ -91,6 +91,12 @@ 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
> +#define P2M_LIMIT CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
> +#else
> +#define P2M_LIMIT 0
> +#endif
> +
>  static DEFINE_SPINLOCK(p2m_update_lock);
>  
>  static unsigned long *p2m_mid_missing_mfn;
> @@ -385,9 +391,11 @@ static void __init xen_rebuild_p2m_list(unsigned long *p2m)
>  void __init xen_vmalloc_p2m_tree(void)
>  {
>  	static struct vm_struct vm;
> +	unsigned long p2m_limit;
>  
> +	p2m_limit = (phys_addr_t)P2M_LIMIT * 1024 * 1024 * 1024 / PAGE_SIZE;
>  	vm.flags = VM_ALLOC;
> -	vm.size = ALIGN(sizeof(unsigned long) * xen_max_p2m_pfn,
> +	vm.size = ALIGN(sizeof(unsigned long) * max(xen_max_p2m_pfn, 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..0f1b509 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -55,6 +55,20 @@ config XEN_BALLOON_MEMORY_HOTPLUG
>  
>  	  In that case step 3 should be omitted.
>  
> +config XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
> +	int

Because you've not supplied a summary for this option, it is not user
configurable.  Is this intentional?

David

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

[Juergen Gross]

On 03/23/2015 01:46 PM, David Vrabel wrote:
> On 20/03/15 12:55, 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  | 10 +++++++++-
>>   drivers/xen/Kconfig | 14 ++++++++++++++
>>   2 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
>> index 9f93af5..b47124d 100644
>> --- a/arch/x86/xen/p2m.c
>> +++ b/arch/x86/xen/p2m.c
>> @@ -91,6 +91,12 @@ 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
>> +#define P2M_LIMIT CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
>> +#else
>> +#define P2M_LIMIT 0
>> +#endif
>> +
>>   static DEFINE_SPINLOCK(p2m_update_lock);
>>
>>   static unsigned long *p2m_mid_missing_mfn;
>> @@ -385,9 +391,11 @@ static void __init xen_rebuild_p2m_list(unsigned long *p2m)
>>   void __init xen_vmalloc_p2m_tree(void)
>>   {
>>   	static struct vm_struct vm;
>> +	unsigned long p2m_limit;
>>
>> +	p2m_limit = (phys_addr_t)P2M_LIMIT * 1024 * 1024 * 1024 / PAGE_SIZE;
>>   	vm.flags = VM_ALLOC;
>> -	vm.size = ALIGN(sizeof(unsigned long) * xen_max_p2m_pfn,
>> +	vm.size = ALIGN(sizeof(unsigned long) * max(xen_max_p2m_pfn, 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..0f1b509 100644
>> --- a/drivers/xen/Kconfig
>> +++ b/drivers/xen/Kconfig
>> @@ -55,6 +55,20 @@ config XEN_BALLOON_MEMORY_HOTPLUG
>>
>>   	  In that case step 3 should be omitted.
>>
>> +config XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
>> +	int
>
> Because you've not supplied a summary for this option, it is not user
> configurable.  Is this intentional?

No. Sorry, I just tested it via editing .config, not using the menues.
Can you add it when committing (e.g. "Upper limit in GBs a pv domain
can be expanded to using memory hotplug")?

Or should I send another patch?


Juergen

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

[David Vrabel]

On 23/03/15 14:29, Juergen Gross wrote:
> On 03/23/2015 01:46 PM, David Vrabel wrote:
>> On 20/03/15 12:55, 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  | 10 +++++++++-
>>>   drivers/xen/Kconfig | 14 ++++++++++++++
>>>   2 files changed, 23 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
>>> index 9f93af5..b47124d 100644
>>> --- a/arch/x86/xen/p2m.c
>>> +++ b/arch/x86/xen/p2m.c
>>> @@ -91,6 +91,12 @@ 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
>>> +#define P2M_LIMIT CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
>>> +#else
>>> +#define P2M_LIMIT 0
>>> +#endif
>>> +
>>>   static DEFINE_SPINLOCK(p2m_update_lock);
>>>
>>>   static unsigned long *p2m_mid_missing_mfn;
>>> @@ -385,9 +391,11 @@ static void __init xen_rebuild_p2m_list(unsigned
>>> long *p2m)
>>>   void __init xen_vmalloc_p2m_tree(void)
>>>   {
>>>       static struct vm_struct vm;
>>> +    unsigned long p2m_limit;
>>>
>>> +    p2m_limit = (phys_addr_t)P2M_LIMIT * 1024 * 1024 * 1024 /
>>> PAGE_SIZE;
>>>       vm.flags = VM_ALLOC;
>>> -    vm.size = ALIGN(sizeof(unsigned long) * xen_max_p2m_pfn,
>>> +    vm.size = ALIGN(sizeof(unsigned long) * max(xen_max_p2m_pfn,
>>> 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..0f1b509 100644
>>> --- a/drivers/xen/Kconfig
>>> +++ b/drivers/xen/Kconfig
>>> @@ -55,6 +55,20 @@ config XEN_BALLOON_MEMORY_HOTPLUG
>>>
>>>         In that case step 3 should be omitted.
>>>
>>> +config XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
>>> +    int
>>
>> Because you've not supplied a summary for this option, it is not user
>> configurable.  Is this intentional?
> 
> No. Sorry, I just tested it via editing .config, not using the menues.
> Can you add it when committing (e.g. "Upper limit in GBs a pv domain
> can be expanded to using memory hotplug")?
> 
> Or should I send another patch?

I'll fix it up.

David

[Patch V2 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, 13 insertions(+)

diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index 0b52d92..65fedb8 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -229,6 +229,19 @@ static enum bp_state reserve_additional_memory(long credit)
 	balloon_hotplug = round_up(balloon_hotplug, PAGES_PER_SECTION);
 	nid = memory_add_physaddr_to_nid(hotplug_start_paddr);
 
+#ifdef CONFIG_XEN_HAVE_PVMMU
+	if (!xen_feature(XENFEAT_auto_translated_physmap)) {
+		unsigned long pfn, i;
+
+		pfn = PFN_DOWN(hotplug_start_paddr);
+		for (i = 0; i < balloon_hotplug; i++)
+			if (!set_phys_to_machine(pfn + i, INVALID_P2M_ENTRY)) {
+				pr_warn("set_phys_to_machine() failed, no memory added\n");
+				return BP_ECANCELED;
+			}
+	}
+#endif
+
 	rc = add_memory(nid, hotplug_start_paddr, balloon_hotplug << PAGE_SHIFT);
 
 	if (rc) {
-- 
2.1.4

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

[Boris Ostrovsky]

On 03/20/2015 08:55 AM, 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, 13 insertions(+)
>
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 0b52d92..65fedb8 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -229,6 +229,19 @@ static enum bp_state reserve_additional_memory(long credit)
>   	balloon_hotplug = round_up(balloon_hotplug, PAGES_PER_SECTION);
>   	nid = memory_add_physaddr_to_nid(hotplug_start_paddr);
>   
> +#ifdef CONFIG_XEN_HAVE_PVMMU
> +	if (!xen_feature(XENFEAT_auto_translated_physmap)) {
> +		unsigned long pfn, i;
> +
> +		pfn = PFN_DOWN(hotplug_start_paddr);
> +		for (i = 0; i < balloon_hotplug; i++)
> +			if (!set_phys_to_machine(pfn + i, INVALID_P2M_ENTRY)) {
> +				pr_warn("set_phys_to_machine() failed, no memory added\n");
> +				return BP_ECANCELED;

I should have asked last time --- should we restore [0..i-1] mapping 
back to identity on error here? I wonder whether leaving those frames as 
invalid (when they are expected to be 'identity') could cause problems 
in the future.

-boris

> +			}
> +	}
> +#endif
> +
>   	rc = add_memory(nid, hotplug_start_paddr, balloon_hotplug << PAGE_SHIFT);
>   
>   	if (rc) {

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

[Juergen Gross]

On 03/20/2015 02:44 PM, Boris Ostrovsky wrote:
> On 03/20/2015 08:55 AM, 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, 13 insertions(+)
>>
>> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
>> index 0b52d92..65fedb8 100644
>> --- a/drivers/xen/balloon.c
>> +++ b/drivers/xen/balloon.c
>> @@ -229,6 +229,19 @@ static enum bp_state
>> reserve_additional_memory(long credit)
>>       balloon_hotplug = round_up(balloon_hotplug, PAGES_PER_SECTION);
>>       nid = memory_add_physaddr_to_nid(hotplug_start_paddr);
>> +#ifdef CONFIG_XEN_HAVE_PVMMU
>> +    if (!xen_feature(XENFEAT_auto_translated_physmap)) {
>> +        unsigned long pfn, i;
>> +
>> +        pfn = PFN_DOWN(hotplug_start_paddr);
>> +        for (i = 0; i < balloon_hotplug; i++)
>> +            if (!set_phys_to_machine(pfn + i, INVALID_P2M_ENTRY)) {
>> +                pr_warn("set_phys_to_machine() failed, no memory
>> added\n");
>> +                return BP_ECANCELED;
>
> I should have asked last time --- should we restore [0..i-1] mapping
> back to identity on error here? I wonder whether leaving those frames as
> invalid (when they are expected to be 'identity') could cause problems
> in the future.

The only reason why set_phys_to_machine() can fail is a lack of memory
(which in this case would really be funny: adding memory fails due to a
lack of memory). In case a retry succeeds everything is fine as setting
an invalid entry to invalid again does no harm. In case the invalid
entry does any harm it would have done so without set_phys_to_machine()
failing. So I see no reason to restore the identity entries.

Juergen

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

[David Vrabel]

On 20/03/15 13:44, Boris Ostrovsky wrote:
> On 03/20/2015 08:55 AM, 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, 13 insertions(+)
>>
>> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
>> index 0b52d92..65fedb8 100644
>> --- a/drivers/xen/balloon.c
>> +++ b/drivers/xen/balloon.c
>> @@ -229,6 +229,19 @@ static enum bp_state
>> reserve_additional_memory(long credit)
>>       balloon_hotplug = round_up(balloon_hotplug, PAGES_PER_SECTION);
>>       nid = memory_add_physaddr_to_nid(hotplug_start_paddr);
>>   +#ifdef CONFIG_XEN_HAVE_PVMMU

I've added this comment:

        /*
         * add_memory() will build page tables for the new memory so
         * the p2m must contain invalidate entries so the correct
         * non-present PTEs will be written.
         *
         * If a failure occurs, the original (identity) p2m entries
         * are not restored since this region is now known not to
         * conflict with any devices.
         */

>> +    if (!xen_feature(XENFEAT_auto_translated_physmap)) {
>> +        unsigned long pfn, i;
>> +
>> +        pfn = PFN_DOWN(hotplug_start_paddr);
>> +        for (i = 0; i < balloon_hotplug; i++)
>> +            if (!set_phys_to_machine(pfn + i, INVALID_P2M_ENTRY)) {
>> +                pr_warn("set_phys_to_machine() failed, no memory
>> added\n");
>> +                return BP_ECANCELED;
> 
> I should have asked last time --- should we restore [0..i-1] mapping
> back to identity on error here? I wonder whether leaving those frames as
> invalid (when they are expected to be 'identity') could cause problems
> in the future.

See new comment above.

David

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

[Daniel Kiper]

On Fri, Mar 20, 2015 at 01:55:39PM +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>

In general...

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

... but...

> ---
>  drivers/xen/balloon.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 0b52d92..65fedb8 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -229,6 +229,19 @@ static enum bp_state reserve_additional_memory(long credit)
>  	balloon_hotplug = round_up(balloon_hotplug, PAGES_PER_SECTION);
>  	nid = memory_add_physaddr_to_nid(hotplug_start_paddr);
>
> +#ifdef CONFIG_XEN_HAVE_PVMMU
> +	if (!xen_feature(XENFEAT_auto_translated_physmap)) {
> +		unsigned long pfn, i;
> +
> +		pfn = PFN_DOWN(hotplug_start_paddr);
> +		for (i = 0; i < balloon_hotplug; i++)
> +			if (!set_phys_to_machine(pfn + i, INVALID_P2M_ENTRY)) {
> +				pr_warn("set_phys_to_machine() failed, no memory added\n");
> +				return BP_ECANCELED;
> +			}
> +	}
> +#endif

Should not we fill everything above "maxmem" with
INVALID_P2M_ENTRY at boot time?

Daniel

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

[Juergen Gross]

On 03/20/2015 02:46 PM, Daniel Kiper wrote:
> On Fri, Mar 20, 2015 at 01:55:39PM +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>
>
> In general...
>
> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
>
> ... but...
>
>> ---
>>   drivers/xen/balloon.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
>> index 0b52d92..65fedb8 100644
>> --- a/drivers/xen/balloon.c
>> +++ b/drivers/xen/balloon.c
>> @@ -229,6 +229,19 @@ static enum bp_state reserve_additional_memory(long credit)
>>   	balloon_hotplug = round_up(balloon_hotplug, PAGES_PER_SECTION);
>>   	nid = memory_add_physaddr_to_nid(hotplug_start_paddr);
>>
>> +#ifdef CONFIG_XEN_HAVE_PVMMU
>> +	if (!xen_feature(XENFEAT_auto_translated_physmap)) {
>> +		unsigned long pfn, i;
>> +
>> +		pfn = PFN_DOWN(hotplug_start_paddr);
>> +		for (i = 0; i < balloon_hotplug; i++)
>> +			if (!set_phys_to_machine(pfn + i, INVALID_P2M_ENTRY)) {
>> +				pr_warn("set_phys_to_machine() failed, no memory added\n");
>> +				return BP_ECANCELED;
>> +			}
>> +	}
>> +#endif
>
> Should not we fill everything above "maxmem" with
> INVALID_P2M_ENTRY at boot time?

In this case we shouldn't report "identity" for pfns above maxmem, too.
Otherwise we could change behaviour of the kernel regarding PCI
passthrough just by changing CONFIG_XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
which doesn't sound right.


Juergen

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

[David Vrabel]

On 20/03/15 13:46, Daniel Kiper wrote:
> On Fri, Mar 20, 2015 at 01:55:39PM +0100, Juergen Gross wrote:
>> Commit 25b884a83d487fd62c3de7ac1ab5549979188482 ("x86/xen: set
>> regions above the end of RAM as 1:1") introduced a regression.
>>
> 
> Should not we fill everything above "maxmem" with
> INVALID_P2M_ENTRY at boot time?

No.  See the commit referenced above.

David

Re: [Xen-devel] [Patch V2 0/2] xen: fix regressions regarding memory hotplug in pv domains

[David Vrabel]

On 20/03/15 12:55, Juergen Gross wrote:
> Fix some regressions introduced in 3.16 and 3.19.

Applied to stable/for-linus-4.0 and tagged for stable, thanks.

David