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 <[email protected]> > >> @@ -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
signature.asc
Description: PGP signature
