[PATCH v3] xen/balloon: Mark unallocated host memory as UNUSABLE

[PATCH v3] xen/balloon: Mark unallocated host memory as UNUSABLE

[Boris Ostrovsky]

Commit f5775e0b6116 ("x86/xen: discard RAM regions above the maximum
reservation") left host memory not assigned to dom0 as available for
memory hotplug.

Unfortunately this also meant that those regions could be used by
others. Specifically, commit fa564ad96366 ("x86/PCI: Enable a 64bit BAR
on AMD Family 15h (Models 00-1f, 30-3f, 60-7f)") may try to map those
addresses as MMIO.

To prevent this mark unallocated host memory as E820_TYPE_UNUSABLE (thus
effectively reverting f5775e0b6116) and keep track of that region as
a hostmem resource that can be used for the hotplug.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
Changes in v3:
* Use PFN_PHYS
* Replace kzalloc with kmalloc
* Declare arch_xen_balloon_init prototype in balloon.h
* Rename resources (s/memory/RAM/)
* Clarify (I think) comment when populating hostmem_resource
* Print open-ended interval on insert_resource() error
* Constify declaration of struct e820_entry *entry

 arch/x86/xen/enlighten.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++
 arch/x86/xen/setup.c     |  6 ++--
 drivers/xen/balloon.c    | 65 ++++++++++++++++++++++++++++++++------
 include/xen/balloon.h    |  5 +++
 4 files changed, 144 insertions(+), 13 deletions(-)

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index d669e9d..c9081c6 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -1,8 +1,12 @@
+#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG
+#include <linux/bootmem.h>
+#endif
 #include <linux/cpu.h>
 #include <linux/kexec.h>
 
 #include <xen/features.h>
 #include <xen/page.h>
+#include <xen/interface/memory.h>
 
 #include <asm/xen/hypercall.h>
 #include <asm/xen/hypervisor.h>
@@ -331,3 +335,80 @@ void xen_arch_unregister_cpu(int num)
 }
 EXPORT_SYMBOL(xen_arch_unregister_cpu);
 #endif
+
+#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG
+void __init arch_xen_balloon_init(struct resource *hostmem_resource)
+{
+	struct xen_memory_map memmap;
+	int rc;
+	unsigned int i, last_guest_ram;
+	phys_addr_t max_addr = PFN_PHYS(max_pfn);
+	struct e820_table *xen_e820_table;
+	const struct e820_entry *entry;
+	struct resource *res;
+
+	if (!xen_initial_domain())
+		return;
+
+	xen_e820_table = kmalloc(sizeof(*xen_e820_table), GFP_KERNEL);
+	if (!xen_e820_table)
+		return;
+
+	memmap.nr_entries = ARRAY_SIZE(xen_e820_table->entries);
+	set_xen_guest_handle(memmap.buffer, xen_e820_table->entries);
+	rc = HYPERVISOR_memory_op(XENMEM_machine_memory_map, &memmap);
+	if (rc) {
+		pr_warn("%s: Can't read host e820 (%d)\n", __func__, rc);
+		goto out;
+	}
+
+	last_guest_ram = 0;
+	for (i = 0; i < memmap.nr_entries; i++) {
+		if (xen_e820_table->entries[i].addr >= max_addr)
+			break;
+		if (xen_e820_table->entries[i].type == E820_TYPE_RAM)
+			last_guest_ram = i;
+	}
+
+	entry = &xen_e820_table->entries[last_guest_ram];
+	if (max_addr >= entry->addr + entry->size)
+		goto out; /* No unallocated host RAM. */
+
+	hostmem_resource->start = max_addr;
+	hostmem_resource->end = entry->addr + entry->size;
+
+	/*
+	 * Mark non-RAM regions between the end of dom0 RAM and end of host RAM
+	 * as unavailable. The rest of that region can be used for hotplug-based
+	 * ballooning.
+	 */
+	for (; i < memmap.nr_entries; i++) {
+		entry = &xen_e820_table->entries[i];
+
+		if (entry->type == E820_TYPE_RAM)
+			continue;
+
+		if (entry->addr >= hostmem_resource->end)
+			break;
+
+		res = kzalloc(sizeof(*res), GFP_KERNEL);
+		if (!res)
+			goto out;
+
+		res->name = "Unavailable host RAM";
+		res->start = entry->addr;
+		res->end = (entry->addr + entry->size < hostmem_resource->end) ?
+			    entry->addr + entry->size : hostmem_resource->end;
+		rc = insert_resource(hostmem_resource, res);
+		if (rc) {
+			pr_warn("%s: Can't insert [%llx - %llx) (%d)\n",
+				__func__, res->start, res->end, rc);
+			kfree(res);
+			goto  out;
+		}
+	}
+
+ out:
+	kfree(xen_e820_table);
+}
+#endif /* CONFIG_XEN_BALLOON_MEMORY_HOTPLUG */
diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
index c114ca7..6e0d208 100644
--- a/arch/x86/xen/setup.c
+++ b/arch/x86/xen/setup.c
@@ -808,7 +808,6 @@ char * __init xen_memory_setup(void)
 	addr = xen_e820_table.entries[0].addr;
 	size = xen_e820_table.entries[0].size;
 	while (i < xen_e820_table.nr_entries) {
-		bool discard = false;
 
 		chunk_size = size;
 		type = xen_e820_table.entries[i].type;
@@ -824,11 +823,10 @@ char * __init xen_memory_setup(void)
 				xen_add_extra_mem(pfn_s, n_pfns);
 				xen_max_p2m_pfn = pfn_s + n_pfns;
 			} else
-				discard = true;
+				type = E820_TYPE_UNUSABLE;
 		}
 
-		if (!discard)
-			xen_align_and_add_e820_region(addr, chunk_size, type);
+		xen_align_and_add_e820_region(addr, chunk_size, type);
 
 		addr += chunk_size;
 		size -= chunk_size;
diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index f77e499..065f0b6 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -257,10 +257,25 @@ static void release_memory_resource(struct resource *resource)
 	kfree(resource);
 }
 
+/*
+ * Host memory not allocated to dom0. We can use this range for hotplug-based
+ * ballooning.
+ *
+ * It's a type-less resource. Setting IORESOURCE_MEM will make resource
+ * management algorithms (arch_remove_reservations()) look into guest e820,
+ * which we don't want.
+ */
+static struct resource hostmem_resource = {
+	.name   = "Host RAM",
+};
+
+void __attribute__((weak)) __init arch_xen_balloon_init(struct resource *res)
+{}
+
 static struct resource *additional_memory_resource(phys_addr_t size)
 {
-	struct resource *res;
-	int ret;
+	struct resource *res, *res_hostmem;
+	int ret = -ENOMEM;
 
 	res = kzalloc(sizeof(*res), GFP_KERNEL);
 	if (!res)
@@ -269,13 +284,42 @@ static struct resource *additional_memory_resource(phys_addr_t size)
 	res->name = "System RAM";
 	res->flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
 
-	ret = allocate_resource(&iomem_resource, res,
-				size, 0, -1,
-				PAGES_PER_SECTION * PAGE_SIZE, NULL, NULL);
-	if (ret < 0) {
-		pr_err("Cannot allocate new System RAM resource\n");
-		kfree(res);
-		return NULL;
+	res_hostmem = kzalloc(sizeof(*res), GFP_KERNEL);
+	if (res_hostmem) {
+		/* Try to grab a range from hostmem */
+		res_hostmem->name = "Host memory";
+		ret = allocate_resource(&hostmem_resource, res_hostmem,
+					size, 0, -1,
+					PAGES_PER_SECTION * PAGE_SIZE, NULL, NULL);
+	}
+
+	if (!ret) {
+		/*
+		 * Insert this resource into iomem. Because hostmem_resource
+		 * tracks portion of guest e820 marked as UNUSABLE noone else
+		 * should try to use it.
+		 */
+		res->start = res_hostmem->start;
+		res->end = res_hostmem->end;
+		ret = insert_resource(&iomem_resource, res);
+		if (ret < 0) {
+			pr_err("Can't insert iomem_resource [%llx - %llx]\n",
+				res->start, res->end);
+			release_memory_resource(res_hostmem);
+			res_hostmem = NULL;
+			res->start = res->end = 0;
+		}
+	}
+
+	if (ret) {
+		ret = allocate_resource(&iomem_resource, res,
+					size, 0, -1,
+					PAGES_PER_SECTION * PAGE_SIZE, NULL, NULL);
+		if (ret < 0) {
+			pr_err("Cannot allocate new System RAM resource\n");
+			kfree(res);
+			return NULL;
+		}
 	}
 
 #ifdef CONFIG_SPARSEMEM
@@ -287,6 +331,7 @@ static struct resource *additional_memory_resource(phys_addr_t size)
 			pr_err("New System RAM resource outside addressable RAM (%lu > %lu)\n",
 			       pfn, limit);
 			release_memory_resource(res);
+			release_memory_resource(res_hostmem);
 			return NULL;
 		}
 	}
@@ -765,6 +810,8 @@ static int __init balloon_init(void)
 	set_online_page_callback(&xen_online_page);
 	register_memory_notifier(&xen_memory_nb);
 	register_sysctl_table(xen_root);
+
+	arch_xen_balloon_init(&hostmem_resource);
 #endif
 
 #ifdef CONFIG_XEN_PV
diff --git a/include/xen/balloon.h b/include/xen/balloon.h
index 4914b93..61f410f 100644
--- a/include/xen/balloon.h
+++ b/include/xen/balloon.h
@@ -44,3 +44,8 @@ static inline void xen_balloon_init(void)
 {
 }
 #endif
+
+#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG
+struct resource;
+void arch_xen_balloon_init(struct resource *hostmem_resource);
+#endif
-- 
2.7.4

Re: [PATCH v3] xen/balloon: Mark unallocated host memory as UNUSABLE

[Juergen Gross]

On 20/12/17 15:05, Boris Ostrovsky wrote:
> Commit f5775e0b6116 ("x86/xen: discard RAM regions above the maximum
> reservation") left host memory not assigned to dom0 as available for
> memory hotplug.
> 
> Unfortunately this also meant that those regions could be used by
> others. Specifically, commit fa564ad96366 ("x86/PCI: Enable a 64bit BAR
> on AMD Family 15h (Models 00-1f, 30-3f, 60-7f)") may try to map those
> addresses as MMIO.
> 
> To prevent this mark unallocated host memory as E820_TYPE_UNUSABLE (thus
> effectively reverting f5775e0b6116) and keep track of that region as
> a hostmem resource that can be used for the hotplug.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

Re: [PATCH v3] xen/balloon: Mark unallocated host memory as UNUSABLE

[Christian König]

Am 20.12.2017 um 15:05 schrieb Boris Ostrovsky:
> Commit f5775e0b6116 ("x86/xen: discard RAM regions above the maximum
> reservation") left host memory not assigned to dom0 as available for
> memory hotplug.
>
> Unfortunately this also meant that those regions could be used by
> others. Specifically, commit fa564ad96366 ("x86/PCI: Enable a 64bit BAR
> on AMD Family 15h (Models 00-1f, 30-3f, 60-7f)") may try to map those
> addresses as MMIO.
>
> To prevent this mark unallocated host memory as E820_TYPE_UNUSABLE (thus
> effectively reverting f5775e0b6116) and keep track of that region as
> a hostmem resource that can be used for the hotplug.
>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Acked-by: Christian König <christian.koenig@amd.com>

> ---
> Changes in v3:
> * Use PFN_PHYS
> * Replace kzalloc with kmalloc
> * Declare arch_xen_balloon_init prototype in balloon.h
> * Rename resources (s/memory/RAM/)
> * Clarify (I think) comment when populating hostmem_resource
> * Print open-ended interval on insert_resource() error
> * Constify declaration of struct e820_entry *entry
>
>   arch/x86/xen/enlighten.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++
>   arch/x86/xen/setup.c     |  6 ++--
>   drivers/xen/balloon.c    | 65 ++++++++++++++++++++++++++++++++------
>   include/xen/balloon.h    |  5 +++
>   4 files changed, 144 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index d669e9d..c9081c6 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -1,8 +1,12 @@
> +#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG
> +#include <linux/bootmem.h>
> +#endif
>   #include <linux/cpu.h>
>   #include <linux/kexec.h>
>   
>   #include <xen/features.h>
>   #include <xen/page.h>
> +#include <xen/interface/memory.h>
>   
>   #include <asm/xen/hypercall.h>
>   #include <asm/xen/hypervisor.h>
> @@ -331,3 +335,80 @@ void xen_arch_unregister_cpu(int num)
>   }
>   EXPORT_SYMBOL(xen_arch_unregister_cpu);
>   #endif
> +
> +#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG
> +void __init arch_xen_balloon_init(struct resource *hostmem_resource)
> +{
> +	struct xen_memory_map memmap;
> +	int rc;
> +	unsigned int i, last_guest_ram;
> +	phys_addr_t max_addr = PFN_PHYS(max_pfn);
> +	struct e820_table *xen_e820_table;
> +	const struct e820_entry *entry;
> +	struct resource *res;
> +
> +	if (!xen_initial_domain())
> +		return;
> +
> +	xen_e820_table = kmalloc(sizeof(*xen_e820_table), GFP_KERNEL);
> +	if (!xen_e820_table)
> +		return;
> +
> +	memmap.nr_entries = ARRAY_SIZE(xen_e820_table->entries);
> +	set_xen_guest_handle(memmap.buffer, xen_e820_table->entries);
> +	rc = HYPERVISOR_memory_op(XENMEM_machine_memory_map, &memmap);
> +	if (rc) {
> +		pr_warn("%s: Can't read host e820 (%d)\n", __func__, rc);
> +		goto out;
> +	}
> +
> +	last_guest_ram = 0;
> +	for (i = 0; i < memmap.nr_entries; i++) {
> +		if (xen_e820_table->entries[i].addr >= max_addr)
> +			break;
> +		if (xen_e820_table->entries[i].type == E820_TYPE_RAM)
> +			last_guest_ram = i;
> +	}
> +
> +	entry = &xen_e820_table->entries[last_guest_ram];
> +	if (max_addr >= entry->addr + entry->size)
> +		goto out; /* No unallocated host RAM. */
> +
> +	hostmem_resource->start = max_addr;
> +	hostmem_resource->end = entry->addr + entry->size;
> +
> +	/*
> +	 * Mark non-RAM regions between the end of dom0 RAM and end of host RAM
> +	 * as unavailable. The rest of that region can be used for hotplug-based
> +	 * ballooning.
> +	 */
> +	for (; i < memmap.nr_entries; i++) {
> +		entry = &xen_e820_table->entries[i];
> +
> +		if (entry->type == E820_TYPE_RAM)
> +			continue;
> +
> +		if (entry->addr >= hostmem_resource->end)
> +			break;
> +
> +		res = kzalloc(sizeof(*res), GFP_KERNEL);
> +		if (!res)
> +			goto out;
> +
> +		res->name = "Unavailable host RAM";
> +		res->start = entry->addr;
> +		res->end = (entry->addr + entry->size < hostmem_resource->end) ?
> +			    entry->addr + entry->size : hostmem_resource->end;
> +		rc = insert_resource(hostmem_resource, res);
> +		if (rc) {
> +			pr_warn("%s: Can't insert [%llx - %llx) (%d)\n",
> +				__func__, res->start, res->end, rc);
> +			kfree(res);
> +			goto  out;
> +		}
> +	}
> +
> + out:
> +	kfree(xen_e820_table);
> +}
> +#endif /* CONFIG_XEN_BALLOON_MEMORY_HOTPLUG */
> diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
> index c114ca7..6e0d208 100644
> --- a/arch/x86/xen/setup.c
> +++ b/arch/x86/xen/setup.c
> @@ -808,7 +808,6 @@ char * __init xen_memory_setup(void)
>   	addr = xen_e820_table.entries[0].addr;
>   	size = xen_e820_table.entries[0].size;
>   	while (i < xen_e820_table.nr_entries) {
> -		bool discard = false;
>   
>   		chunk_size = size;
>   		type = xen_e820_table.entries[i].type;
> @@ -824,11 +823,10 @@ char * __init xen_memory_setup(void)
>   				xen_add_extra_mem(pfn_s, n_pfns);
>   				xen_max_p2m_pfn = pfn_s + n_pfns;
>   			} else
> -				discard = true;
> +				type = E820_TYPE_UNUSABLE;
>   		}
>   
> -		if (!discard)
> -			xen_align_and_add_e820_region(addr, chunk_size, type);
> +		xen_align_and_add_e820_region(addr, chunk_size, type);
>   
>   		addr += chunk_size;
>   		size -= chunk_size;
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index f77e499..065f0b6 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -257,10 +257,25 @@ static void release_memory_resource(struct resource *resource)
>   	kfree(resource);
>   }
>   
> +/*
> + * Host memory not allocated to dom0. We can use this range for hotplug-based
> + * ballooning.
> + *
> + * It's a type-less resource. Setting IORESOURCE_MEM will make resource
> + * management algorithms (arch_remove_reservations()) look into guest e820,
> + * which we don't want.
> + */
> +static struct resource hostmem_resource = {
> +	.name   = "Host RAM",
> +};
> +
> +void __attribute__((weak)) __init arch_xen_balloon_init(struct resource *res)
> +{}
> +
>   static struct resource *additional_memory_resource(phys_addr_t size)
>   {
> -	struct resource *res;
> -	int ret;
> +	struct resource *res, *res_hostmem;
> +	int ret = -ENOMEM;
>   
>   	res = kzalloc(sizeof(*res), GFP_KERNEL);
>   	if (!res)
> @@ -269,13 +284,42 @@ static struct resource *additional_memory_resource(phys_addr_t size)
>   	res->name = "System RAM";
>   	res->flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
>   
> -	ret = allocate_resource(&iomem_resource, res,
> -				size, 0, -1,
> -				PAGES_PER_SECTION * PAGE_SIZE, NULL, NULL);
> -	if (ret < 0) {
> -		pr_err("Cannot allocate new System RAM resource\n");
> -		kfree(res);
> -		return NULL;
> +	res_hostmem = kzalloc(sizeof(*res), GFP_KERNEL);
> +	if (res_hostmem) {
> +		/* Try to grab a range from hostmem */
> +		res_hostmem->name = "Host memory";
> +		ret = allocate_resource(&hostmem_resource, res_hostmem,
> +					size, 0, -1,
> +					PAGES_PER_SECTION * PAGE_SIZE, NULL, NULL);
> +	}
> +
> +	if (!ret) {
> +		/*
> +		 * Insert this resource into iomem. Because hostmem_resource
> +		 * tracks portion of guest e820 marked as UNUSABLE noone else
> +		 * should try to use it.
> +		 */
> +		res->start = res_hostmem->start;
> +		res->end = res_hostmem->end;
> +		ret = insert_resource(&iomem_resource, res);
> +		if (ret < 0) {
> +			pr_err("Can't insert iomem_resource [%llx - %llx]\n",
> +				res->start, res->end);
> +			release_memory_resource(res_hostmem);
> +			res_hostmem = NULL;
> +			res->start = res->end = 0;
> +		}
> +	}
> +
> +	if (ret) {
> +		ret = allocate_resource(&iomem_resource, res,
> +					size, 0, -1,
> +					PAGES_PER_SECTION * PAGE_SIZE, NULL, NULL);
> +		if (ret < 0) {
> +			pr_err("Cannot allocate new System RAM resource\n");
> +			kfree(res);
> +			return NULL;
> +		}
>   	}
>   
>   #ifdef CONFIG_SPARSEMEM
> @@ -287,6 +331,7 @@ static struct resource *additional_memory_resource(phys_addr_t size)
>   			pr_err("New System RAM resource outside addressable RAM (%lu > %lu)\n",
>   			       pfn, limit);
>   			release_memory_resource(res);
> +			release_memory_resource(res_hostmem);
>   			return NULL;
>   		}
>   	}
> @@ -765,6 +810,8 @@ static int __init balloon_init(void)
>   	set_online_page_callback(&xen_online_page);
>   	register_memory_notifier(&xen_memory_nb);
>   	register_sysctl_table(xen_root);
> +
> +	arch_xen_balloon_init(&hostmem_resource);
>   #endif
>   
>   #ifdef CONFIG_XEN_PV
> diff --git a/include/xen/balloon.h b/include/xen/balloon.h
> index 4914b93..61f410f 100644
> --- a/include/xen/balloon.h
> +++ b/include/xen/balloon.h
> @@ -44,3 +44,8 @@ static inline void xen_balloon_init(void)
>   {
>   }
>   #endif
> +
> +#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG
> +struct resource;
> +void arch_xen_balloon_init(struct resource *hostmem_resource);
> +#endif

Re: [Xen-devel] [PATCH v3] xen/balloon: Mark unallocated host memory as UNUSABLE

[Igor Druzhinin]

On 20/12/2017 14:05, Boris Ostrovsky wrote:
> Commit f5775e0b6116 ("x86/xen: discard RAM regions above the maximum
> reservation") left host memory not assigned to dom0 as available for
> memory hotplug.
> 
> Unfortunately this also meant that those regions could be used by
> others. Specifically, commit fa564ad96366 ("x86/PCI: Enable a 64bit BAR
> on AMD Family 15h (Models 00-1f, 30-3f, 60-7f)") may try to map those
> addresses as MMIO.
> 
> To prevent this mark unallocated host memory as E820_TYPE_UNUSABLE (thus
> effectively reverting f5775e0b6116) and keep track of that region as
> a hostmem resource that can be used for the hotplug.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

This commit breaks Xen balloon memory hotplug for us in Dom0 with
"hoplug_unpopulated" set to 1. The issue is that the common kernel
memory onlining procedures require "System RAM" resource to be 1-st
level. That means by inserting it under "Unusable memory" as the commit
above does (intentionally or not) we make it 2-nd level and break memory
onlining.

There are multiple ways to fix it depending on what was the intention of
original commit and what exactly it tried to workaround. It seems it
does several things at once:
1) Marks non-Dom0 host memory "Unusable memory" in resource tree.
2) Keeps track of all the areas safe for hotplug in Dom0
3) Changes allocation algorithms itself in balloon driver to use those areas

Are all the things above necessary to cover the issue in fa564ad96366
("x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 00-1f, 30-3f,
60-7f)")?

Can we remove "Unusable memory" resources as soon as we finished
booting? Is removing on-demand is preferable over "shoot them all" in
that case?

Does it even make sense to remove the 1-st level only restriction in
kernel/resource.c ?

Igor

Re: [Xen-devel] [PATCH v3] xen/balloon: Mark unallocated host memory as UNUSABLE

[Boris Ostrovsky]

On 11/25/18 8:00 PM, Igor Druzhinin wrote:
> On 20/12/2017 14:05, Boris Ostrovsky wrote:
>> Commit f5775e0b6116 ("x86/xen: discard RAM regions above the maximum
>> reservation") left host memory not assigned to dom0 as available for
>> memory hotplug.
>>
>> Unfortunately this also meant that those regions could be used by
>> others. Specifically, commit fa564ad96366 ("x86/PCI: Enable a 64bit BAR
>> on AMD Family 15h (Models 00-1f, 30-3f, 60-7f)") may try to map those
>> addresses as MMIO.
>>
>> To prevent this mark unallocated host memory as E820_TYPE_UNUSABLE (thus
>> effectively reverting f5775e0b6116) and keep track of that region as
>> a hostmem resource that can be used for the hotplug.
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> This commit breaks Xen balloon memory hotplug for us in Dom0 with
> "hoplug_unpopulated" set to 1. The issue is that the common kernel
> memory onlining procedures require "System RAM" resource to be 1-st
> level. That means by inserting it under "Unusable memory" as the commit
> above does (intentionally or not) we make it 2-nd level and break memory
> onlining.

What do you mean by 1st and 2nd level?



>
> There are multiple ways to fix it depending on what was the intention of
> original commit and what exactly it tried to workaround. It seems it
> does several things at once:
> 1) Marks non-Dom0 host memory "Unusable memory" in resource tree.
> 2) Keeps track of all the areas safe for hotplug in Dom0
> 3) Changes allocation algorithms itself in balloon driver to use those areas

Pretty much. (3) is true in the sense that memory is first allocated
from hostmem_resource (which is non-dom0 RAM).


>
> Are all the things above necessary to cover the issue in fa564ad96366
> ("x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 00-1f, 30-3f,
> 60-7f)")?

Not anymore, as far as that particular commit is concerned, but that's
because of 03a551734 ("x86/PCI: Move and shrink AMD 64-bit window to
avoid conflict") which was introduced after balloon patch. IIRC there
were some issues with fa564ad96366unrelated to balloon.


>
> Can we remove "Unusable memory" resources as soon as we finished
> booting? Is removing on-demand is preferable over "shoot them all" in
> that case?

The concern is that in principle nothing prevents someone else to do
exact same thing fa564ad96366 did, which is grab something from right
above end of RAM as the kernel sees it. And that can be done at any point.


-boris

>
> Does it even make sense to remove the 1-st level only restriction in
> kernel/resource.c ?

Re: [Xen-devel] [PATCH v3] xen/balloon: Mark unallocated host memory as UNUSABLE

[Igor Druzhinin]

On 26/11/2018 16:25, Boris Ostrovsky wrote:
> On 11/25/18 8:00 PM, Igor Druzhinin wrote:
>> On 20/12/2017 14:05, Boris Ostrovsky wrote:
>>> Commit f5775e0b6116 ("x86/xen: discard RAM regions above the maximum
>>> reservation") left host memory not assigned to dom0 as available for
>>> memory hotplug.
>>>
>>> Unfortunately this also meant that those regions could be used by
>>> others. Specifically, commit fa564ad96366 ("x86/PCI: Enable a 64bit BAR
>>> on AMD Family 15h (Models 00-1f, 30-3f, 60-7f)") may try to map those
>>> addresses as MMIO.
>>>
>>> To prevent this mark unallocated host memory as E820_TYPE_UNUSABLE (thus
>>> effectively reverting f5775e0b6116) and keep track of that region as
>>> a hostmem resource that can be used for the hotplug.
>>>
>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> This commit breaks Xen balloon memory hotplug for us in Dom0 with
>> "hoplug_unpopulated" set to 1. The issue is that the common kernel
>> memory onlining procedures require "System RAM" resource to be 1-st
>> level. That means by inserting it under "Unusable memory" as the commit
>> above does (intentionally or not) we make it 2-nd level and break memory
>> onlining.
> 
> What do you mean by 1st and 2nd level?
> 

I mean the level of a resource in IOMEM tree (the one that's printed
from /proc/iomem). 1-st level means its parent is root and so on.

>>
>> There are multiple ways to fix it depending on what was the intention of
>> original commit and what exactly it tried to workaround. It seems it
>> does several things at once:
>> 1) Marks non-Dom0 host memory "Unusable memory" in resource tree.
>> 2) Keeps track of all the areas safe for hotplug in Dom0
>> 3) Changes allocation algorithms itself in balloon driver to use those areas
> 
> Pretty much. (3) is true in the sense that memory is first allocated
> from hostmem_resource (which is non-dom0 RAM).
> 
>>
>> Are all the things above necessary to cover the issue in fa564ad96366
>> ("x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 00-1f, 30-3f,
>> 60-7f)")?
> 
> Not anymore, as far as that particular commit is concerned, but that's
> because of 03a551734 ("x86/PCI: Move and shrink AMD 64-bit window to
> avoid conflict") which was introduced after balloon patch. IIRC there
> were some issues with fa564ad96366 unrelated to balloon.
> 

If it's not a problem anymore IIUC, can we revert the change as it still
breaks "hotplug_unpopulated=1" for the reasons I described above?

> 
>>
>> Can we remove "Unusable memory" resources as soon as we finished
>> booting? Is removing on-demand is preferable over "shoot them all" in
>> that case?
> 
> The concern is that in principle nothing prevents someone else to do
> exact same thing fa564ad96366 did, which is grab something from right
> above end of RAM as the kernel sees it. And that can be done at any point.
> 

Nothing prevents - true, but that's plainly wrong from OS point of view
to grab physical ranges for something without knowing what's actually
behind on that platform. I think we shouldn't consider this as a valid
thing to do and don't try to workaround initially incorrect code.

> 
> -boris
> 
>>
>> Does it even make sense to remove the 1-st level only restriction in
>> kernel/resource.c ?
> 
> 
> 

Re: [Xen-devel] [PATCH v3] xen/balloon: Mark unallocated host memory as UNUSABLE

[Boris Ostrovsky]

On 11/26/18 12:10 PM, Igor Druzhinin wrote:
> On 26/11/2018 16:25, Boris Ostrovsky wrote:
>> On 11/25/18 8:00 PM, Igor Druzhinin wrote:
>>> On 20/12/2017 14:05, Boris Ostrovsky wrote:
>>>> Commit f5775e0b6116 ("x86/xen: discard RAM regions above the maximum
>>>> reservation") left host memory not assigned to dom0 as available for
>>>> memory hotplug.
>>>>
>>>> Unfortunately this also meant that those regions could be used by
>>>> others. Specifically, commit fa564ad96366 ("x86/PCI: Enable a 64bit BAR
>>>> on AMD Family 15h (Models 00-1f, 30-3f, 60-7f)") may try to map those
>>>> addresses as MMIO.
>>>>
>>>> To prevent this mark unallocated host memory as E820_TYPE_UNUSABLE (thus
>>>> effectively reverting f5775e0b6116) and keep track of that region as
>>>> a hostmem resource that can be used for the hotplug.
>>>>
>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>> This commit breaks Xen balloon memory hotplug for us in Dom0 with
>>> "hoplug_unpopulated" set to 1. The issue is that the common kernel
>>> memory onlining procedures require "System RAM" resource to be 1-st
>>> level. That means by inserting it under "Unusable memory" as the commit
>>> above does (intentionally or not) we make it 2-nd level and break memory
>>> onlining.
>> What do you mean by 1st and 2nd level?
>>
> I mean the level of a resource in IOMEM tree (the one that's printed
> from /proc/iomem). 1-st level means its parent is root and so on.

Ah, OK. Doesn't
additional_memory_resource()->insert_resource(iomem_resource) place the
RAM at 1st level? And if not, can we make it so?

>
>>> There are multiple ways to fix it depending on what was the intention of
>>> original commit and what exactly it tried to workaround. It seems it
>>> does several things at once:
>>> 1) Marks non-Dom0 host memory "Unusable memory" in resource tree.
>>> 2) Keeps track of all the areas safe for hotplug in Dom0
>>> 3) Changes allocation algorithms itself in balloon driver to use those areas
>> Pretty much. (3) is true in the sense that memory is first allocated
>> from hostmem_resource (which is non-dom0 RAM).
>>
>>> Are all the things above necessary to cover the issue in fa564ad96366
>>> ("x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 00-1f, 30-3f,
>>> 60-7f)")?
>> Not anymore, as far as that particular commit is concerned, but that's
>> because of 03a551734 ("x86/PCI: Move and shrink AMD 64-bit window to
>> avoid conflict") which was introduced after balloon patch. IIRC there
>> were some issues with fa564ad96366 unrelated to balloon.
>>
> If it's not a problem anymore IIUC, can we revert the change as it still
> breaks "hotplug_unpopulated=1" for the reasons I described above?

Since this seems to have broken existing feature this would be an
option. But before going that route I'd like to see if we can fix the patch.

I have been unable to reproduce your problem. Can you describe what you did?


>
>>> Can we remove "Unusable memory" resources as soon as we finished
>>> booting? Is removing on-demand is preferable over "shoot them all" in
>>> that case?
>> The concern is that in principle nothing prevents someone else to do
>> exact same thing fa564ad96366 did, which is grab something from right
>> above end of RAM as the kernel sees it. And that can be done at any point.
>>
> Nothing prevents - true, but that's plainly wrong from OS point of view
> to grab physical ranges for something without knowing what's actually
> behind on that platform. 

I am not sure I agree that this is plainly wrong. If not for BIOS issues
that 03a551734cf mentions I think what the original implementation of
fa564ad963 did was perfectly reasonable. Which is why I would prefer to
keep keep the hostmem resource *if possible*.


-boris


> I think we shouldn't consider this as a valid
> thing to do and don't try to workaround initially incorrect code.
>
>> -boris
>>
>>> Does it even make sense to remove the 1-st level only restriction in
>>> kernel/resource.c ?
>>
>>

Re: [Xen-devel] [PATCH v3] xen/balloon: Mark unallocated host memory as UNUSABLE

[Igor Druzhinin]

On 26/11/2018 19:42, Boris Ostrovsky wrote:
> On 11/26/18 12:10 PM, Igor Druzhinin wrote:
>> On 26/11/2018 16:25, Boris Ostrovsky wrote:
>>> On 11/25/18 8:00 PM, Igor Druzhinin wrote:
>>>> On 20/12/2017 14:05, Boris Ostrovsky wrote:
>>>>> Commit f5775e0b6116 ("x86/xen: discard RAM regions above the maximum
>>>>> reservation") left host memory not assigned to dom0 as available for
>>>>> memory hotplug.
>>>>>
>>>>> Unfortunately this also meant that those regions could be used by
>>>>> others. Specifically, commit fa564ad96366 ("x86/PCI: Enable a 64bit BAR
>>>>> on AMD Family 15h (Models 00-1f, 30-3f, 60-7f)") may try to map those
>>>>> addresses as MMIO.
>>>>>
>>>>> To prevent this mark unallocated host memory as E820_TYPE_UNUSABLE (thus
>>>>> effectively reverting f5775e0b6116) and keep track of that region as
>>>>> a hostmem resource that can be used for the hotplug.
>>>>>
>>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>> This commit breaks Xen balloon memory hotplug for us in Dom0 with
>>>> "hoplug_unpopulated" set to 1. The issue is that the common kernel
>>>> memory onlining procedures require "System RAM" resource to be 1-st
>>>> level. That means by inserting it under "Unusable memory" as the commit
>>>> above does (intentionally or not) we make it 2-nd level and break memory
>>>> onlining.
>>> What do you mean by 1st and 2nd level?
>>>
>> I mean the level of a resource in IOMEM tree (the one that's printed
>> from /proc/iomem). 1-st level means its parent is root and so on.
> 
> Ah, OK. Doesn't
> additional_memory_resource()->insert_resource(iomem_resource) place the
> RAM at 1st level? And if not, can we make it so?
> 

That'd mean splitting "Unusable memory" resource. Since it's allocated
from bootmem it has proven to be quite difficult but there are seem to
be special functions available particularly for memory resource
management operations that I've not yet experimented with. So the answer
is probably - maybe yes but not straightforward.

>>
>>>> There are multiple ways to fix it depending on what was the intention of
>>>> original commit and what exactly it tried to workaround. It seems it
>>>> does several things at once:
>>>> 1) Marks non-Dom0 host memory "Unusable memory" in resource tree.
>>>> 2) Keeps track of all the areas safe for hotplug in Dom0
>>>> 3) Changes allocation algorithms itself in balloon driver to use those areas
>>> Pretty much. (3) is true in the sense that memory is first allocated
>>> from hostmem_resource (which is non-dom0 RAM).
>>>
>>>> Are all the things above necessary to cover the issue in fa564ad96366
>>>> ("x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 00-1f, 30-3f,
>>>> 60-7f)")?
>>> Not anymore, as far as that particular commit is concerned, but that's
>>> because of 03a551734 ("x86/PCI: Move and shrink AMD 64-bit window to
>>> avoid conflict") which was introduced after balloon patch. IIRC there
>>> were some issues with fa564ad96366 unrelated to balloon.
>>>
>> If it's not a problem anymore IIUC, can we revert the change as it still
>> breaks "hotplug_unpopulated=1" for the reasons I described above?
> 
> Since this seems to have broken existing feature this would be an
> option. But before going that route I'd like to see if we can fix the patch.
> 
> I have been unable to reproduce your problem. Can you describe what you did?
> 

It doesn't happen on all configurations as sometimes the memory is
successfully hotplugged to a hole depending on the size of Dom0 memory.
But we reproduced it quite reliably with small Dom0 sizes like 752MB.

XenServer is using this feature to hotplug additional memory for grant
table operations so we started a VM and observed a stable hang.

> 
>>
>>>> Can we remove "Unusable memory" resources as soon as we finished
>>>> booting? Is removing on-demand is preferable over "shoot them all" in
>>>> that case?
>>> The concern is that in principle nothing prevents someone else to do
>>> exact same thing fa564ad96366 did, which is grab something from right
>>> above end of RAM as the kernel sees it. And that can be done at any point.
>>>
>> Nothing prevents - true, but that's plainly wrong from OS point of view
>> to grab physical ranges for something without knowing what's actually
>> behind on that platform. 
> 
> I am not sure I agree that this is plainly wrong. If not for BIOS issues
> that 03a551734cf mentions I think what the original implementation of
> fa564ad963 did was perfectly reasonable. Which is why I would prefer to
> keep keep the hostmem resource *if possible*.
> 

Exactly, those *are* BIOS issues and are not supposed to be workarounded
by the OS. And as the next commit showed even the workaround didn't
quite helped with it.

I agree that having hotmem as a precaution is fine but only if there is
a non-cringy way to keep things working with it which I'm not sure does
exist.

Igor

> 
> -boris
> 
> 
>> I think we shouldn't consider this as a valid
>> thing to do and don't try to workaround initially incorrect code.
>>
>>> -boris
>>>
>>>> Does it even make sense to remove the 1-st level only restriction in
>>>> kernel/resource.c ?
>>>
>>>
> 

Re: [Xen-devel] [PATCH v3] xen/balloon: Mark unallocated host memory as UNUSABLE

[Boris Ostrovsky]

On 11/26/18 2:57 PM, Igor Druzhinin wrote:
> On 26/11/2018 19:42, Boris Ostrovsky wrote:
>> On 11/26/18 12:10 PM, Igor Druzhinin wrote:
>>> On 26/11/2018 16:25, Boris Ostrovsky wrote:
>>>> On 11/25/18 8:00 PM, Igor Druzhinin wrote:
>>>>> On 20/12/2017 14:05, Boris Ostrovsky wrote:
>>>>>> Commit f5775e0b6116 ("x86/xen: discard RAM regions above the maximum
>>>>>> reservation") left host memory not assigned to dom0 as available for
>>>>>> memory hotplug.
>>>>>>
>>>>>> Unfortunately this also meant that those regions could be used by
>>>>>> others. Specifically, commit fa564ad96366 ("x86/PCI: Enable a 64bit BAR
>>>>>> on AMD Family 15h (Models 00-1f, 30-3f, 60-7f)") may try to map those
>>>>>> addresses as MMIO.
>>>>>>
>>>>>> To prevent this mark unallocated host memory as E820_TYPE_UNUSABLE (thus
>>>>>> effectively reverting f5775e0b6116) and keep track of that region as
>>>>>> a hostmem resource that can be used for the hotplug.
>>>>>>
>>>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>>> This commit breaks Xen balloon memory hotplug for us in Dom0 with
>>>>> "hoplug_unpopulated" set to 1. The issue is that the common kernel
>>>>> memory onlining procedures require "System RAM" resource to be 1-st
>>>>> level. That means by inserting it under "Unusable memory" as the commit
>>>>> above does (intentionally or not) we make it 2-nd level and break memory
>>>>> onlining.
>>>> What do you mean by 1st and 2nd level?
>>>>
>>> I mean the level of a resource in IOMEM tree (the one that's printed
>>> from /proc/iomem). 1-st level means its parent is root and so on.
>> Ah, OK. Doesn't
>> additional_memory_resource()->insert_resource(iomem_resource) place the
>> RAM at 1st level? And if not, can we make it so?
>>
> That'd mean splitting "Unusable memory" resource. Since it's allocated
> from bootmem it has proven to be quite difficult but there are seem to
> be special functions available particularly for memory resource
> management operations that I've not yet experimented with. So the answer
> is probably - maybe yes but not straightforward.
>
>>>>> There are multiple ways to fix it depending on what was the intention of
>>>>> original commit and what exactly it tried to workaround. It seems it
>>>>> does several things at once:
>>>>> 1) Marks non-Dom0 host memory "Unusable memory" in resource tree.
>>>>> 2) Keeps track of all the areas safe for hotplug in Dom0
>>>>> 3) Changes allocation algorithms itself in balloon driver to use those areas
>>>> Pretty much. (3) is true in the sense that memory is first allocated
>>>> from hostmem_resource (which is non-dom0 RAM).
>>>>
>>>>> Are all the things above necessary to cover the issue in fa564ad96366
>>>>> ("x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 00-1f, 30-3f,
>>>>> 60-7f)")?
>>>> Not anymore, as far as that particular commit is concerned, but that's
>>>> because of 03a551734 ("x86/PCI: Move and shrink AMD 64-bit window to
>>>> avoid conflict") which was introduced after balloon patch. IIRC there
>>>> were some issues with fa564ad96366 unrelated to balloon.
>>>>
>>> If it's not a problem anymore IIUC, can we revert the change as it still
>>> breaks "hotplug_unpopulated=1" for the reasons I described above?
>> Since this seems to have broken existing feature this would be an
>> option. But before going that route I'd like to see if we can fix the patch.
>>
>> I have been unable to reproduce your problem. Can you describe what you did?
>>
> It doesn't happen on all configurations as sometimes the memory is
> successfully hotplugged to a hole depending on the size of Dom0 memory.
> But we reproduced it quite reliably with small Dom0 sizes like 752MB.
>
> XenServer is using this feature to hotplug additional memory for grant
> table operations so we started a VM and observed a stable hang.
>
>>>>> Can we remove "Unusable memory" resources as soon as we finished
>>>>> booting? Is removing on-demand is preferable over "shoot them all" in
>>>>> that case?
>>>> The concern is that in principle nothing prevents someone else to do
>>>> exact same thing fa564ad96366 did, which is grab something from right
>>>> above end of RAM as the kernel sees it. And that can be done at any point.
>>>>
>>> Nothing prevents - true, but that's plainly wrong from OS point of view
>>> to grab physical ranges for something without knowing what's actually
>>> behind on that platform. 
>> I am not sure I agree that this is plainly wrong. If not for BIOS issues
>> that 03a551734cf mentions I think what the original implementation of
>> fa564ad963 did was perfectly reasonable. Which is why I would prefer to
>> keep keep the hostmem resource *if possible*.
>>
> Exactly, those *are* BIOS issues and are not supposed to be workarounded
> by the OS. And as the next commit showed even the workaround didn't
> quite helped with it.
>
> I agree that having hotmem as a precaution is fine but only if there is
> a non-cringy way to keep things working with it which I'm not sure does
> exist.

We have most of the interfaces in the resource framework to do what we
want. I put together a semi-working prototype but the tricky part is
resource locking --- we need to remove a chunk from hostmem (which will
cause hostmem to be resized and possibly split), and insert this chunk
to iomem's top level as System RAM, all while holding resource_lock.

I haven't been able to come up with an acceptable interface for that.

Given that we are actually broken I guess I am OK with reverting the
patch, but please make sure this works on AMD boxes (I think family 15h
is what needs to be tested).

-boris



>
> Igor
>
>> -boris
>>
>>
>>> I think we shouldn't consider this as a valid
>>> thing to do and don't try to workaround initially incorrect code.
>>>
>>>> -boris
>>>>
>>>>> Does it even make sense to remove the 1-st level only restriction in
>>>>> kernel/resource.c ?
>>>>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3] xen/balloon: Mark unallocated host memory as UNUSABLE

[Igor Druzhinin]

On 27/11/2018 03:28, Boris Ostrovsky wrote:
> On 11/26/18 2:57 PM, Igor Druzhinin wrote:
>> On 26/11/2018 19:42, Boris Ostrovsky wrote:
>>> On 11/26/18 12:10 PM, Igor Druzhinin wrote:
>>>> On 26/11/2018 16:25, Boris Ostrovsky wrote:
>>>>> On 11/25/18 8:00 PM, Igor Druzhinin wrote:
>>>>>> On 20/12/2017 14:05, Boris Ostrovsky wrote:
>>>>>>> Commit f5775e0b6116 ("x86/xen: discard RAM regions above the maximum
>>>>>>> reservation") left host memory not assigned to dom0 as available for
>>>>>>> memory hotplug.
>>>>>>>
>>>>>>> Unfortunately this also meant that those regions could be used by
>>>>>>> others. Specifically, commit fa564ad96366 ("x86/PCI: Enable a 64bit BAR
>>>>>>> on AMD Family 15h (Models 00-1f, 30-3f, 60-7f)") may try to map those
>>>>>>> addresses as MMIO.
>>>>>>>
>>>>>>> To prevent this mark unallocated host memory as E820_TYPE_UNUSABLE (thus
>>>>>>> effectively reverting f5775e0b6116) and keep track of that region as
>>>>>>> a hostmem resource that can be used for the hotplug.
>>>>>>>
>>>>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>>>> This commit breaks Xen balloon memory hotplug for us in Dom0 with
>>>>>> "hoplug_unpopulated" set to 1. The issue is that the common kernel
>>>>>> memory onlining procedures require "System RAM" resource to be 1-st
>>>>>> level. That means by inserting it under "Unusable memory" as the commit
>>>>>> above does (intentionally or not) we make it 2-nd level and break memory
>>>>>> onlining.
>>>>> What do you mean by 1st and 2nd level?
>>>>>
>>>> I mean the level of a resource in IOMEM tree (the one that's printed
>>>> from /proc/iomem). 1-st level means its parent is root and so on.
>>> Ah, OK. Doesn't
>>> additional_memory_resource()->insert_resource(iomem_resource) place the
>>> RAM at 1st level? And if not, can we make it so?
>>>
>> That'd mean splitting "Unusable memory" resource. Since it's allocated
>> from bootmem it has proven to be quite difficult but there are seem to
>> be special functions available particularly for memory resource
>> management operations that I've not yet experimented with. So the answer
>> is probably - maybe yes but not straightforward.
>>
>>>>>> There are multiple ways to fix it depending on what was the intention of
>>>>>> original commit and what exactly it tried to workaround. It seems it
>>>>>> does several things at once:
>>>>>> 1) Marks non-Dom0 host memory "Unusable memory" in resource tree.
>>>>>> 2) Keeps track of all the areas safe for hotplug in Dom0
>>>>>> 3) Changes allocation algorithms itself in balloon driver to use those areas
>>>>> Pretty much. (3) is true in the sense that memory is first allocated
>>>>> from hostmem_resource (which is non-dom0 RAM).
>>>>>
>>>>>> Are all the things above necessary to cover the issue in fa564ad96366
>>>>>> ("x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 00-1f, 30-3f,
>>>>>> 60-7f)")?
>>>>> Not anymore, as far as that particular commit is concerned, but that's
>>>>> because of 03a551734 ("x86/PCI: Move and shrink AMD 64-bit window to
>>>>> avoid conflict") which was introduced after balloon patch. IIRC there
>>>>> were some issues with fa564ad96366 unrelated to balloon.
>>>>>
>>>> If it's not a problem anymore IIUC, can we revert the change as it still
>>>> breaks "hotplug_unpopulated=1" for the reasons I described above?
>>> Since this seems to have broken existing feature this would be an
>>> option. But before going that route I'd like to see if we can fix the patch.
>>>
>>> I have been unable to reproduce your problem. Can you describe what you did?
>>>
>> It doesn't happen on all configurations as sometimes the memory is
>> successfully hotplugged to a hole depending on the size of Dom0 memory.
>> But we reproduced it quite reliably with small Dom0 sizes like 752MB.
>>
>> XenServer is using this feature to hotplug additional memory for grant
>> table operations so we started a VM and observed a stable hang.
>>
>>>>>> Can we remove "Unusable memory" resources as soon as we finished
>>>>>> booting? Is removing on-demand is preferable over "shoot them all" in
>>>>>> that case?
>>>>> The concern is that in principle nothing prevents someone else to do
>>>>> exact same thing fa564ad96366 did, which is grab something from right
>>>>> above end of RAM as the kernel sees it. And that can be done at any point.
>>>>>
>>>> Nothing prevents - true, but that's plainly wrong from OS point of view
>>>> to grab physical ranges for something without knowing what's actually
>>>> behind on that platform. 
>>> I am not sure I agree that this is plainly wrong. If not for BIOS issues
>>> that 03a551734cf mentions I think what the original implementation of
>>> fa564ad963 did was perfectly reasonable. Which is why I would prefer to
>>> keep keep the hostmem resource *if possible*.
>>>
>> Exactly, those *are* BIOS issues and are not supposed to be workarounded
>> by the OS. And as the next commit showed even the workaround didn't
>> quite helped with it.
>>
>> I agree that having hotmem as a precaution is fine but only if there is
>> a non-cringy way to keep things working with it which I'm not sure does
>> exist.
> 
> We have most of the interfaces in the resource framework to do what we
> want. I put together a semi-working prototype but the tricky part is
> resource locking --- we need to remove a chunk from hostmem (which will
> cause hostmem to be resized and possibly split), and insert this chunk
> to iomem's top level as System RAM, all while holding resource_lock.
> 
> I haven't been able to come up with an acceptable interface for that.
> 
> Given that we are actually broken I guess I am OK with reverting the
> patch, but please make sure this works on AMD boxes (I think family 15h
> is what needs to be tested).
> 

After their last commit I don't see how this can be broken:
1) They only claim addresses starting from 0xbd00000000
*unconditionally* which means if there is some memory behind this range
on the host (regardless if it's Dom0 or native Linux) they'll break
their own systems.

2) So, theoretically, to trigger the original issue we'd need to have a
system with RAM higher than 0xbd00000000 and that shouldn't be assigned
to Dom0 but that contradicts (1).

Igor

>>
>>> -boris
>>>
>>>
>>>> I think we shouldn't consider this as a valid
>>>> thing to do and don't try to workaround initially incorrect code.
>>>>
>>>>> -boris
>>>>>
>>>>>> Does it even make sense to remove the 1-st level only restriction in
>>>>>> kernel/resource.c ?
>>>>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xenproject.org
>> https://lists.xenproject.org/mailman/listinfo/xen-devel
>