[PATCH 0/3] x86: "brk" allocator

[PATCH 0/3] x86: "brk" allocator

[Jan Beulich]

Derive for our purposes something similar to what Linux has been having for
a long while.

1: x86: introduce "brk" allocator
2: x86/EFI: replace ebmalloc()
3: xhci-dbc: use brk_alloc()

To reduce padding holes, .bss.page_aligned and per-CPU data may want moving
immediately ahead of __brk_start[]. Albeit then the tail of per-CPU data will
all be padding space; sadly the TSS wants/needs page-aligning for XPTI
purposes.

Jan

[PATCH 1/3] x86: introduce "brk" allocator

[Jan Beulich]

... to replace ebmalloc(), and then to find further use(s) to allow
recovering memory which is needed very early (and hence needs setting up
statically), but may not fully be used (or not used at all).

Note that unlike free_ebmalloc_unused_mem(), brk_free_unused() (once
other code is converted) will be able to free part of the BRK space even
in the xen.efi case. That would happen if BRK space extends across a 2Mb
boundary, and actual use stops before that boundary.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Changing setup.c's reserve_e820_ram() uses would be cumbersome when done
right here. That'll be done when ebmalloc() is replaced, and hence
what's there can also simply be replaced.

The xen.efi detection may want separating out into a helper.

When linking xen.efi, ld produces a base relocation for the reference to
__subsystem__, which is wrong (that's an absolute symbol, after all).
While that will need fixing there, it does no harm for our purposes.

--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -1,4 +1,5 @@
 obj-bin-y += head.o
+obj-bin-y += brk.init.o
 obj-bin-y += built-in-32.o
 obj-bin-y += $(obj64)
 
--- /dev/null
+++ b/xen/arch/x86/boot/brk.c
@@ -0,0 +1,72 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include <xen/efi.h>
+#include <xen/lib.h>
+#include <xen/mm.h>
+#include <xen/page-defs.h>
+
+#include <asm/brk.h>
+
+extern char __brk_start[];
+extern const char __bss_end[];
+
+static unsigned long __initdata allocated;
+static bool __initdata finished;
+
+void *__init brk_alloc(size_t size)
+{
+    void *ptr = __brk_start + allocated;
+
+    if ( finished )
+        return NULL;
+
+    /* Allocations PAGE_SIZE and up will be page-aligned. */
+    if ( size >= PAGE_SIZE )
+        allocated = ROUNDUP(allocated, PAGE_SIZE);
+
+    allocated += ROUNDUP(size, sizeof(void *));
+
+    if ( allocated > __bss_end - __brk_start )
+        return NULL;
+
+    return ptr;
+}
+
+unsigned long __init brk_get_unused_start(void)
+{
+    finished = true;
+
+    allocated = ROUNDUP(allocated, PAGE_SIZE);
+
+    return (unsigned long)__brk_start + allocated;
+}
+
+void __init brk_free_unused(void)
+{
+    unsigned long start = brk_get_unused_start(),
+                  end = (unsigned long)__bss_end;
+    unsigned int subsys;
+
+    /*
+     * Only xen.efi will have the symbol __subsystem__ available, and it'll
+     * be non-zero (10) there.  In ELF the symbol will be undefined, and
+     * hence zero will be loaded into the register.
+     */
+    asm ( ".weak __subsystem__; mov $__subsystem__, %0" : "=r" (subsys) );
+
+    /* using_2M_mapping() isn't available here. */
+    if ( IS_ENABLED(CONFIG_XEN_ALIGN_2M) || subsys )
+        start = PAGE_ALIGN_2M(start);
+
+    if ( start >= end )
+        return;
+
+    destroy_xen_mappings(start, PAGE_ALIGN_2M(end));
+
+    /*
+     * By reserving needed space early in the E820 map, excess space gets freed
+     * way before we make it here. Don't free the range a 2nd time.
+     */
+
+    printk(XENLOG_INFO "Freed %lukB unused BRK memory\n", (end - start) >> 10);
+}
--- /dev/null
+++ b/xen/arch/x86/include/asm/brk.h
@@ -0,0 +1,7 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include <xen/types.h>
+
+void *brk_alloc(size_t size);
+unsigned long brk_get_unused_start(void);
+void brk_free_unused(void);
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -331,7 +331,11 @@ SECTIONS
        __bss_start = .;
        *(.bss.page_aligned*)
        PERCPU_BSS
-       *(.bss .bss.*)
+       *(.bss .bss.[a-zA-Z0-9_]*)
+       . = ALIGN(PAGE_SIZE);
+       __brk_start = .;
+       *(.bss..brk.page_aligned*)
+       *(.bss..brk*)
        . = ALIGN(POINTER_ALIGN);
        __bss_end = .;
   } PHDR(text)
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -112,6 +112,7 @@
 #include <xen/vmap.h>
 #include <xen/xmalloc.h>
 
+#include <asm/brk.h>
 #include <asm/e820.h>
 #include <asm/fixmap.h>
 #include <asm/flushtlb.h>
@@ -337,6 +338,8 @@ void __init arch_init_memory(void)
 
     efi_init_memory();
 
+    brk_free_unused();
+
 #ifndef NDEBUG
     if ( highmem_start )
     {

Re: [PATCH 1/3] x86: introduce "brk" allocator

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 4899 bytes --]

On Thu, Nov 13, 2025 at 12:08:18PM +0100, Jan Beulich wrote:
> ... to replace ebmalloc(), and then to find further use(s) to allow
> recovering memory which is needed very early (and hence needs setting up
> statically), but may not fully be used (or not used at all).
> 
> Note that unlike free_ebmalloc_unused_mem(), brk_free_unused() (once
> other code is converted) will be able to free part of the BRK space even
> in the xen.efi case. That would happen if BRK space extends across a 2Mb
> boundary, and actual use stops before that boundary.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Changing setup.c's reserve_e820_ram() uses would be cumbersome when done
> right here. That'll be done when ebmalloc() is replaced, and hence
> what's there can also simply be replaced.
> 
> The xen.efi detection may want separating out into a helper.
> 
> When linking xen.efi, ld produces a base relocation for the reference to
> __subsystem__, which is wrong (that's an absolute symbol, after all).
> While that will need fixing there, it does no harm for our purposes.
> 
> --- a/xen/arch/x86/boot/Makefile
> +++ b/xen/arch/x86/boot/Makefile
> @@ -1,4 +1,5 @@
>  obj-bin-y += head.o
> +obj-bin-y += brk.init.o
>  obj-bin-y += built-in-32.o
>  obj-bin-y += $(obj64)
>  
> --- /dev/null
> +++ b/xen/arch/x86/boot/brk.c
> @@ -0,0 +1,72 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include <xen/efi.h>
> +#include <xen/lib.h>
> +#include <xen/mm.h>
> +#include <xen/page-defs.h>
> +
> +#include <asm/brk.h>
> +
> +extern char __brk_start[];
> +extern const char __bss_end[];
> +
> +static unsigned long __initdata allocated;
> +static bool __initdata finished;
> +
> +void *__init brk_alloc(size_t size)
> +{
> +    void *ptr = __brk_start + allocated;
> +
> +    if ( finished )
> +        return NULL;
> +
> +    /* Allocations PAGE_SIZE and up will be page-aligned. */
> +    if ( size >= PAGE_SIZE )
> +        allocated = ROUNDUP(allocated, PAGE_SIZE);
> +
> +    allocated += ROUNDUP(size, sizeof(void *));
> +
> +    if ( allocated > __bss_end - __brk_start )
> +        return NULL;
> +
> +    return ptr;
> +}
> +
> +unsigned long __init brk_get_unused_start(void)

It's a bit unintuitive for brk_get_* to have this significant side
effect. Maybe name it brk_finalize_get_unused_start() ?

> +{
> +    finished = true;
> +
> +    allocated = ROUNDUP(allocated, PAGE_SIZE);
> +
> +    return (unsigned long)__brk_start + allocated;
> +}
> +
> +void __init brk_free_unused(void)
> +{
> +    unsigned long start = brk_get_unused_start(),
> +                  end = (unsigned long)__bss_end;
> +    unsigned int subsys;
> +
> +    /*
> +     * Only xen.efi will have the symbol __subsystem__ available, and it'll
> +     * be non-zero (10) there.  In ELF the symbol will be undefined, and
> +     * hence zero will be loaded into the register.
> +     */
> +    asm ( ".weak __subsystem__; mov $__subsystem__, %0" : "=r" (subsys) );

Is this really the best way to detect xen.efi?

> +
> +    /* using_2M_mapping() isn't available here. */
> +    if ( IS_ENABLED(CONFIG_XEN_ALIGN_2M) || subsys )
> +        start = PAGE_ALIGN_2M(start);
> +
> +    if ( start >= end )
> +        return;
> +
> +    destroy_xen_mappings(start, PAGE_ALIGN_2M(end));
> +
> +    /*
> +     * By reserving needed space early in the E820 map, excess space gets freed
> +     * way before we make it here. Don't free the range a 2nd time.
> +     */
> +
> +    printk(XENLOG_INFO "Freed %lukB unused BRK memory\n", (end - start) >> 10);
> +}
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/brk.h
> @@ -0,0 +1,7 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include <xen/types.h>
> +
> +void *brk_alloc(size_t size);
> +unsigned long brk_get_unused_start(void);
> +void brk_free_unused(void);
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -331,7 +331,11 @@ SECTIONS
>         __bss_start = .;
>         *(.bss.page_aligned*)
>         PERCPU_BSS
> -       *(.bss .bss.*)
> +       *(.bss .bss.[a-zA-Z0-9_]*)
> +       . = ALIGN(PAGE_SIZE);
> +       __brk_start = .;
> +       *(.bss..brk.page_aligned*)
> +       *(.bss..brk*)
>         . = ALIGN(POINTER_ALIGN);
>         __bss_end = .;
>    } PHDR(text)
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -112,6 +112,7 @@
>  #include <xen/vmap.h>
>  #include <xen/xmalloc.h>
>  
> +#include <asm/brk.h>
>  #include <asm/e820.h>
>  #include <asm/fixmap.h>
>  #include <asm/flushtlb.h>
> @@ -337,6 +338,8 @@ void __init arch_init_memory(void)
>  
>      efi_init_memory();
>  
> +    brk_free_unused();
> +
>  #ifndef NDEBUG
>      if ( highmem_start )
>      {
> 
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] x86: introduce "brk" allocator

[Jan Beulich]

On 13.11.2025 18:41, Marek Marczykowski-Górecki wrote:
> On Thu, Nov 13, 2025 at 12:08:18PM +0100, Jan Beulich wrote:
>> --- /dev/null
>> +++ b/xen/arch/x86/boot/brk.c
>> @@ -0,0 +1,72 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +
>> +#include <xen/efi.h>
>> +#include <xen/lib.h>
>> +#include <xen/mm.h>
>> +#include <xen/page-defs.h>
>> +
>> +#include <asm/brk.h>
>> +
>> +extern char __brk_start[];
>> +extern const char __bss_end[];
>> +
>> +static unsigned long __initdata allocated;
>> +static bool __initdata finished;
>> +
>> +void *__init brk_alloc(size_t size)
>> +{
>> +    void *ptr = __brk_start + allocated;
>> +
>> +    if ( finished )
>> +        return NULL;
>> +
>> +    /* Allocations PAGE_SIZE and up will be page-aligned. */
>> +    if ( size >= PAGE_SIZE )
>> +        allocated = ROUNDUP(allocated, PAGE_SIZE);
>> +
>> +    allocated += ROUNDUP(size, sizeof(void *));
>> +
>> +    if ( allocated > __bss_end - __brk_start )
>> +        return NULL;
>> +
>> +    return ptr;
>> +}
>> +
>> +unsigned long __init brk_get_unused_start(void)
> 
> It's a bit unintuitive for brk_get_* to have this significant side
> effect. Maybe name it brk_finalize_get_unused_start() ?

Getting too long for my taste, and a caller obtaining this value kind of
needs to understand that either what it gets back is stale the moment it
uses the result, or (for it to not be stale) no further changes (i.e.
allocations) are permitted afterwards.

>> +{
>> +    finished = true;
>> +
>> +    allocated = ROUNDUP(allocated, PAGE_SIZE);
>> +
>> +    return (unsigned long)__brk_start + allocated;
>> +}
>> +
>> +void __init brk_free_unused(void)
>> +{
>> +    unsigned long start = brk_get_unused_start(),
>> +                  end = (unsigned long)__bss_end;
>> +    unsigned int subsys;
>> +
>> +    /*
>> +     * Only xen.efi will have the symbol __subsystem__ available, and it'll
>> +     * be non-zero (10) there.  In ELF the symbol will be undefined, and
>> +     * hence zero will be loaded into the register.
>> +     */
>> +    asm ( ".weak __subsystem__; mov $__subsystem__, %0" : "=r" (subsys) );
> 
> Is this really the best way to detect xen.efi?

Well, it took me a while to figure _some_ reasonably reliable way. I'm
all ears towards better approaches.

Jan

[PATCH 2/3] x86/EFI: replace ebmalloc()

[Jan Beulich]

Use the new brk_alloc() instead, with ebmalloc() merely being a thin
wrapper.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I'm not quite certain whether we ought to permit non-page-granular
reservations. The in-memory image being somewhat larger due to possibly
excessive padding isn't really a big problem, I think.

--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -262,7 +262,7 @@ quiet_cmd_obj_init_o = INIT_O  $@
 define cmd_obj_init_o
     $(OBJDUMP) -h $< | while read idx name sz rest; do \
         case "$$name" in \
-        .*.local) ;; \
+        .*.local|.bss..brk*) ;; \
         .text|.text.*|.data|.data.*|.bss|.bss.*) \
             test $$(echo $$sz | sed 's,00*,0,') != 0 || continue; \
             echo "Error: size of $<:$$name is 0x$$sz" >&2; \
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -10,6 +10,7 @@
 #include <xen/vga.h>
 
 #include <asm/boot-helpers.h>
+#include <asm/brk.h>
 #include <asm/e820.h>
 #include <asm/edd.h>
 #include <asm/microcode.h>
@@ -119,6 +120,18 @@ static void __init relocate_trampoline(u
     reloc_trampoline64();
 }
 
+DEFINE_BRK(efi, MB(1));
+
+static void *__init ebmalloc(size_t size)
+{
+    void *ptr = brk_alloc(size);
+
+    if ( !ptr )
+        blexit(L"Out of BRK memory\r\n");
+
+    return ptr;
+}
+
 static void __init place_string(u32 *addr, const char *s)
 {
     char *alloc = NULL;
--- a/xen/arch/x86/efi/stub.c
+++ b/xen/arch/x86/efi/stub.c
@@ -41,12 +41,4 @@ void __init noreturn efi_multiboot2(EFI_
 
 void __init efi_init_memory(void) { }
 
-bool efi_boot_mem_unused(unsigned long *start, unsigned long *end)
-{
-    /* FIXME: Simplify once the call here with two NULLs goes away. */
-    if ( start || end )
-        *start = *end = (unsigned long)_end;
-    return false;
-}
-
 void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t l4e) { }
--- a/xen/arch/x86/include/asm/brk.h
+++ b/xen/arch/x86/include/asm/brk.h
@@ -2,6 +2,10 @@
 
 #include <xen/types.h>
 
+#define DEFINE_BRK(var, size) \
+    static char __section(".bss..brk.page_aligned") __aligned(PAGE_SIZE) \
+        __used var ## _brk_[size]
+
 void *brk_alloc(size_t size);
 unsigned long brk_get_unused_start(void);
 void brk_free_unused(void);
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -9,6 +9,8 @@ extern const char __2M_rodata_start[], _
 extern char __2M_init_start[], __2M_init_end[];
 extern char __2M_rwdata_start[], __2M_rwdata_end[];
 
+extern unsigned long brk_end;
+
 extern unsigned long xenheap_initial_phys_start;
 extern uint64_t boot_tsc_stamp;
 
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -31,6 +31,7 @@
 #include <asm/alternative.h>
 #include <asm/apic.h>
 #include <asm/bootinfo.h>
+#include <asm/brk.h>
 #include <asm/bzimage.h>
 #include <asm/cpu-policy.h>
 #include <asm/e820.h>
@@ -163,6 +164,8 @@ cpumask_t __read_mostly cpu_present_map;
 
 unsigned long __read_mostly xen_phys_start;
 
+unsigned long __ro_after_init brk_end;
+
 /* Only used in asm code and within this source file */
 char asmlinkage __section(".init.bss.stack_aligned") __aligned(STACK_SIZE)
     cpu0_stack[STACK_SIZE];
@@ -1109,7 +1112,6 @@ void asmlinkage __init noreturn __start_
     struct boot_info *bi;
     unsigned long nr_pages, raw_max_page;
     int i, j, bytes = 0;
-    unsigned long eb_start, eb_end;
     bool acpi_boot_table_init_done = false, relocated = false;
     bool vm_init_done = false;
     int ret;
@@ -1473,7 +1475,7 @@ void asmlinkage __init noreturn __start_
         /*
          * This needs to remain in sync with remove_xen_ranges() and the
          * respective reserve_e820_ram() invocation below. No need to
-         * query efi_boot_mem_unused() here, though.
+         * query brk_get_unused_start() here, though.
          */
         xen->start = virt_to_maddr(_stext);
         xen->size  = __2M_rwdata_end - _stext;
@@ -1616,18 +1618,11 @@ void asmlinkage __init noreturn __start_
     if ( !xen_phys_start )
         panic("Not enough memory to relocate Xen\n");
 
-    /* FIXME: Putting a hole in .bss would shatter the large page mapping. */
-    if ( using_2M_mapping() )
-        efi_boot_mem_unused(NULL, NULL);
-
     /* This needs to remain in sync with remove_xen_ranges(). */
-    if ( efi_boot_mem_unused(&eb_start, &eb_end) )
-    {
-        reserve_e820_ram(&boot_e820, __pa(_stext), __pa(eb_start));
-        reserve_e820_ram(&boot_e820, __pa(eb_end), __pa(__2M_rwdata_end));
-    }
-    else
-        reserve_e820_ram(&boot_e820, __pa(_stext), __pa(__2M_rwdata_end));
+    brk_end = brk_get_unused_start();
+    if ( using_2M_mapping() )
+        brk_end = PAGE_ALIGN_2M(brk_end);
+    reserve_e820_ram(&boot_e820, __pa(_stext), __pa(brk_end));
 
     /* Late kexec reservation (dynamic start address). */
     kexec_reserve_area();
@@ -2231,7 +2226,6 @@ __initcall(init_xen_cap_info);
 
 int __hwdom_init remove_xen_ranges(struct rangeset *r)
 {
-    paddr_t start, end;
     int rc;
 
     /* S3 resume code (and other real mode trampoline code) */
@@ -2253,26 +2247,10 @@ int __hwdom_init remove_xen_ranges(struc
         return rc;
 
     /* hypervisor .data + .bss */
-    if ( efi_boot_mem_unused(&start, &end) )
-    {
-        ASSERT(__pa(start) >= __pa(&__2M_rwdata_start));
-        rc = rangeset_remove_range(r, PFN_DOWN(__pa(&__2M_rwdata_start)),
-                                   PFN_DOWN(__pa(start) - 1));
-        if ( rc )
-            return rc;
-        ASSERT(__pa(end) <= __pa(&__2M_rwdata_end));
-        rc = rangeset_remove_range(r, PFN_DOWN(__pa(end)),
-                                   PFN_DOWN(__pa(&__2M_rwdata_end) - 1));
-        if ( rc )
-            return rc;
-    }
-    else
-    {
-        rc = rangeset_remove_range(r, PFN_DOWN(__pa(&__2M_rwdata_start)),
-                                   PFN_DOWN(__pa(&__2M_rwdata_end) - 1));
-        if ( rc )
-            return rc;
-    }
+    rc = rangeset_remove_range(r, PFN_DOWN(__pa(&__2M_rwdata_start)),
+                               PFN_DOWN(__pa(brk_end) - 1));
+    if ( rc )
+        return rc;
 
     return 0;
 }
--- a/xen/arch/x86/tboot.c
+++ b/xen/arch/x86/tboot.c
@@ -321,8 +321,6 @@ void tboot_shutdown(uint32_t shutdown_ty
     /* if this is S3 then set regions to MAC */
     if ( shutdown_type == TB_SHUTDOWN_S3 )
     {
-        unsigned long s, e;
-
         /*
          * Xen regions for tboot to MAC. This needs to remain in sync with
          * remove_xen_ranges().
@@ -336,16 +334,8 @@ void tboot_shutdown(uint32_t shutdown_ty
         g_tboot_shared->mac_regions[1].size = __2M_rodata_end - _stext;
         /* hypervisor .data + .bss */
         g_tboot_shared->mac_regions[2].start = (uint64_t)__pa(&__2M_rwdata_start);
-        g_tboot_shared->mac_regions[2].size = __2M_rwdata_end - __2M_rwdata_start;
-        if ( efi_boot_mem_unused(&s, &e) )
-        {
-            g_tboot_shared->mac_regions[2].size =
-                s - (unsigned long)__2M_rwdata_start;
-            g_tboot_shared->mac_regions[3].start = __pa(e);
-            g_tboot_shared->mac_regions[3].size =
-                (unsigned long)__2M_rwdata_end - e;
-            g_tboot_shared->num_mac_regions = 4;
-        }
+        g_tboot_shared->mac_regions[2].size =
+            brk_end - (unsigned long)__2M_rwdata_start;
 
         /*
          * MAC domains and other Xen memory
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1788,8 +1788,6 @@ void __init efi_init_memory(void)
         pte_attr_t prot;
     } *extra, *extra_head = NULL;
 
-    free_ebmalloc_unused_mem();
-
     if ( !efi_enabled(EFI_BOOT) )
         return;
 
--- a/xen/common/efi/ebmalloc.c
+++ /dev/null
@@ -1,74 +0,0 @@
-#include "efi.h"
-#include <xen/init.h>
-#include <xen/mm.h>
-
-#ifdef CONFIG_ARM
-/*
- * TODO: Enable EFI boot allocator on ARM.
- * This code can be common for x86 and ARM.
- * Things TODO on ARM before enabling ebmalloc:
- *   - estimate required EBMALLOC_SIZE value,
- *   - where (in which section) ebmalloc_mem[] should live; if in
- *     .bss.page_aligned, as it is right now, then whole BSS zeroing
- *     have to be disabled in xen/arch/arm/arm64/head.S; though BSS
- *     should be initialized somehow before use of variables living there,
- *   - use ebmalloc() in ARM/common EFI boot code,
- *   - call free_ebmalloc_unused_mem() somewhere in init code.
- */
-#define EBMALLOC_SIZE	MB(0)
-#else
-#define EBMALLOC_SIZE	MB(1)
-#endif
-
-static char __section(".bss.page_aligned") __aligned(PAGE_SIZE)
-    ebmalloc_mem[EBMALLOC_SIZE];
-static unsigned long __read_mostly ebmalloc_allocated;
-
-/* EFI boot allocator. */
-void __init *ebmalloc(size_t size)
-{
-    void *ptr = ebmalloc_mem + ebmalloc_allocated;
-
-    ebmalloc_allocated += ROUNDUP(size, sizeof(void *));
-
-    if ( ebmalloc_allocated > sizeof(ebmalloc_mem) )
-        blexit(L"Out of static memory\r\n");
-
-    return ptr;
-}
-
-bool efi_boot_mem_unused(unsigned long *start, unsigned long *end)
-{
-    /* FIXME: Drop once the call here with two NULLs goes away. */
-    if ( !start && !end )
-    {
-        ebmalloc_allocated = sizeof(ebmalloc_mem);
-        return false;
-    }
-
-    *start = (unsigned long)ebmalloc_mem + PAGE_ALIGN(ebmalloc_allocated);
-    *end = (unsigned long)ebmalloc_mem + sizeof(ebmalloc_mem);
-
-    return *start < *end;
-}
-
-void __init free_ebmalloc_unused_mem(void)
-{
-    unsigned long start, end;
-
-    if ( !efi_boot_mem_unused(&start, &end) )
-        return;
-
-    destroy_xen_mappings(start, end);
-
-#ifdef CONFIG_X86
-    /*
-     * By reserving the space early in the E820 map, it gets freed way before
-     * we make it here. Don't free the range a 2nd time.
-     */
-#else
-    init_xenheap_pages(__pa(start), __pa(end));
-#endif
-
-    printk(XENLOG_INFO "Freed %lukB unused BSS memory\n", (end - start) >> 10);
-}
--- a/xen/common/efi/efi-common.mk
+++ b/xen/common/efi/efi-common.mk
@@ -1,4 +1,4 @@
-EFIOBJ-y := boot.init.o pe.init.o ebmalloc.o runtime.o
+EFIOBJ-y := boot.init.o pe.init.o runtime.o
 EFIOBJ-$(CONFIG_COMPAT) += compat.o
 
 CFLAGS-y += -fshort-wchar
--- a/xen/common/efi/efi.h
+++ b/xen/common/efi/efi.h
@@ -48,10 +48,6 @@ void noreturn blexit(const CHAR16 *str);
 
 const CHAR16 *wmemchr(const CHAR16 *s, CHAR16 c, UINTN n);
 
-/* EFI boot allocator. */
-void *ebmalloc(size_t size);
-void free_ebmalloc_unused_mem(void);
-
 const void *pe_find_section(const void *image, const UINTN image_size,
                             const CHAR16 *section_name, UINTN *size_out);
 
--- a/xen/include/xen/efi.h
+++ b/xen/include/xen/efi.h
@@ -39,7 +39,6 @@ static inline bool efi_enabled(unsigned
 extern bool efi_secure_boot;
 
 void efi_init_memory(void);
-bool efi_boot_mem_unused(unsigned long *start, unsigned long *end);
 bool efi_rs_using_pgtables(void);
 unsigned long efi_get_time(void);
 void efi_halt_system(void);

Re: [PATCH 2/3] x86/EFI: replace ebmalloc()

[Marek Marczykowski]

[-- Attachment #1: Type: text/plain, Size: 3063 bytes --]

On Thu, Nov 13, 2025 at 12:09:37PM +0100, Jan Beulich wrote:
> Use the new brk_alloc() instead, with ebmalloc() merely being a thin
> wrapper.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I'm not quite certain whether we ought to permit non-page-granular
> reservations. The in-memory image being somewhat larger due to possibly
> excessive padding isn't really a big problem, I think.

My grep says ebmalloc is used in just two places:
1. For efi_memmap (via efi_arch_allocate_mmap_buffer())
2. For various cmdline options and module names (via aplace_string())

The second one is probably undesirable to allocate full page for each
one. On the other hand, the current approach (putting small allocations
at the same page as an earlier page-aligned one) also has its issues -
see comments on 3/3 patch.

> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -262,7 +262,7 @@ quiet_cmd_obj_init_o = INIT_O  $@
>  define cmd_obj_init_o
>      $(OBJDUMP) -h $< | while read idx name sz rest; do \
>          case "$$name" in \
> -        .*.local) ;; \
> +        .*.local|.bss..brk*) ;; \
>          .text|.text.*|.data|.data.*|.bss|.bss.*) \
>              test $$(echo $$sz | sed 's,00*,0,') != 0 || continue; \
>              echo "Error: size of $<:$$name is 0x$$sz" >&2; \
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -10,6 +10,7 @@
>  #include <xen/vga.h>
>  
>  #include <asm/boot-helpers.h>
> +#include <asm/brk.h>
>  #include <asm/e820.h>
>  #include <asm/edd.h>
>  #include <asm/microcode.h>
> @@ -119,6 +120,18 @@ static void __init relocate_trampoline(u
>      reloc_trampoline64();
>  }
>  
> +DEFINE_BRK(efi, MB(1));
> +
> +static void *__init ebmalloc(size_t size)
> +{
> +    void *ptr = brk_alloc(size);
> +
> +    if ( !ptr )
> +        blexit(L"Out of BRK memory\r\n");
> +
> +    return ptr;
> +}
> +
>  static void __init place_string(u32 *addr, const char *s)
>  {
>      char *alloc = NULL;
> --- a/xen/arch/x86/efi/stub.c
> +++ b/xen/arch/x86/efi/stub.c
> @@ -41,12 +41,4 @@ void __init noreturn efi_multiboot2(EFI_
>  
>  void __init efi_init_memory(void) { }
>  
> -bool efi_boot_mem_unused(unsigned long *start, unsigned long *end)
> -{
> -    /* FIXME: Simplify once the call here with two NULLs goes away. */
> -    if ( start || end )
> -        *start = *end = (unsigned long)_end;
> -    return false;
> -}
> -
>  void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t l4e) { }
> --- a/xen/arch/x86/include/asm/brk.h
> +++ b/xen/arch/x86/include/asm/brk.h
> @@ -2,6 +2,10 @@
>  
>  #include <xen/types.h>
>  
> +#define DEFINE_BRK(var, size) \
> +    static char __section(".bss..brk.page_aligned") __aligned(PAGE_SIZE) \
> +        __used var ## _brk_[size]

This chunk belongs to the previous patch I think.

> +
>  void *brk_alloc(size_t size);
>  unsigned long brk_get_unused_start(void);
>  void brk_free_unused(void);

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/3] x86/EFI: replace ebmalloc()

[Jan Beulich]

On 13.11.2025 13:40, Marek Marczykowski wrote:
> On Thu, Nov 13, 2025 at 12:09:37PM +0100, Jan Beulich wrote:
>> --- a/xen/arch/x86/include/asm/brk.h
>> +++ b/xen/arch/x86/include/asm/brk.h
>> @@ -2,6 +2,10 @@
>>  
>>  #include <xen/types.h>
>>  
>> +#define DEFINE_BRK(var, size) \
>> +    static char __section(".bss..brk.page_aligned") __aligned(PAGE_SIZE) \
>> +        __used var ## _brk_[size]
> 
> This chunk belongs to the previous patch I think.

It could, but it's not used there yet (i.e. would count as dead code).

Jan

Re: [PATCH 2/3] x86/EFI: replace ebmalloc()

[Marek Marczykowski]

[-- Attachment #1: Type: text/plain, Size: 892 bytes --]

On Thu, Nov 13, 2025 at 01:46:20PM +0100, Jan Beulich wrote:
> On 13.11.2025 13:40, Marek Marczykowski wrote:
> > On Thu, Nov 13, 2025 at 12:09:37PM +0100, Jan Beulich wrote:
> >> --- a/xen/arch/x86/include/asm/brk.h
> >> +++ b/xen/arch/x86/include/asm/brk.h
> >> @@ -2,6 +2,10 @@
> >>  
> >>  #include <xen/types.h>
> >>  
> >> +#define DEFINE_BRK(var, size) \
> >> +    static char __section(".bss..brk.page_aligned") __aligned(PAGE_SIZE) \
> >> +        __used var ## _brk_[size]
> > 
> > This chunk belongs to the previous patch I think.
> 
> It could, but it's not used there yet (i.e. would count as dead code).

But in the current shape the linker script change in the first patch is
unused. IOW, I think adding .bss..brk.page_aligned to the linker script
should go together with DEFINE_BRK.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/3] x86/EFI: replace ebmalloc()

[Jan Beulich]

On 13.11.2025 13:59, Marek Marczykowski wrote:
> On Thu, Nov 13, 2025 at 01:46:20PM +0100, Jan Beulich wrote:
>> On 13.11.2025 13:40, Marek Marczykowski wrote:
>>> On Thu, Nov 13, 2025 at 12:09:37PM +0100, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/include/asm/brk.h
>>>> +++ b/xen/arch/x86/include/asm/brk.h
>>>> @@ -2,6 +2,10 @@
>>>>  
>>>>  #include <xen/types.h>
>>>>  
>>>> +#define DEFINE_BRK(var, size) \
>>>> +    static char __section(".bss..brk.page_aligned") __aligned(PAGE_SIZE) \
>>>> +        __used var ## _brk_[size]
>>>
>>> This chunk belongs to the previous patch I think.
>>
>> It could, but it's not used there yet (i.e. would count as dead code).
> 
> But in the current shape the linker script change in the first patch is
> unused. IOW, I think adding .bss..brk.page_aligned to the linker script
> should go together with DEFINE_BRK.

Again - could be done that way, but there's no concept of "dead code" in
the linker script (afaik at least).

Jan

Re: [PATCH 2/3] x86/EFI: replace ebmalloc()

[Marek Marczykowski]

[-- Attachment #1: Type: text/plain, Size: 1353 bytes --]

On Thu, Nov 13, 2025 at 02:24:38PM +0100, Jan Beulich wrote:
> On 13.11.2025 13:59, Marek Marczykowski wrote:
> > On Thu, Nov 13, 2025 at 01:46:20PM +0100, Jan Beulich wrote:
> >> On 13.11.2025 13:40, Marek Marczykowski wrote:
> >>> On Thu, Nov 13, 2025 at 12:09:37PM +0100, Jan Beulich wrote:
> >>>> --- a/xen/arch/x86/include/asm/brk.h
> >>>> +++ b/xen/arch/x86/include/asm/brk.h
> >>>> @@ -2,6 +2,10 @@
> >>>>  
> >>>>  #include <xen/types.h>
> >>>>  
> >>>> +#define DEFINE_BRK(var, size) \
> >>>> +    static char __section(".bss..brk.page_aligned") __aligned(PAGE_SIZE) \
> >>>> +        __used var ## _brk_[size]
> >>>
> >>> This chunk belongs to the previous patch I think.
> >>
> >> It could, but it's not used there yet (i.e. would count as dead code).
> > 
> > But in the current shape the linker script change in the first patch is
> > unused. IOW, I think adding .bss..brk.page_aligned to the linker script
> > should go together with DEFINE_BRK.
> 
> Again - could be done that way, but there's no concept of "dead code" in
> the linker script (afaik at least).

Anyway, I voiced my opinion, but it isn't important enough to block the
patch on it. Either way:

Reviewed-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/3] x86/EFI: replace ebmalloc()

[Jan Beulich]

On 13.11.2025 13:40, Marek Marczykowski wrote:
> On Thu, Nov 13, 2025 at 12:09:37PM +0100, Jan Beulich wrote:
>> Use the new brk_alloc() instead, with ebmalloc() merely being a thin
>> wrapper.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> I'm not quite certain whether we ought to permit non-page-granular
>> reservations. The in-memory image being somewhat larger due to possibly
>> excessive padding isn't really a big problem, I think.
> 
> My grep says ebmalloc is used in just two places:
> 1. For efi_memmap (via efi_arch_allocate_mmap_buffer())
> 2. For various cmdline options and module names (via aplace_string())
> 
> The second one is probably undesirable to allocate full page for each
> one. On the other hand, the current approach (putting small allocations
> at the same page as an earlier page-aligned one) also has its issues -
> see comments on 3/3 patch.

Imo if such sharing of a page is unwanted, then it's the side caring about
the non-sharing which ought to request an exact multiple of pages. Wasting
space due to doing this in the BRK implementation is undesirable.

Jan

Re: [PATCH 2/3] x86/EFI: replace ebmalloc()

[Marek Marczykowski]

[-- Attachment #1: Type: text/plain, Size: 1330 bytes --]

On Thu, Nov 13, 2025 at 01:53:40PM +0100, Jan Beulich wrote:
> On 13.11.2025 13:40, Marek Marczykowski wrote:
> > On Thu, Nov 13, 2025 at 12:09:37PM +0100, Jan Beulich wrote:
> >> Use the new brk_alloc() instead, with ebmalloc() merely being a thin
> >> wrapper.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> I'm not quite certain whether we ought to permit non-page-granular
> >> reservations. The in-memory image being somewhat larger due to possibly
> >> excessive padding isn't really a big problem, I think.
> > 
> > My grep says ebmalloc is used in just two places:
> > 1. For efi_memmap (via efi_arch_allocate_mmap_buffer())
> > 2. For various cmdline options and module names (via aplace_string())
> > 
> > The second one is probably undesirable to allocate full page for each
> > one. On the other hand, the current approach (putting small allocations
> > at the same page as an earlier page-aligned one) also has its issues -
> > see comments on 3/3 patch.
> 
> Imo if such sharing of a page is unwanted, then it's the side caring about
> the non-sharing which ought to request an exact multiple of pages. Wasting
> space due to doing this in the BRK implementation is undesirable.

Makes sense.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 3/3] xhci-dbc: use brk_alloc()

[Jan Beulich]

This way the relatively large chunk of DMA buffers can be freed when the
driver isn't in use.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/char/xhci-dbc.c
+++ b/xen/drivers/char/xhci-dbc.c
@@ -27,6 +27,8 @@
 #include <xen/serial.h>
 #include <xen/timer.h>
 #include <xen/types.h>
+
+#include <asm/brk.h>
 #include <asm/fixmap.h>
 #include <asm/io.h>
 #include <asm/string.h>
@@ -1321,7 +1323,7 @@ static struct uart_driver dbc_uart_drive
 };
 
 /* Those are accessed via DMA. */
-struct dbc_dma_bufs {
+struct __aligned(PAGE_SIZE) dbc_dma_bufs {
     struct xhci_trb evt_trb[DBC_TRB_RING_CAP];
     struct xhci_trb out_trb[DBC_TRB_RING_CAP];
     struct xhci_trb in_trb[DBC_TRB_RING_CAP];
@@ -1335,8 +1337,7 @@ struct dbc_dma_bufs {
      * DMA-reachable by the USB controller.
      */
 };
-static struct dbc_dma_bufs __section(".bss.page_aligned") __aligned(PAGE_SIZE)
-    dbc_dma_bufs;
+DEFINE_BRK(xhci, sizeof(struct dbc_dma_bufs));
 
 static int __init cf_check xhci_parse_dbgp(const char *opt_dbgp)
 {
@@ -1413,24 +1414,33 @@ void __init xhci_dbc_uart_init(void)
 {
     struct dbc_uart *uart = &dbc_uart;
     struct dbc *dbc = &uart->dbc;
+    struct dbc_dma_bufs *dma_bufs;
 
     if ( !dbc->enable )
         return;
 
-    dbc->dbc_ctx = &dbc_dma_bufs.ctx;
-    dbc->dbc_erst = &dbc_dma_bufs.erst;
-    dbc->dbc_ering.trb = dbc_dma_bufs.evt_trb;
-    dbc->dbc_oring.trb = dbc_dma_bufs.out_trb;
-    dbc->dbc_iring.trb = dbc_dma_bufs.in_trb;
-    dbc->dbc_owork.buf = dbc_dma_bufs.out_wrk_buf;
-    dbc->dbc_iwork.buf = dbc_dma_bufs.in_wrk_buf;
-    dbc->dbc_str = dbc_dma_bufs.str_buf;
+    dma_bufs = brk_alloc(sizeof(*dma_bufs));
+    if ( !dma_bufs )
+    {
+        dbc->enable = false;
+        printk(XENLOG_ERR "XHCI: not enough BRK space available\n");
+        return;
+    }
+
+    dbc->dbc_ctx = &dma_bufs->ctx;
+    dbc->dbc_erst = &dma_bufs->erst;
+    dbc->dbc_ering.trb = dma_bufs->evt_trb;
+    dbc->dbc_oring.trb = dma_bufs->out_trb;
+    dbc->dbc_iring.trb = dma_bufs->in_trb;
+    dbc->dbc_owork.buf = dma_bufs->out_wrk_buf;
+    dbc->dbc_iwork.buf = dma_bufs->in_wrk_buf;
+    dbc->dbc_str = dma_bufs->str_buf;
 
     if ( dbc_open(dbc) )
     {
         iommu_add_extra_reserved_device_memory(
-                PFN_DOWN(virt_to_maddr(&dbc_dma_bufs)),
-                PFN_UP(sizeof(dbc_dma_bufs)),
+                PFN_DOWN(virt_to_maddr(dma_bufs)),
+                PFN_DOWN(sizeof(*dma_bufs)),
                 uart->dbc.sbdf,
                 "XHCI console");
         serial_register_uart(SERHND_XHCI, &dbc_uart_driver, &dbc_uart);

Re: [PATCH 3/3] xhci-dbc: use brk_alloc()

[Marek Marczykowski]

[-- Attachment #1: Type: text/plain, Size: 3403 bytes --]

On Thu, Nov 13, 2025 at 12:10:16PM +0100, Jan Beulich wrote:
> This way the relatively large chunk of DMA buffers can be freed when the
> driver isn't in use.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/drivers/char/xhci-dbc.c
> +++ b/xen/drivers/char/xhci-dbc.c
> @@ -27,6 +27,8 @@
>  #include <xen/serial.h>
>  #include <xen/timer.h>
>  #include <xen/types.h>
> +
> +#include <asm/brk.h>
>  #include <asm/fixmap.h>
>  #include <asm/io.h>
>  #include <asm/string.h>
> @@ -1321,7 +1323,7 @@ static struct uart_driver dbc_uart_drive
>  };
>  
>  /* Those are accessed via DMA. */
> -struct dbc_dma_bufs {
> +struct __aligned(PAGE_SIZE) dbc_dma_bufs {
>      struct xhci_trb evt_trb[DBC_TRB_RING_CAP];
>      struct xhci_trb out_trb[DBC_TRB_RING_CAP];
>      struct xhci_trb in_trb[DBC_TRB_RING_CAP];
> @@ -1335,8 +1337,7 @@ struct dbc_dma_bufs {
>       * DMA-reachable by the USB controller.
>       */
>  };
> -static struct dbc_dma_bufs __section(".bss.page_aligned") __aligned(PAGE_SIZE)
> -    dbc_dma_bufs;
> +DEFINE_BRK(xhci, sizeof(struct dbc_dma_bufs));

I think with this change (or rather with using brk_alloc() below), the
structure wants to be padded up to the page size, to avoid putting
anything else on the same page (see comment just outside of context
above). Previously .bss.page_aligned contained (hopefully) only
page-aligned objects, but now brk_alloc() can combine them.

>  
>  static int __init cf_check xhci_parse_dbgp(const char *opt_dbgp)
>  {
> @@ -1413,24 +1414,33 @@ void __init xhci_dbc_uart_init(void)
>  {
>      struct dbc_uart *uart = &dbc_uart;
>      struct dbc *dbc = &uart->dbc;
> +    struct dbc_dma_bufs *dma_bufs;
>  
>      if ( !dbc->enable )
>          return;
>  
> -    dbc->dbc_ctx = &dbc_dma_bufs.ctx;
> -    dbc->dbc_erst = &dbc_dma_bufs.erst;
> -    dbc->dbc_ering.trb = dbc_dma_bufs.evt_trb;
> -    dbc->dbc_oring.trb = dbc_dma_bufs.out_trb;
> -    dbc->dbc_iring.trb = dbc_dma_bufs.in_trb;
> -    dbc->dbc_owork.buf = dbc_dma_bufs.out_wrk_buf;
> -    dbc->dbc_iwork.buf = dbc_dma_bufs.in_wrk_buf;
> -    dbc->dbc_str = dbc_dma_bufs.str_buf;
> +    dma_bufs = brk_alloc(sizeof(*dma_bufs));
> +    if ( !dma_bufs )
> +    {
> +        dbc->enable = false;
> +        printk(XENLOG_ERR "XHCI: not enough BRK space available\n");
> +        return;
> +    }
> +
> +    dbc->dbc_ctx = &dma_bufs->ctx;
> +    dbc->dbc_erst = &dma_bufs->erst;
> +    dbc->dbc_ering.trb = dma_bufs->evt_trb;
> +    dbc->dbc_oring.trb = dma_bufs->out_trb;
> +    dbc->dbc_iring.trb = dma_bufs->in_trb;
> +    dbc->dbc_owork.buf = dma_bufs->out_wrk_buf;
> +    dbc->dbc_iwork.buf = dma_bufs->in_wrk_buf;
> +    dbc->dbc_str = dma_bufs->str_buf;
>  
>      if ( dbc_open(dbc) )
>      {
>          iommu_add_extra_reserved_device_memory(
> -                PFN_DOWN(virt_to_maddr(&dbc_dma_bufs)),
> -                PFN_UP(sizeof(dbc_dma_bufs)),
> +                PFN_DOWN(virt_to_maddr(dma_bufs)),
> +                PFN_DOWN(sizeof(*dma_bufs)),

Is that really correct? But with padding (see earlier comment) it
shouldn't really matter.

>                  uart->dbc.sbdf,
>                  "XHCI console");
>          serial_register_uart(SERHND_XHCI, &dbc_uart_driver, &dbc_uart);
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] xhci-dbc: use brk_alloc()

[Jan Beulich]

On 13.11.2025 13:39, Marek Marczykowski wrote:
> On Thu, Nov 13, 2025 at 12:10:16PM +0100, Jan Beulich wrote:
>> --- a/xen/drivers/char/xhci-dbc.c
>> +++ b/xen/drivers/char/xhci-dbc.c
>> @@ -27,6 +27,8 @@
>>  #include <xen/serial.h>
>>  #include <xen/timer.h>
>>  #include <xen/types.h>
>> +
>> +#include <asm/brk.h>
>>  #include <asm/fixmap.h>
>>  #include <asm/io.h>
>>  #include <asm/string.h>
>> @@ -1321,7 +1323,7 @@ static struct uart_driver dbc_uart_drive
>>  };
>>  
>>  /* Those are accessed via DMA. */
>> -struct dbc_dma_bufs {
>> +struct __aligned(PAGE_SIZE) dbc_dma_bufs {
>>      struct xhci_trb evt_trb[DBC_TRB_RING_CAP];
>>      struct xhci_trb out_trb[DBC_TRB_RING_CAP];
>>      struct xhci_trb in_trb[DBC_TRB_RING_CAP];
>> @@ -1335,8 +1337,7 @@ struct dbc_dma_bufs {
>>       * DMA-reachable by the USB controller.
>>       */
>>  };
>> -static struct dbc_dma_bufs __section(".bss.page_aligned") __aligned(PAGE_SIZE)
>> -    dbc_dma_bufs;
>> +DEFINE_BRK(xhci, sizeof(struct dbc_dma_bufs));
> 
> I think with this change (or rather with using brk_alloc() below), the
> structure wants to be padded up to the page size, to avoid putting
> anything else on the same page (see comment just outside of context
> above).

Are you sure? My understanding is that sizeof(xyz) is always evenly divisible by
alignof(xyz). Hence such padding doesn't need making explicit. (And yes, I did
see that comment while making the change.)

>> @@ -1413,24 +1414,33 @@ void __init xhci_dbc_uart_init(void)
>>  {
>>      struct dbc_uart *uart = &dbc_uart;
>>      struct dbc *dbc = &uart->dbc;
>> +    struct dbc_dma_bufs *dma_bufs;
>>  
>>      if ( !dbc->enable )
>>          return;
>>  
>> -    dbc->dbc_ctx = &dbc_dma_bufs.ctx;
>> -    dbc->dbc_erst = &dbc_dma_bufs.erst;
>> -    dbc->dbc_ering.trb = dbc_dma_bufs.evt_trb;
>> -    dbc->dbc_oring.trb = dbc_dma_bufs.out_trb;
>> -    dbc->dbc_iring.trb = dbc_dma_bufs.in_trb;
>> -    dbc->dbc_owork.buf = dbc_dma_bufs.out_wrk_buf;
>> -    dbc->dbc_iwork.buf = dbc_dma_bufs.in_wrk_buf;
>> -    dbc->dbc_str = dbc_dma_bufs.str_buf;
>> +    dma_bufs = brk_alloc(sizeof(*dma_bufs));
>> +    if ( !dma_bufs )
>> +    {
>> +        dbc->enable = false;
>> +        printk(XENLOG_ERR "XHCI: not enough BRK space available\n");
>> +        return;
>> +    }
>> +
>> +    dbc->dbc_ctx = &dma_bufs->ctx;
>> +    dbc->dbc_erst = &dma_bufs->erst;
>> +    dbc->dbc_ering.trb = dma_bufs->evt_trb;
>> +    dbc->dbc_oring.trb = dma_bufs->out_trb;
>> +    dbc->dbc_iring.trb = dma_bufs->in_trb;
>> +    dbc->dbc_owork.buf = dma_bufs->out_wrk_buf;
>> +    dbc->dbc_iwork.buf = dma_bufs->in_wrk_buf;
>> +    dbc->dbc_str = dma_bufs->str_buf;
>>  
>>      if ( dbc_open(dbc) )
>>      {
>>          iommu_add_extra_reserved_device_memory(
>> -                PFN_DOWN(virt_to_maddr(&dbc_dma_bufs)),
>> -                PFN_UP(sizeof(dbc_dma_bufs)),
>> +                PFN_DOWN(virt_to_maddr(dma_bufs)),
>> +                PFN_DOWN(sizeof(*dma_bufs)),
> 
> Is that really correct? But with padding (see earlier comment) it
> shouldn't really matter.

I think this is addressed by the reply further up as well.

Jan

Re: [PATCH 3/3] xhci-dbc: use brk_alloc()

[Marek Marczykowski]

[-- Attachment #1: Type: text/plain, Size: 3695 bytes --]

On Thu, Nov 13, 2025 at 01:50:28PM +0100, Jan Beulich wrote:
> On 13.11.2025 13:39, Marek Marczykowski wrote:
> > On Thu, Nov 13, 2025 at 12:10:16PM +0100, Jan Beulich wrote:
> >> --- a/xen/drivers/char/xhci-dbc.c
> >> +++ b/xen/drivers/char/xhci-dbc.c
> >> @@ -27,6 +27,8 @@
> >>  #include <xen/serial.h>
> >>  #include <xen/timer.h>
> >>  #include <xen/types.h>
> >> +
> >> +#include <asm/brk.h>
> >>  #include <asm/fixmap.h>
> >>  #include <asm/io.h>
> >>  #include <asm/string.h>
> >> @@ -1321,7 +1323,7 @@ static struct uart_driver dbc_uart_drive
> >>  };
> >>  
> >>  /* Those are accessed via DMA. */
> >> -struct dbc_dma_bufs {
> >> +struct __aligned(PAGE_SIZE) dbc_dma_bufs {
> >>      struct xhci_trb evt_trb[DBC_TRB_RING_CAP];
> >>      struct xhci_trb out_trb[DBC_TRB_RING_CAP];
> >>      struct xhci_trb in_trb[DBC_TRB_RING_CAP];
> >> @@ -1335,8 +1337,7 @@ struct dbc_dma_bufs {
> >>       * DMA-reachable by the USB controller.
> >>       */
> >>  };
> >> -static struct dbc_dma_bufs __section(".bss.page_aligned") __aligned(PAGE_SIZE)
> >> -    dbc_dma_bufs;
> >> +DEFINE_BRK(xhci, sizeof(struct dbc_dma_bufs));
> > 
> > I think with this change (or rather with using brk_alloc() below), the
> > structure wants to be padded up to the page size, to avoid putting
> > anything else on the same page (see comment just outside of context
> > above).
> 
> Are you sure? My understanding is that sizeof(xyz) is always evenly divisible by
> alignof(xyz). Hence such padding doesn't need making explicit. (And yes, I did
> see that comment while making the change.)

Ok, then indeed the added (or rather moved) __aligned(PAGE_SIZE) is enough.

Reviewed-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

> >> @@ -1413,24 +1414,33 @@ void __init xhci_dbc_uart_init(void)
> >>  {
> >>      struct dbc_uart *uart = &dbc_uart;
> >>      struct dbc *dbc = &uart->dbc;
> >> +    struct dbc_dma_bufs *dma_bufs;
> >>  
> >>      if ( !dbc->enable )
> >>          return;
> >>  
> >> -    dbc->dbc_ctx = &dbc_dma_bufs.ctx;
> >> -    dbc->dbc_erst = &dbc_dma_bufs.erst;
> >> -    dbc->dbc_ering.trb = dbc_dma_bufs.evt_trb;
> >> -    dbc->dbc_oring.trb = dbc_dma_bufs.out_trb;
> >> -    dbc->dbc_iring.trb = dbc_dma_bufs.in_trb;
> >> -    dbc->dbc_owork.buf = dbc_dma_bufs.out_wrk_buf;
> >> -    dbc->dbc_iwork.buf = dbc_dma_bufs.in_wrk_buf;
> >> -    dbc->dbc_str = dbc_dma_bufs.str_buf;
> >> +    dma_bufs = brk_alloc(sizeof(*dma_bufs));
> >> +    if ( !dma_bufs )
> >> +    {
> >> +        dbc->enable = false;
> >> +        printk(XENLOG_ERR "XHCI: not enough BRK space available\n");
> >> +        return;
> >> +    }
> >> +
> >> +    dbc->dbc_ctx = &dma_bufs->ctx;
> >> +    dbc->dbc_erst = &dma_bufs->erst;
> >> +    dbc->dbc_ering.trb = dma_bufs->evt_trb;
> >> +    dbc->dbc_oring.trb = dma_bufs->out_trb;
> >> +    dbc->dbc_iring.trb = dma_bufs->in_trb;
> >> +    dbc->dbc_owork.buf = dma_bufs->out_wrk_buf;
> >> +    dbc->dbc_iwork.buf = dma_bufs->in_wrk_buf;
> >> +    dbc->dbc_str = dma_bufs->str_buf;
> >>  
> >>      if ( dbc_open(dbc) )
> >>      {
> >>          iommu_add_extra_reserved_device_memory(
> >> -                PFN_DOWN(virt_to_maddr(&dbc_dma_bufs)),
> >> -                PFN_UP(sizeof(dbc_dma_bufs)),
> >> +                PFN_DOWN(virt_to_maddr(dma_bufs)),
> >> +                PFN_DOWN(sizeof(*dma_bufs)),
> > 
> > Is that really correct? But with padding (see earlier comment) it
> > shouldn't really matter.
> 
> I think this is addressed by the reply further up as well.
> 
> Jan

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/3] x86: "brk" allocator

[Andrew Cooper]

On 13/11/2025 11:06 am, Jan Beulich wrote:
> Derive for our purposes something similar to what Linux has been having for
> a long while.
>
> 1: x86: introduce "brk" allocator
> 2: x86/EFI: replace ebmalloc()
> 3: xhci-dbc: use brk_alloc()
>
> To reduce padding holes, .bss.page_aligned and per-CPU data may want moving
> immediately ahead of __brk_start[]. Albeit then the tail of per-CPU data will
> all be padding space; sadly the TSS wants/needs page-aligning for XPTI
> purposes.

I've not done this yet, but I was intending to make the TSSes be
dynamically allocated, so they (along with the IDTs) can be omitted
entirely under FRED.

The VT-x tr limit bug/misfeature complicates this somewhat, hence why
I've left this out of the FRED basic support, but it can be addressed by
pointing HOST_TR_BASE at a singleton TSS.

~Andrew