linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE
@ 2017-12-18 22:22 Boris Ostrovsky
  2017-12-19  8:22 ` Juergen Gross
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Boris Ostrovsky @ 2017-12-18 22:22 UTC (permalink / raw)
  To: xen-devel, linux-kernel
  Cc: jgross, helgaas, christian.koenig, 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 v2:

In enlighten.c:
- Fix 32-bit build problem (include bootmem.h), make variables 32-bit safe
- Add a test to avoid inserting a resource into hostmem which is beyond
  hostmem's end
- Replace 'while' loop with 'for' to simplify array boundary check.
- Drop out of memory warnings
- Add a comment clarifying use of hostmem_resource.

 arch/x86/xen/enlighten.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++
 arch/x86/xen/setup.c     |  6 ++--
 drivers/xen/balloon.c    | 65 ++++++++++++++++++++++++++++++++++------
 3 files changed, 135 insertions(+), 13 deletions(-)

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index d669e9d..ac96142 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,76 @@ 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 = max_pfn << PAGE_SHIFT;
+	struct e820_table *xen_e820_table;
+	struct e820_entry *entry;
+	struct resource *res;
+
+	if (!xen_initial_domain())
+		return;
+
+	xen_e820_table = kzalloc(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 as not available. */
+	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 = "Host memory";
+		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..fb5aa7c 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 memory",
+};
+
+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
-- 
2.7.4

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

* Re: [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE
  2017-12-18 22:22 [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE Boris Ostrovsky
@ 2017-12-19  8:22 ` Juergen Gross
  2017-12-19  8:23 ` [Xen-devel] " Jan Beulich
       [not found] ` <5A38DA840200007800198579@suse.com>
  2 siblings, 0 replies; 10+ messages in thread
From: Juergen Gross @ 2017-12-19  8:22 UTC (permalink / raw)
  To: Boris Ostrovsky, xen-devel, linux-kernel; +Cc: helgaas, christian.koenig

On 18/12/17 23:22, 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>
> ---
> Changes in v2:
> 
> In enlighten.c:
> - Fix 32-bit build problem (include bootmem.h), make variables 32-bit safe
> - Add a test to avoid inserting a resource into hostmem which is beyond
>   hostmem's end
> - Replace 'while' loop with 'for' to simplify array boundary check.
> - Drop out of memory warnings
> - Add a comment clarifying use of hostmem_resource.
> 
>  arch/x86/xen/enlighten.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++
>  arch/x86/xen/setup.c     |  6 ++--
>  drivers/xen/balloon.c    | 65 ++++++++++++++++++++++++++++++++++------
>  3 files changed, 135 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index d669e9d..ac96142 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,76 @@ 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)

Sorry for noticing only now, but shouldn't you add a prototype in some
header for this function?

> +{
> +	struct xen_memory_map memmap;
> +	int rc;
> +	unsigned int i, last_guest_ram;
> +	phys_addr_t max_addr = max_pfn << PAGE_SHIFT;

Using PFN_PHYS() would have avoided the still present overflow on 32-bit
kernel: the shift is done with the 32-bit quantity and only then the
result is promoted to 64-bit.


Juergen

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

* Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE
  2017-12-18 22:22 [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE Boris Ostrovsky
  2017-12-19  8:22 ` Juergen Gross
@ 2017-12-19  8:23 ` Jan Beulich
  2017-12-19 14:25   ` Boris Ostrovsky
       [not found] ` <5A38DA840200007800198579@suse.com>
  2 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2017-12-19  8:23 UTC (permalink / raw)
  To: Boris Ostrovsky
  Cc: christian.koenig, helgaas, xen-devel, Juergen Gross, linux-kernel

>>> On 18.12.17 at 23:22, <boris.ostrovsky@oracle.com> wrote:
> +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 = max_pfn << PAGE_SHIFT;

PFN_PHYS() as right now you still have an issue on 32-bit.

> +	struct e820_table *xen_e820_table;
> +	struct e820_entry *entry;

const?

> +	struct resource *res;
> +
> +	if (!xen_initial_domain())
> +		return;
> +
> +	xen_e820_table = kzalloc(sizeof(*xen_e820_table), GFP_KERNEL);

Wouldn't kmalloc() suffice here?

> +	if (!xen_e820_table)
> +		return;

Not saying "out of memory" here is certainly fine, but shouldn't
there nevertheless be a warning, as failure to go through the
rest of the function will impact overall functionality?

> +	memmap.nr_entries = ARRAY_SIZE(xen_e820_table->entries);

Is it really reasonable to have a static upper bound here? As we
know especially EFI systems can come with a pretty scattered
(pseudo) E820 table. Even if (iirc) this has a static upper bound
right now in the hypervisor too, it would be nice if the kernel
didn't need further changes once the hypervisor is being made
more flexible.

> +	/* Mark non-RAM regions as not available. */
> +	for (; i < memmap.nr_entries; i++) {
> +		entry = &xen_e820_table->entries[i];
> +
> +		if (entry->type == E820_TYPE_RAM)
> +			continue;

I can't seem to match up this with ...

> +		if (entry->addr >= hostmem_resource->end)
> +			break;
> +
> +		res = kzalloc(sizeof(*res), GFP_KERNEL);
> +		if (!res)
> +			goto out;
> +
> +		res->name = "Host memory";

... this. Do you mean != instead (with the comment ahead of the
loop also clarified, saying something like "host RAM regions which
aren't RAM for us")? And perhaps better "Host RAM"?

> +		rc = insert_resource(hostmem_resource, res);
> +		if (rc) {
> +			pr_warn("%s: Can't insert [%llx - %llx] (%d)\n",

[%llx,%llx) ? Plus won't "ll" cause issues with 32-bit non-PAE builds?
(Same issues somewhere further down.)

> +				__func__, res->start, res->end, rc);
> +			kfree(res);
> +			goto  out;

Perhaps better not to bail out of the loop here (at least if rc is
not -ENOMEM)?

Jan

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

* Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE
       [not found] ` <5A38DA840200007800198579@suse.com>
@ 2017-12-19  9:21   ` Juergen Gross
  2017-12-19  9:27     ` Jan Beulich
       [not found]     ` <5A38E9A502000078001985CF@suse.com>
  0 siblings, 2 replies; 10+ messages in thread
From: Juergen Gross @ 2017-12-19  9:21 UTC (permalink / raw)
  To: Jan Beulich, Boris Ostrovsky
  Cc: christian.koenig, helgaas, xen-devel, linux-kernel

On 19/12/17 09:23, Jan Beulich wrote:
>>>> On 18.12.17 at 23:22, <boris.ostrovsky@oracle.com> wrote:
>> +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 = max_pfn << PAGE_SHIFT;
> 
> PFN_PHYS() as right now you still have an issue on 32-bit.

Why? PFN_PHYS is defined as:

#define PFN_PHYS(x) ((phys_addr_t)(x) << PAGE_SHIFT)


Juergen

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

* Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE
  2017-12-19  9:21   ` Juergen Gross
@ 2017-12-19  9:27     ` Jan Beulich
       [not found]     ` <5A38E9A502000078001985CF@suse.com>
  1 sibling, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2017-12-19  9:27 UTC (permalink / raw)
  To: Juergen Gross
  Cc: christian.koenig, helgaas, xen-devel, Boris Ostrovsky, linux-kernel

>>> On 19.12.17 at 10:21, <jgross@suse.com> wrote:
> On 19/12/17 09:23, Jan Beulich wrote:
>>>>> On 18.12.17 at 23:22, <boris.ostrovsky@oracle.com> wrote:
>>> +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 = max_pfn << PAGE_SHIFT;
>> 
>> PFN_PHYS() as right now you still have an issue on 32-bit.
> 
> Why? PFN_PHYS is defined as:
> 
> #define PFN_PHYS(x) ((phys_addr_t)(x) << PAGE_SHIFT)

Well, that's why I suggested its use (just like you did in your
own review). IOW - now I'm confused.

Jan

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

* Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE
       [not found]     ` <5A38E9A502000078001985CF@suse.com>
@ 2017-12-19 10:08       ` Juergen Gross
  0 siblings, 0 replies; 10+ messages in thread
From: Juergen Gross @ 2017-12-19 10:08 UTC (permalink / raw)
  To: Jan Beulich
  Cc: christian.koenig, helgaas, xen-devel, Boris Ostrovsky, linux-kernel

On 19/12/17 10:27, Jan Beulich wrote:
>>>> On 19.12.17 at 10:21, <jgross@suse.com> wrote:
>> On 19/12/17 09:23, Jan Beulich wrote:
>>>>>> On 18.12.17 at 23:22, <boris.ostrovsky@oracle.com> wrote:
>>>> +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 = max_pfn << PAGE_SHIFT;
>>>
>>> PFN_PHYS() as right now you still have an issue on 32-bit.
>>
>> Why? PFN_PHYS is defined as:
>>
>> #define PFN_PHYS(x) ((phys_addr_t)(x) << PAGE_SHIFT)
> 
> Well, that's why I suggested its use (just like you did in your
> own review). IOW - now I'm confused.

Sorry, just got your answer wrong.

As I had already found the same issue somehow I assumked this remark
would be referencing my review. Sorry for the noise.


Juergen

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

* Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE
  2017-12-19  8:23 ` [Xen-devel] " Jan Beulich
@ 2017-12-19 14:25   ` Boris Ostrovsky
  2017-12-19 14:40     ` Jan Beulich
  0 siblings, 1 reply; 10+ messages in thread
From: Boris Ostrovsky @ 2017-12-19 14:25 UTC (permalink / raw)
  To: Jan Beulich
  Cc: christian.koenig, helgaas, xen-devel, Juergen Gross, linux-kernel

On 12/19/2017 03:23 AM, Jan Beulich wrote:
>>>> On 18.12.17 at 23:22, <boris.ostrovsky@oracle.com> wrote:
>>>>
>>>> +
>>>> +	xen_e820_table = kzalloc(sizeof(*xen_e820_table), GFP_KERNEL);
> Wouldn't kmalloc() suffice here?

Yes.

>
>> +	if (!xen_e820_table)
>> +		return;
> Not saying "out of memory" here is certainly fine, but shouldn't
> there nevertheless be a warning, as failure to go through the
> rest of the function will impact overall functionality?


Commit ebfdc40969f claims that these types of messages are unnecessary
because allocation failures are signalled by the memory subsystem.


>
>> +	memmap.nr_entries = ARRAY_SIZE(xen_e820_table->entries);
> Is it really reasonable to have a static upper bound here? As we
> know especially EFI systems can come with a pretty scattered
> (pseudo) E820 table. Even if (iirc) this has a static upper bound
> right now in the hypervisor too, it would be nice if the kernel
> didn't need further changes once the hypervisor is being made
> more flexible.


This is how we obtain the map in xen_memory_setup(). Are you suggesting
that we should query for the size first?


>
>> +	/* Mark non-RAM regions as not available. */
>> +	for (; i < memmap.nr_entries; i++) {
>> +		entry = &xen_e820_table->entries[i];
>> +
>> +		if (entry->type == E820_TYPE_RAM)
>> +			continue;
> I can't seem to match up this with ...
>
>> +		if (entry->addr >= hostmem_resource->end)
>> +			break;
>> +
>> +		res = kzalloc(sizeof(*res), GFP_KERNEL);
>> +		if (!res)
>> +			goto out;
>> +
>> +		res->name = "Host memory";
> ... this. Do you mean != instead (with the comment ahead of the
> loop also clarified, saying something like "host RAM regions which
> aren't RAM for us")? And perhaps better "Host RAM"?

Right, this is not memory but rather something else (and so "!=" is
correct). "Unavailable host RAM"?

>
>> +		rc = insert_resource(hostmem_resource, res);
>> +		if (rc) {
>> +			pr_warn("%s: Can't insert [%llx - %llx] (%d)\n",
> [%llx,%llx) ? Plus won't "ll" cause issues with 32-bit non-PAE builds?
> (Same issues somewhere further down.)

This will not be built for non-PAE configurations because memory hotplug
requires PAE.

>
>> +				__func__, res->start, res->end, rc);
>> +			kfree(res);
>> +			goto  out;
> Perhaps better not to bail out of the loop here (at least if rc is
> not -ENOMEM)?

We shouldn't get -ENOMEM here since resource insertion doesn't allocate
anything.

The reason I decided to bail here was because I thought that if we fail
once it means there is a bug somewhere (since we shouldn't really fail)
and so subsequent attempts to insert the range would fail as well.


-boris

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

* Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE
  2017-12-19 14:25   ` Boris Ostrovsky
@ 2017-12-19 14:40     ` Jan Beulich
  2017-12-19 15:03       ` Boris Ostrovsky
  0 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2017-12-19 14:40 UTC (permalink / raw)
  To: Boris Ostrovsky
  Cc: christian.koenig, helgaas, xen-devel, Juergen Gross, linux-kernel

>>> On 19.12.17 at 15:25, <boris.ostrovsky@oracle.com> wrote:
> On 12/19/2017 03:23 AM, Jan Beulich wrote:
>>>>> On 18.12.17 at 23:22, <boris.ostrovsky@oracle.com> wrote:
>>> +	if (!xen_e820_table)
>>> +		return;
>> Not saying "out of memory" here is certainly fine, but shouldn't
>> there nevertheless be a warning, as failure to go through the
>> rest of the function will impact overall functionality?
> 
> Commit ebfdc40969f claims that these types of messages are unnecessary
> because allocation failures are signalled by the memory subsystem.

But the memory subsystem can't possibly provide an indication of
what will not work because of the failed allocation.

>>> +	memmap.nr_entries = ARRAY_SIZE(xen_e820_table->entries);
>> Is it really reasonable to have a static upper bound here? As we
>> know especially EFI systems can come with a pretty scattered
>> (pseudo) E820 table. Even if (iirc) this has a static upper bound
>> right now in the hypervisor too, it would be nice if the kernel
>> didn't need further changes once the hypervisor is being made
>> more flexible.
> 
> This is how we obtain the map in xen_memory_setup(). Are you suggesting
> that we should query for the size first?

That would be better, I think.

>>> +	/* Mark non-RAM regions as not available. */
>>> +	for (; i < memmap.nr_entries; i++) {
>>> +		entry = &xen_e820_table->entries[i];
>>> +
>>> +		if (entry->type == E820_TYPE_RAM)
>>> +			continue;
>> I can't seem to match up this with ...
>>
>>> +		if (entry->addr >= hostmem_resource->end)
>>> +			break;
>>> +
>>> +		res = kzalloc(sizeof(*res), GFP_KERNEL);
>>> +		if (!res)
>>> +			goto out;
>>> +
>>> +		res->name = "Host memory";
>> ... this. Do you mean != instead (with the comment ahead of the
>> loop also clarified, saying something like "host RAM regions which
>> aren't RAM for us")? And perhaps better "Host RAM"?
> 
> Right, this is not memory but rather something else (and so "!=" is
> correct). "Unavailable host RAM"?

If you like to be even more specific than what I had suggested -
sure.

Jan

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

* Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE
  2017-12-19 14:40     ` Jan Beulich
@ 2017-12-19 15:03       ` Boris Ostrovsky
  2017-12-19 15:56         ` Jan Beulich
  0 siblings, 1 reply; 10+ messages in thread
From: Boris Ostrovsky @ 2017-12-19 15:03 UTC (permalink / raw)
  To: Jan Beulich
  Cc: christian.koenig, helgaas, xen-devel, Juergen Gross, linux-kernel

On 12/19/2017 09:40 AM, Jan Beulich wrote:
>>>> On 19.12.17 at 15:25, <boris.ostrovsky@oracle.com> wrote:
>> On 12/19/2017 03:23 AM, Jan Beulich wrote:
>>>>>> On 18.12.17 at 23:22, <boris.ostrovsky@oracle.com> wrote:
>>>> +	if (!xen_e820_table)
>>>> +		return;
>>> Not saying "out of memory" here is certainly fine, but shouldn't
>>> there nevertheless be a warning, as failure to go through the
>>> rest of the function will impact overall functionality?
>> Commit ebfdc40969f claims that these types of messages are unnecessary
>> because allocation failures are signalled by the memory subsystem.
> But the memory subsystem can't possibly provide an indication of
> what will not work because of the failed allocation.


There should be a stack dump which will make it clear which routine failed.


>
>>>> +	memmap.nr_entries = ARRAY_SIZE(xen_e820_table->entries);
>>> Is it really reasonable to have a static upper bound here? As we
>>> know especially EFI systems can come with a pretty scattered
>>> (pseudo) E820 table. Even if (iirc) this has a static upper bound
>>> right now in the hypervisor too, it would be nice if the kernel
>>> didn't need further changes once the hypervisor is being made
>>> more flexible.
>> This is how we obtain the map in xen_memory_setup(). Are you suggesting
>> that we should query for the size first?
> That would be better, I think.


I think we will first need to fix xen_memory_setup() to do that too and
that would be a separate patch.

I am also not clear how this will work on earlier version of the
hypervisor that didn't support querying for size. From what I am seeing
in 4.4 we will get -EFAULT if the buffer is NULL.


>
>>>> +	/* Mark non-RAM regions as not available. */
>>>> +	for (; i < memmap.nr_entries; i++) {
>>>> +		entry = &xen_e820_table->entries[i];
>>>> +
>>>> +		if (entry->type == E820_TYPE_RAM)
>>>> +			continue;
>>> I can't seem to match up this with ...
>>>
>>>> +		if (entry->addr >= hostmem_resource->end)
>>>> +			break;
>>>> +
>>>> +		res = kzalloc(sizeof(*res), GFP_KERNEL);
>>>> +		if (!res)
>>>> +			goto out;
>>>> +
>>>> +		res->name = "Host memory";
>>> ... this. Do you mean != instead (with the comment ahead of the
>>> loop also clarified, saying something like "host RAM regions which
>>> aren't RAM for us")? And perhaps better "Host RAM"?
>> Right, this is not memory but rather something else (and so "!=" is
>> correct). "Unavailable host RAM"?
> If you like to be even more specific than what I had suggested -
> sure.

But did you want to have some changes in the preceding comment? Not sure
I read your comment correctly.

-boris

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

* Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE
  2017-12-19 15:03       ` Boris Ostrovsky
@ 2017-12-19 15:56         ` Jan Beulich
  0 siblings, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2017-12-19 15:56 UTC (permalink / raw)
  To: Boris Ostrovsky
  Cc: christian.koenig, helgaas, xen-devel, Juergen Gross, linux-kernel

>>> On 19.12.17 at 16:03, <boris.ostrovsky@oracle.com> wrote:
> On 12/19/2017 09:40 AM, Jan Beulich wrote:
>>>>> On 19.12.17 at 15:25, <boris.ostrovsky@oracle.com> wrote:
>>> On 12/19/2017 03:23 AM, Jan Beulich wrote:
>>>>> +	memmap.nr_entries = ARRAY_SIZE(xen_e820_table->entries);
>>>> Is it really reasonable to have a static upper bound here? As we
>>>> know especially EFI systems can come with a pretty scattered
>>>> (pseudo) E820 table. Even if (iirc) this has a static upper bound
>>>> right now in the hypervisor too, it would be nice if the kernel
>>>> didn't need further changes once the hypervisor is being made
>>>> more flexible.
>>> This is how we obtain the map in xen_memory_setup(). Are you suggesting
>>> that we should query for the size first?
>> That would be better, I think.
> 
> 
> I think we will first need to fix xen_memory_setup() to do that too and
> that would be a separate patch.
> 
> I am also not clear how this will work on earlier version of the
> hypervisor that didn't support querying for size. From what I am seeing
> in 4.4 we will get -EFAULT if the buffer is NULL.

That's not nice, I agree, but can be dealt with.

>>>>> +	/* Mark non-RAM regions as not available. */
>>>>> +	for (; i < memmap.nr_entries; i++) {
>>>>> +		entry = &xen_e820_table->entries[i];
>>>>> +
>>>>> +		if (entry->type == E820_TYPE_RAM)
>>>>> +			continue;
>>>> I can't seem to match up this with ...
>>>>
>>>>> +		if (entry->addr >= hostmem_resource->end)
>>>>> +			break;
>>>>> +
>>>>> +		res = kzalloc(sizeof(*res), GFP_KERNEL);
>>>>> +		if (!res)
>>>>> +			goto out;
>>>>> +
>>>>> +		res->name = "Host memory";
>>>> ... this. Do you mean != instead (with the comment ahead of the
>>>> loop also clarified, saying something like "host RAM regions which
>>>> aren't RAM for us")? And perhaps better "Host RAM"?
>>> Right, this is not memory but rather something else (and so "!=" is
>>> correct). "Unavailable host RAM"?
>> If you like to be even more specific than what I had suggested -
>> sure.
> 
> But did you want to have some changes in the preceding comment? Not sure
> I read your comment correctly.

Well, "non-RAM" is ambiguous in this context, so yes, I'd prefer it
to be clarified. Whether you use what I've suggested or something
else I don't care much.

Jan

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

end of thread, other threads:[~2017-12-19 15:56 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-18 22:22 [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE Boris Ostrovsky
2017-12-19  8:22 ` Juergen Gross
2017-12-19  8:23 ` [Xen-devel] " Jan Beulich
2017-12-19 14:25   ` Boris Ostrovsky
2017-12-19 14:40     ` Jan Beulich
2017-12-19 15:03       ` Boris Ostrovsky
2017-12-19 15:56         ` Jan Beulich
     [not found] ` <5A38DA840200007800198579@suse.com>
2017-12-19  9:21   ` Juergen Gross
2017-12-19  9:27     ` Jan Beulich
     [not found]     ` <5A38E9A502000078001985CF@suse.com>
2017-12-19 10:08       ` Juergen Gross

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