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 <[email protected]>
---
Changes in v2:
In enlighten.c:
- Fix 32-bit build problem (include bootmem.h), make variables 32-bit safe
- Add a test to avoid inserting a resource into hostmem which is beyond
hostmem's end
- Replace 'while' loop with 'for' to simplify array boundary check.
- Drop out of memory warnings
- Add a comment clarifying use of hostmem_resource.
arch/x86/xen/enlighten.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++
arch/x86/xen/setup.c | 6 ++--
drivers/xen/balloon.c | 65 ++++++++++++++++++++++++++++++++++------
3 files changed, 135 insertions(+), 13 deletions(-)
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index d669e9d..ac96142 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -1,8 +1,12 @@
+#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG
+#include <linux/bootmem.h>
+#endif
#include <linux/cpu.h>
#include <linux/kexec.h>
#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 +335,76 @@ 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;
+ unsigned int i, last_guest_ram;
+ phys_addr_t max_addr = max_pfn << PAGE_SHIFT;
+ struct e820_table *xen_e820_table;
+ struct e820_entry *entry;
+ struct resource *res;
+
+ if (!xen_initial_domain())
+ return;
+
+ xen_e820_table = kzalloc(sizeof(*xen_e820_table), GFP_KERNEL);
+ if (!xen_e820_table)
+ 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 = 0;
+ for (i = 0; i < memmap.nr_entries; i++) {
+ if (xen_e820_table->entries[i].addr >= max_addr)
+ break;
+ if (xen_e820_table->entries[i].type == E820_TYPE_RAM)
+ last_guest_ram = i;
+ }
+
+ 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;
+
+ /* 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;
+
+ if (entry->addr >= hostmem_resource->end)
+ break;
+
+ res = kzalloc(sizeof(*res), GFP_KERNEL);
+ if (!res)
+ goto out;
+
+ res->name = "Host memory";
+ res->start = entry->addr;
+ res->end = (entry->addr + entry->size < hostmem_resource->end) ?
+ entry->addr + entry->size : hostmem_resource->end;
+ rc = insert_resource(hostmem_resource, res);
+ if (rc) {
+ pr_warn("%s: Can't insert [%llx - %llx] (%d)\n",
+ __func__, res->start, res->end, rc);
+ kfree(res);
+ goto out;
+ }
+ }
+
+ out:
+ kfree(xen_e820_table);
+}
+#endif /* CONFIG_XEN_BALLOON_MEMORY_HOTPLUG */
diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
index c114ca7..6e0d208 100644
--- a/arch/x86/xen/setup.c
+++ b/arch/x86/xen/setup.c
@@ -808,7 +808,6 @@ char * __init xen_memory_setup(void)
addr = xen_e820_table.entries[0].addr;
size = xen_e820_table.entries[0].size;
while (i < xen_e820_table.nr_entries) {
- bool discard = false;
chunk_size = size;
type = xen_e820_table.entries[i].type;
@@ -824,11 +823,10 @@ char * __init xen_memory_setup(void)
xen_add_extra_mem(pfn_s, n_pfns);
xen_max_p2m_pfn = pfn_s + n_pfns;
} else
- discard = true;
+ type = E820_TYPE_UNUSABLE;
}
- if (!discard)
- xen_align_and_add_e820_region(addr, chunk_size, type);
+ xen_align_and_add_e820_region(addr, chunk_size, type);
addr += chunk_size;
size -= chunk_size;
diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index f77e499..fb5aa7c 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -257,10 +257,25 @@ static void release_memory_resource(struct resource *resource)
kfree(resource);
}
+/*
+ * Host memory not allocated to dom0. We can use this range for hotplug-based
+ * ballooning.
+ *
+ * It's a type-less resource. Setting IORESOURCE_MEM will make resource
+ * management algorithms (arch_remove_reservations()) look into guest e820,
+ * which we don't want.
+ */
+static struct resource hostmem_resource = {
+ .name = "Host memory",
+};
+
+void __attribute__((weak)) __init arch_xen_balloon_init(struct resource *res)
+{}
+
static struct resource *additional_memory_resource(phys_addr_t size)
{
- struct resource *res;
- int ret;
+ struct resource *res, *res_hostmem;
+ int ret = -ENOMEM;
res = kzalloc(sizeof(*res), GFP_KERNEL);
if (!res)
@@ -269,13 +284,42 @@ static struct resource *additional_memory_resource(phys_addr_t size)
res->name = "System RAM";
res->flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
- ret = allocate_resource(&iomem_resource, res,
- size, 0, -1,
- PAGES_PER_SECTION * PAGE_SIZE, NULL, NULL);
- if (ret < 0) {
- pr_err("Cannot allocate new System RAM resource\n");
- kfree(res);
- return NULL;
+ res_hostmem = kzalloc(sizeof(*res), GFP_KERNEL);
+ if (res_hostmem) {
+ /* Try to grab a range from hostmem */
+ res_hostmem->name = "Host memory";
+ ret = allocate_resource(&hostmem_resource, res_hostmem,
+ size, 0, -1,
+ PAGES_PER_SECTION * PAGE_SIZE, NULL, NULL);
+ }
+
+ if (!ret) {
+ /*
+ * Insert this resource into iomem. Because hostmem_resource
+ * tracks portion of guest e820 marked as UNUSABLE noone else
+ * should try to use it.
+ */
+ res->start = res_hostmem->start;
+ res->end = res_hostmem->end;
+ ret = insert_resource(&iomem_resource, res);
+ if (ret < 0) {
+ pr_err("Can't insert iomem_resource [%llx - %llx]\n",
+ res->start, res->end);
+ release_memory_resource(res_hostmem);
+ res_hostmem = NULL;
+ res->start = res->end = 0;
+ }
+ }
+
+ if (ret) {
+ ret = allocate_resource(&iomem_resource, res,
+ size, 0, -1,
+ PAGES_PER_SECTION * PAGE_SIZE, NULL, NULL);
+ if (ret < 0) {
+ pr_err("Cannot allocate new System RAM resource\n");
+ kfree(res);
+ return NULL;
+ }
}
#ifdef CONFIG_SPARSEMEM
@@ -287,6 +331,7 @@ static struct resource *additional_memory_resource(phys_addr_t size)
pr_err("New System RAM resource outside addressable RAM (%lu > %lu)\n",
pfn, limit);
release_memory_resource(res);
+ release_memory_resource(res_hostmem);
return NULL;
}
}
@@ -765,6 +810,8 @@ static int __init balloon_init(void)
set_online_page_callback(&xen_online_page);
register_memory_notifier(&xen_memory_nb);
register_sysctl_table(xen_root);
+
+ arch_xen_balloon_init(&hostmem_resource);
#endif
#ifdef CONFIG_XEN_PV
--
2.7.4
On 18/12/17 23:22, 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 <[email protected]>
> ---
> Changes in v2:
>
> In enlighten.c:
> - Fix 32-bit build problem (include bootmem.h), make variables 32-bit safe
> - Add a test to avoid inserting a resource into hostmem which is beyond
> hostmem's end
> - Replace 'while' loop with 'for' to simplify array boundary check.
> - Drop out of memory warnings
> - Add a comment clarifying use of hostmem_resource.
>
> arch/x86/xen/enlighten.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++
> arch/x86/xen/setup.c | 6 ++--
> drivers/xen/balloon.c | 65 ++++++++++++++++++++++++++++++++++------
> 3 files changed, 135 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index d669e9d..ac96142 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -1,8 +1,12 @@
> +#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG
> +#include <linux/bootmem.h>
> +#endif
> #include <linux/cpu.h>
> #include <linux/kexec.h>
>
> #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 +335,76 @@ 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)
Sorry for noticing only now, but shouldn't you add a prototype in some
header for this function?
> +{
> + struct xen_memory_map memmap;
> + int rc;
> + unsigned int i, last_guest_ram;
> + phys_addr_t max_addr = max_pfn << PAGE_SHIFT;
Using PFN_PHYS() would have avoided the still present overflow on 32-bit
kernel: the shift is done with the 32-bit quantity and only then the
result is promoted to 64-bit.
Juergen
>>> On 18.12.17 at 23:22, <[email protected]> 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
On 12/19/2017 03:23 AM, Jan Beulich wrote:
>>>> On 18.12.17 at 23:22, <[email protected]> wrote:
>>>>
>>>> +
>>>> + xen_e820_table = kzalloc(sizeof(*xen_e820_table), GFP_KERNEL);
> Wouldn't kmalloc() suffice here?
Yes.
>
>> + 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?
Commit ebfdc40969f claims that these types of messages are unnecessary
because allocation failures are signalled by the memory subsystem.
>
>> + 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.
This is how we obtain the map in xen_memory_setup(). Are you suggesting
that we should query for the size first?
>
>> + /* 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"?
Right, this is not memory but rather something else (and so "!=" is
correct). "Unavailable 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.)
This will not be built for non-PAE configurations because memory hotplug
requires PAE.
>
>> + __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)?
We shouldn't get -ENOMEM here since resource insertion doesn't allocate
anything.
The reason I decided to bail here was because I thought that if we fail
once it means there is a bug somewhere (since we shouldn't really fail)
and so subsequent attempts to insert the range would fail as well.
-boris
>>> On 19.12.17 at 15:25, <[email protected]> wrote:
> On 12/19/2017 03:23 AM, Jan Beulich wrote:
>>>>> On 18.12.17 at 23:22, <[email protected]> wrote:
>>> + 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?
>
> Commit ebfdc40969f claims that these types of messages are unnecessary
> because allocation failures are signalled by the memory subsystem.
But the memory subsystem can't possibly provide an indication of
what will not work because of the failed allocation.
>>> + 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.
>
> This is how we obtain the map in xen_memory_setup(). Are you suggesting
> that we should query for the size first?
That would be better, I think.
>>> + /* 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"?
>
> Right, this is not memory but rather something else (and so "!=" is
> correct). "Unavailable host RAM"?
If you like to be even more specific than what I had suggested -
sure.
Jan
On 12/19/2017 09:40 AM, Jan Beulich wrote:
>>>> On 19.12.17 at 15:25, <[email protected]> wrote:
>> On 12/19/2017 03:23 AM, Jan Beulich wrote:
>>>>>> On 18.12.17 at 23:22, <[email protected]> wrote:
>>>> + 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?
>> Commit ebfdc40969f claims that these types of messages are unnecessary
>> because allocation failures are signalled by the memory subsystem.
> But the memory subsystem can't possibly provide an indication of
> what will not work because of the failed allocation.
There should be a stack dump which will make it clear which routine failed.
>
>>>> + 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.
>> This is how we obtain the map in xen_memory_setup(). Are you suggesting
>> that we should query for the size first?
> That would be better, I think.
I think we will first need to fix xen_memory_setup() to do that too and
that would be a separate patch.
I am also not clear how this will work on earlier version of the
hypervisor that didn't support querying for size. From what I am seeing
in 4.4 we will get -EFAULT if the buffer is NULL.
>
>>>> + /* 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"?
>> Right, this is not memory but rather something else (and so "!=" is
>> correct). "Unavailable host RAM"?
> If you like to be even more specific than what I had suggested -
> sure.
But did you want to have some changes in the preceding comment? Not sure
I read your comment correctly.
-boris
>>> On 19.12.17 at 16:03, <[email protected]> wrote:
> On 12/19/2017 09:40 AM, Jan Beulich wrote:
>>>>> On 19.12.17 at 15:25, <[email protected]> wrote:
>>> On 12/19/2017 03:23 AM, Jan Beulich wrote:
>>>>> + 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.
>>> This is how we obtain the map in xen_memory_setup(). Are you suggesting
>>> that we should query for the size first?
>> That would be better, I think.
>
>
> I think we will first need to fix xen_memory_setup() to do that too and
> that would be a separate patch.
>
> I am also not clear how this will work on earlier version of the
> hypervisor that didn't support querying for size. From what I am seeing
> in 4.4 we will get -EFAULT if the buffer is NULL.
That's not nice, I agree, but can be dealt with.
>>>>> + /* 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"?
>>> Right, this is not memory but rather something else (and so "!=" is
>>> correct). "Unavailable host RAM"?
>> If you like to be even more specific than what I had suggested -
>> sure.
>
> But did you want to have some changes in the preceding comment? Not sure
> I read your comment correctly.
Well, "non-RAM" is ambiguous in this context, so yes, I'd prefer it
to be clarified. Whether you use what I've suggested or something
else I don't care much.
Jan