Re: [PATCH] drivers/char: mark XHCI DMA buffers reserved in memory map

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Mar 11, 2024 at 09:33:11PM +0100, Marek Marczykowski-Górecki wrote:
> The IOMMU driver checks if RMRR/IVMD are marked as reserved in memory
> map. This should be true for addresses coming from the firmware, but
> when extra pages used by Xen itself are included in the mapping, those
> are taken from usable RAM used. Mark those pages as reserved too.
> 
> Not marking the pages as reserved didn't caused issues before due to
> another a bug in IOMMU driver code, that was fixed in 83afa3135830
> ("amd-vi: fix IVMD memory type checks").
> 
> Failing to reserve memory will lead to panic in IOMMU setup code. And
> not including the page in IOMMU mapping will lead to broken console (on
> due to IOMMU faults). Handling failure with panic() isn't the most user
> friendly thing, but at this stage the alternative would require undoing
> a lot of console init. Since the user can do it much easier - by simply
> not enabling xhci console next time, say that and panic.
> 
> Fixes: 3a1a7b809ffa "drivers/char: mark DMA buffers as reserved for the XHCI"
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> ---
> As an alternative implementation I have considered changing
> iommu_get_extra_reserved_device_memory() to modify memory map. But that
> may hide (or cause) some other issues when this API will gain some more
> users in the future.
> 
> The reserve_e820_ram() is x86-specific. Is there some equivalent API for
> ARM, or maybe even some abstract one? That said, I have no way to test
> XHCI console on ARM, I don't know if such hardware even exists...
> ---
>  xen/arch/x86/setup.c        |  3 +++
>  xen/drivers/char/xhci-dbc.c | 22 ++++++++++++++++++++++
>  xen/include/xen/serial.h    |  2 ++
>  3 files changed, 27 insertions(+)
> 
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index a21984b1ccd8..8ab89b3710ed 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1806,6 +1806,9 @@ void asmlinkage __init noreturn __start_xen(unsigned 
> long mbi_p)
>      mmio_ro_ranges = rangeset_new(NULL, "r/o mmio ranges",
>                                    RANGESETF_prettyprint_hex);
>  
> +    /* Needs to happen after E820 processing but before IOMMU init */
> +    xhci_dbc_uart_reserve_ram();

Overall it might be better if some generic solution for all users of
iommu_add_extra_reserved_device_memory() could be implemented, but I'm
unsure whether the intention is for the interface to always be used
against RAM.

>      xsm_multiboot_init(module_map, mbi);
>  
>      /*
> diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
> index 3bf389be7d0b..e31f3cba7838 100644
> --- a/xen/drivers/char/xhci-dbc.c
> +++ b/xen/drivers/char/xhci-dbc.c
> @@ -31,6 +31,9 @@
>  #include <asm/io.h>
>  #include <asm/string.h>
>  #include <asm/system.h>
> +#ifdef CONFIG_X86
> +#include <asm/e820.h>
> +#endif
>  
>  /* uncomment to have dbc_uart_dump() debug function */
>  /* #define DBC_DEBUG 1 */
> @@ -1426,6 +1429,25 @@ void __init xhci_dbc_uart_init(void)
>      }
>  }
>  
> +void __init xhci_dbc_uart_reserve_ram(void)
> +{
> +    struct dbc *dbc = &dbc_uart.dbc;

const.  Or seeing as it's used only once you could just use
dbc_uart.dbc.enable.

> +
> +    if ( !dbc->enable )
> +        return;
> +
> +#ifdef CONFIG_X86
> +    if ( !reserve_e820_ram(
> +            &e820,
> +            virt_to_maddr(&dbc_dma_bufs),
> +            virt_to_maddr(&dbc_dma_bufs) + sizeof(dbc_dma_bufs) - 1) )

It would be best to align this up:
PAGE_ALIGN(virt_to_maddr(&dbc_dma_bufs) + sizeof(dbc_dma_bufs))

The '- 1' is wrong, as reserve_e820_ram() expects a non-inclusive end
parameter.

> +        panic("Failed to reserve XHCI DMA buffers (%"PRIx64"-%"PRIx64"), "

We usually represent inclusive memory ranges as "[%lx, %lx]".

> +              "disable xhci console to work around\n",
> +              virt_to_maddr(&dbc_dma_bufs),
> +              virt_to_maddr(&dbc_dma_bufs) + sizeof(dbc_dma_bufs) - 1);
> +#endif
> +}

Won't it be best to make the whole function guarded by CONFIG_X86? So
that attempts to use it on !X86 will get a build failure and clearly
notice some work is needed in order to use the function on other
arches?

Thanks, Roger.