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 <marma...@invisiblethingslab.com>
> ---
> 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.
