Message-Id: <5A38DA840200007800198579@prv-mh.provo.novell.com>
Date: Tue, 19 Dec 2017 01:23:16 -0700
From: "Jan Beulich" <JBeulich@suse.com>
To: "Boris Ostrovsky" <boris.ostrovsky@oracle.com>
Cc: <christian.koenig@amd.com>, <helgaas@kernel.org>,
        <xen-devel@lists.xen.org>, "Juergen Gross" <jgross@suse.com>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host
 memory as UNUSABLE
References: <1513635771-11779-1-git-send-email-boris.ostrovsky@oracle.com>
In-Reply-To: <1513635771-11779-1-git-send-email-boris.ostrovsky@oracle.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 8BIT
Content-Disposition: inline
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2200
Lines: 77

>>> 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