Messages in this thread | | | Date | Tue, 19 Dec 2017 01:23:16 -0700 | From | "Jan Beulich" <> | Subject | Re: [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
| |