Subject: Re: [PATCH] xen/balloon: Mark unallocated host memory as UNUSABLE
To: Boris Ostrovsky <boris.ostrovsky@oracle.com>, xen-devel@lists.xen.org,
        linux-kernel@vger.kernel.org
Cc: helgaas@kernel.org, christian.koenig@amd.com
References: <1513119066-10748-1-git-send-email-boris.ostrovsky@oracle.com>
From: Juergen Gross <jgross@suse.com>
Message-ID: <14852f9f-f234-3226-f32b-3080ca114332@suse.com>
Date: Fri, 15 Dec 2017 13:09:08 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.0
MIME-Version: 1.0
In-Reply-To: <1513119066-10748-1-git-send-email-boris.ostrovsky@oracle.com>
Content-Type: text/plain; charset=utf-8
Content-Language: de-DE
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3443
Lines: 109

On 12/12/17 23:51, 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>
> ---
> I don't see /proc/meminfo reporting the hotplugged memory (although
> internal data such as max_pfn is updated properly). Need to look at
> hotplug code some more. But then I didn't see meminfo changing with
> existing code either.
> 
> 
>  arch/x86/xen/enlighten.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++
>  arch/x86/xen/setup.c     |  6 ++---
>  drivers/xen/balloon.c    | 65 ++++++++++++++++++++++++++++++++++++++-------
>  3 files changed, 127 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index d669e9d..19223b9 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -3,6 +3,7 @@
>  
>  #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 +332,71 @@ 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, i, last_guest_ram;

i and last_guest_ram should be unsigned int

> +	unsigned long max_addr = max_pfn << PAGE_SHIFT;

This might overflow for 32 bit builds

> +	struct e820_table *xen_e820_table;
> +	struct e820_entry *entry;
> +	struct resource *res = NULL;
> +
> +	if (!xen_initial_domain())
> +		return;
> +
> +	xen_e820_table = kzalloc(sizeof(*xen_e820_table), GFP_KERNEL);
> +	if (!xen_e820_table) {
> +		pr_warn("%s: Out of memory\n", __func__);
> +		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 = i = 0;
> +	while (xen_e820_table->entries[i].addr < max_addr) {
> +		if (xen_e820_table->entries[i].type == E820_TYPE_RAM)
> +			last_guest_ram = i;
> +		i++;

Check for i < memmap.nr_entries?

> +	}
> +
> +	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;
> +	for (; i < memmap.nr_entries; i++) {
> +		entry = &xen_e820_table->entries[i];
> +		if (entry->type == E820_TYPE_RAM)

Shouldn't that be != ?

> +			continue;
> +
> +		res = kzalloc(sizeof(*res), GFP_KERNEL);
> +		if (!res) {
> +			pr_warn("%s: Out of memory\n", __func__);

Don't print error message in case of allocation failures.


Juergen