Re: [PATCH v2 2/2] drivers/char: mark extra reserved device memory in memory map

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.03.2024 17:25, 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 (due
> to IOMMU faults). The pages chosen by the XHCI console driver should
> still be usable by the CPU though, and the console code already can deal
> with too slow console by dropping characters (and console not printing
> anything is a special case of "slow"). When reserving fails print an error
> message showing which pages failed and who requested them. This should
> be enough hint to find why XHCI console doesn't work.
> 
> Fixes: 3a1a7b809ffa "drivers/char: mark DMA buffers as reserved for the XHCI"
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> ---
> Alternative error handling could be a panic, but with this version I
> think it can be avoided. And not panicing gives a better chance to
> actually see the error message (from the hopefully started dom0),
> especially as the affected driver is the console one.
> 
> 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...

These are normal PCI devices, so I don't see why they shouldn't be usable
on non-x86 systems. But this is all okay as long as XHCI depends on X86
in Kconfig.

> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -21,6 +21,9 @@
>  #include <xen/softirq.h>
>  #include <xen/keyhandler.h>
>  #include <xsm/xsm.h>
> +#ifdef CONFIG_X86
> +#include <asm/e820.h>
> +#endif

This could do with a separating newline.

> @@ -715,6 +718,22 @@ int __init 
> iommu_get_extra_reserved_device_memory(iommu_grdm_t *func,
>  
>      for ( idx = 0; idx < nr_extra_reserved_ranges; idx++ )
>      {
> +#ifdef CONFIG_X86
> +        if ( !reserve_e820_ram(
> +                &e820,
> +                pfn_to_paddr(extra_reserved_ranges[idx].start),
> +                pfn_to_paddr(extra_reserved_ranges[idx].start +
> +                             extra_reserved_ranges[idx].nr)) )

Indentation is odd here - it should be one level down from the start
of the function name. That side, code here and ...

> +        {
> +            printk(XENLOG_ERR "Failed to reserve [%"PRIx64"-%"PRIx64") for 
> %s, "
> +                   "skipping IOMMU mapping for it, some functionality may be 
> broken\n",
> +                   pfn_to_paddr(extra_reserved_ranges[idx].start),
> +                   pfn_to_paddr(extra_reserved_ranges[idx].start +
> +                                extra_reserved_ranges[idx].nr),

... here would likely benefit from introducing "start" and "end"
local variables.

> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -324,7 +324,8 @@ struct iommu_ops {
>  };
>  
>  /*
> - * To be called by Xen internally, to register extra RMRR/IVMD ranges.
> + * To be called by Xen internally, to register extra RMRR/IVMD ranges for RAM
> + * pages.
>   * Needs to be called before IOMMU initialization.
>   */
>  extern int iommu_add_extra_reserved_device_memory(unsigned long start,
> @@ -334,6 +335,8 @@ extern int 
> iommu_add_extra_reserved_device_memory(unsigned long start,
>  /*
>   * To be called by specific IOMMU driver during initialization,
>   * to fetch ranges registered with iommu_add_extra_reserved_device_memory().
> + * This has a side effect of marking requested ranges as "reserverd" in the

Nit: "reserved"

Jan

> + * memory map.
>   */
>  extern int iommu_get_extra_reserved_device_memory(iommu_grdm_t *func,
>                                                    void *ctxt);