lkml.org 
[lkml]   [2017]   [Dec]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE
>>> 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

\
 
 \ /
  Last update: 2017-12-19 09:23    [W:0.109 / U:2.900 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site