|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH RFC] x86/xen: attempt to inflate the memory balloon on PVH
On Tue, 20 Feb 2024, Roger Pau Monne wrote:
> When running as PVH or HVM Linux will use holes in the memory map as scratch
> space to map grants, foreign domain pages and possibly miscellaneous other
> stuff. However the usage of such memory map holes for Xen purposes can be
> problematic. The request of holesby Xen happen quite early in the kernel boot
> process (grant table setup already uses scratch map space), and it's possible
> that by then not all devices have reclaimed their MMIO space. It's not
> unlikely for chunks of Xen scratch map space to end up using PCI bridge MMIO
> window memory, which (as expected) causes quite a lot of issues in the system.
Am I understanding correctly that XEN_BALLOON_MEMORY_HOTPLUG doesn't
help because it becomes available too late in the PVH boot sequence?
> At least for PVH dom0 we have the possibility of using regions marked as
> UNUSABLE in the e820 memory map. Either if the region is UNUSABLE in the
> native memory map, or it has been converted into UNUSABLE in order to hide RAM
> regions from dom0, the second stage translation page-tables can populate those
> areas without issues.
>
> PV already has this kind of logic, where the balloon driver is inflated at
> boot. Re-use the current logic in order to also inflate it when running as
> PVH. onvert UNUSABLE regions up to the ratio specified in EXTRA_MEM_RATIO to
> RAM, while reserving them using xen_add_extra_mem() (which is also moved so
> it's no longer tied to CONFIG_PV).
>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> RFC reasons:
>
> * Note that it would be preferred for the hypervisor to provide an explicit
> range to be used as scratch mapping space, but that requires changes to
> Xen,
> and it's not fully clear whether Xen can figure out the position of all
> MMIO
> regions at boot in order to suggest a scratch mapping region for dom0.
>
> * Should the whole set of xen_{add,del,chk,inv}_extra_mem() functions be
> moved
> to a different file? For the purposes of PVH only xen_add_extra_mem() is
> moved and the chk and inv ones are PV specific and might not want moving to
> a separate file just to guard them with CONFIG_PV.
> ---
> arch/x86/include/asm/xen/hypervisor.h | 1 +
> arch/x86/platform/pvh/enlighten.c | 3 ++
> arch/x86/xen/enlighten.c | 32 +++++++++++++
> arch/x86/xen/enlighten_pvh.c | 68 +++++++++++++++++++++++++++
> arch/x86/xen/setup.c | 44 -----------------
> arch/x86/xen/xen-ops.h | 14 ++++++
> drivers/xen/balloon.c | 2 -
> 7 files changed, 118 insertions(+), 46 deletions(-)
>
> diff --git a/arch/x86/include/asm/xen/hypervisor.h
> b/arch/x86/include/asm/xen/hypervisor.h
> index a9088250770f..31e2bf8d5db7 100644
> --- a/arch/x86/include/asm/xen/hypervisor.h
> +++ b/arch/x86/include/asm/xen/hypervisor.h
> @@ -62,6 +62,7 @@ void xen_arch_unregister_cpu(int num);
> #ifdef CONFIG_PVH
> void __init xen_pvh_init(struct boot_params *boot_params);
> void __init mem_map_via_hcall(struct boot_params *boot_params_p);
> +void __init xen_reserve_extra_memory(struct boot_params *bootp);
> #endif
>
> /* Lazy mode for batching updates / context switch */
> diff --git a/arch/x86/platform/pvh/enlighten.c
> b/arch/x86/platform/pvh/enlighten.c
> index 00a92cb2c814..a12117f3d4de 100644
> --- a/arch/x86/platform/pvh/enlighten.c
> +++ b/arch/x86/platform/pvh/enlighten.c
> @@ -74,6 +74,9 @@ static void __init init_pvh_bootparams(bool xen_guest)
> } else
> xen_raw_printk("Warning: Can fit ISA range into e820\n");
>
> + if (xen_guest)
> + xen_reserve_extra_memory(&pvh_bootparams);
> +
> pvh_bootparams.hdr.cmd_line_ptr =
> pvh_start_info.cmdline_paddr;
>
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index 3c61bb98c10e..a01ca255b0c6 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -6,6 +6,7 @@
> #include <linux/console.h>
> #include <linux/cpu.h>
> #include <linux/kexec.h>
> +#include <linux/memblock.h>
> #include <linux/slab.h>
> #include <linux/panic_notifier.h>
>
> @@ -350,3 +351,34 @@ void xen_arch_unregister_cpu(int num)
> }
> EXPORT_SYMBOL(xen_arch_unregister_cpu);
> #endif
> +
> +/* Amount of extra memory space we add to the e820 ranges */
> +struct xen_memory_region xen_extra_mem[XEN_EXTRA_MEM_MAX_REGIONS] __initdata;
> +
> +void __init xen_add_extra_mem(unsigned long start_pfn, unsigned long n_pfns)
> +{
> + unsigned int i;
> +
> + /*
> + * No need to check for zero size, should happen rarely and will only
> + * write a new entry regarded to be unused due to zero size.
> + */
> + for (i = 0; i < XEN_EXTRA_MEM_MAX_REGIONS; i++) {
> + /* Add new region. */
> + if (xen_extra_mem[i].n_pfns == 0) {
> + xen_extra_mem[i].start_pfn = start_pfn;
> + xen_extra_mem[i].n_pfns = n_pfns;
> + break;
> + }
> + /* Append to existing region. */
> + if (xen_extra_mem[i].start_pfn + xen_extra_mem[i].n_pfns ==
> + start_pfn) {
> + xen_extra_mem[i].n_pfns += n_pfns;
> + break;
> + }
> + }
> + if (i == XEN_EXTRA_MEM_MAX_REGIONS)
> + printk(KERN_WARNING "Warning: not enough extra memory
> regions\n");
> +
> + memblock_reserve(PFN_PHYS(start_pfn), PFN_PHYS(n_pfns));
> +}
> diff --git a/arch/x86/xen/enlighten_pvh.c b/arch/x86/xen/enlighten_pvh.c
> index ada3868c02c2..c28f073c1df5 100644
> --- a/arch/x86/xen/enlighten_pvh.c
> +++ b/arch/x86/xen/enlighten_pvh.c
> @@ -1,6 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0
> #include <linux/acpi.h>
> #include <linux/export.h>
> +#include <linux/mm.h>
>
> #include <xen/hvc-console.h>
>
> @@ -72,3 +73,70 @@ void __init mem_map_via_hcall(struct boot_params
> *boot_params_p)
> }
> boot_params_p->e820_entries = memmap.nr_entries;
> }
> +
> +/*
> + * Reserve e820 UNUSABLE regions to inflate the memory balloon.
> + *
> + * On PVH dom0 the host memory map is used, RAM regions available to dom0 are
> + * located as the same place as in the native memory map, but since dom0 gets
> + * less memory than the total amount of host RAM the ranges that can't be
> + * populated are converted from RAM -> UNUSABLE. Use such regions (up to the
> + * ratio signaled in EXTRA_MEM_RATIO) in order to inflate the balloon driver
> at
> + * boot. Doing so prevents the guest (even if just temporary) from using
> holes
> + * in the memory map in order to map grants or foreign addresses, and
> + * hopefully limits the risk of a clash with a device MMIO region. Ideally
> the
> + * hypervisor should notify us which memory ranges are suitable for creating
> + * foreign mappings, but that's not yet implemented.
> + */
> +void __init xen_reserve_extra_memory(struct boot_params *bootp)
> +{
> + unsigned int i, ram_pages = 0, extra_pages;
> +
> + for (i = 0; i < bootp->e820_entries; i++) {
> + struct boot_e820_entry *e = &bootp->e820_table[i];
> +
> + if (e->type != E820_TYPE_RAM)
> + continue;
> + ram_pages += PFN_DOWN(e->addr + e->size) - PFN_UP(e->addr);
> + }
> +
> + /* Max amount of extra memory. */
> + extra_pages = EXTRA_MEM_RATIO * ram_pages;
> +
> + /*
> + * Convert UNUSABLE ranges to RAM and reserve them for foreign mapping
> + * purposes.
> + */
> + for (i = 0; i < bootp->e820_entries && extra_pages; i++) {
> + struct boot_e820_entry *e = &bootp->e820_table[i];
> + unsigned long pages;
> +
> + if (e->type != E820_TYPE_UNUSABLE)
> + continue;
> +
> + pages = min(extra_pages,
> + PFN_DOWN(e->addr + e->size) - PFN_UP(e->addr));
> +
> + if (pages != (PFN_DOWN(e->addr + e->size) - PFN_UP(e->addr))) {
> + struct boot_e820_entry *next;
> +
> + if (bootp->e820_entries ==
> + ARRAY_SIZE(bootp->e820_table))
> + /* No space left to split - skip region. */
> + continue;
> +
> + /* Split entry. */
> + next = e + 1;
> + memmove(next, e,
> + (bootp->e820_entries - i) * sizeof(*e));
> + bootp->e820_entries++;
> + next->addr = PAGE_ALIGN(e->addr) + PFN_PHYS(pages);
> + e->size = next->addr - e->addr;
> + next->size -= e->size;
Is this really worth doing? Can we just skip this range and continue or
simply break out and call it a day? Or even add the whole range instead?
The reason I am asking is that I am expecting E820_TYPE_UNUSABLE regions
not to be huge. Splitting one just to cover the few remaining pages out
of extra_pages doesn't seem worth it?
Everything else looks OK to me.
> + }
> + e->type = E820_TYPE_RAM;
> + extra_pages -= pages;
> +
> + xen_add_extra_mem(PFN_UP(e->addr), pages);
> + }
> +}
> diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
> index b3e37961065a..380591028cb8 100644
> --- a/arch/x86/xen/setup.c
> +++ b/arch/x86/xen/setup.c
> @@ -38,9 +38,6 @@
>
> #define GB(x) ((uint64_t)(x) * 1024 * 1024 * 1024)
>
> -/* Amount of extra memory space we add to the e820 ranges */
> -struct xen_memory_region xen_extra_mem[XEN_EXTRA_MEM_MAX_REGIONS] __initdata;
> -
> /* Number of pages released from the initial allocation. */
> unsigned long xen_released_pages;
>
> @@ -64,18 +61,6 @@ static struct {
> } xen_remap_buf __initdata __aligned(PAGE_SIZE);
> static unsigned long xen_remap_mfn __initdata = INVALID_P2M_ENTRY;
>
> -/*
> - * The maximum amount of extra memory compared to the base size. The
> - * main scaling factor is the size of struct page. At extreme ratios
> - * of base:extra, all the base memory can be filled with page
> - * structures for the extra memory, leaving no space for anything
> - * else.
> - *
> - * 10x seems like a reasonable balance between scaling flexibility and
> - * leaving a practically usable system.
> - */
> -#define EXTRA_MEM_RATIO (10)
> -
> static bool xen_512gb_limit __initdata = IS_ENABLED(CONFIG_XEN_512GB);
>
> static void __init xen_parse_512gb(void)
> @@ -96,35 +81,6 @@ static void __init xen_parse_512gb(void)
> xen_512gb_limit = val;
> }
>
> -static void __init xen_add_extra_mem(unsigned long start_pfn,
> - unsigned long n_pfns)
> -{
> - int i;
> -
> - /*
> - * No need to check for zero size, should happen rarely and will only
> - * write a new entry regarded to be unused due to zero size.
> - */
> - for (i = 0; i < XEN_EXTRA_MEM_MAX_REGIONS; i++) {
> - /* Add new region. */
> - if (xen_extra_mem[i].n_pfns == 0) {
> - xen_extra_mem[i].start_pfn = start_pfn;
> - xen_extra_mem[i].n_pfns = n_pfns;
> - break;
> - }
> - /* Append to existing region. */
> - if (xen_extra_mem[i].start_pfn + xen_extra_mem[i].n_pfns ==
> - start_pfn) {
> - xen_extra_mem[i].n_pfns += n_pfns;
> - break;
> - }
> - }
> - if (i == XEN_EXTRA_MEM_MAX_REGIONS)
> - printk(KERN_WARNING "Warning: not enough extra memory
> regions\n");
> -
> - memblock_reserve(PFN_PHYS(start_pfn), PFN_PHYS(n_pfns));
> -}
> -
> static void __init xen_del_extra_mem(unsigned long start_pfn,
> unsigned long n_pfns)
> {
> diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
> index a87ab36889e7..79cf93f2c92f 100644
> --- a/arch/x86/xen/xen-ops.h
> +++ b/arch/x86/xen/xen-ops.h
> @@ -163,4 +163,18 @@ void xen_hvm_post_suspend(int suspend_cancelled);
> static inline void xen_hvm_post_suspend(int suspend_cancelled) {}
> #endif
>
> +/*
> + * The maximum amount of extra memory compared to the base size. The
> + * main scaling factor is the size of struct page. At extreme ratios
> + * of base:extra, all the base memory can be filled with page
> + * structures for the extra memory, leaving no space for anything
> + * else.
> + *
> + * 10x seems like a reasonable balance between scaling flexibility and
> + * leaving a practically usable system.
> + */
> +#define EXTRA_MEM_RATIO (10)
> +
> +void xen_add_extra_mem(unsigned long start_pfn, unsigned long n_pfns);
> +
> #endif /* XEN_OPS_H */
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 976c6cdf9ee6..aaf2514fcfa4 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -672,7 +672,6 @@ EXPORT_SYMBOL(xen_free_ballooned_pages);
>
> static void __init balloon_add_regions(void)
> {
> -#if defined(CONFIG_XEN_PV)
> unsigned long start_pfn, pages;
> unsigned long pfn, extra_pfn_end;
> unsigned int i;
> @@ -696,7 +695,6 @@ static void __init balloon_add_regions(void)
>
> balloon_stats.total_pages += extra_pfn_end - start_pfn;
> }
> -#endif
> }
>
> static int __init balloon_init(void)
> --
> 2.43.0
>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |