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);

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

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

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