Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.12.17 at 23:22, <boris.ostrovsky@xxxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel