| Message ID | 20240312162541.384793-2-marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] IOMMU: store name for extra reserved device memory | expand |
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@invisiblethingslab.com> > --- > 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);
On Mon, Mar 18, 2024 at 02:48:09PM +0100, Jan Beulich wrote: > 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@invisiblethingslab.com> > > --- > > 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. That's why I'm asking for similar API for ARM. The x86-specific part here is only about IOMMU, other parts should work just fine (but as I said, I have no way to test). Anyway, I'll leave it to whoever will need this driver on ARM (or other arch).
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index 03587c0cd680..a311a37a2a03 100644 --- 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 unsigned int __read_mostly iommu_dev_iotlb_timeout = 1000; integer_param("iommu_dev_iotlb_timeout", iommu_dev_iotlb_timeout); @@ -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)) ) + { + 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), + extra_reserved_ranges[idx].name); + continue; + } +#endif ret = func(extra_reserved_ranges[idx].start, extra_reserved_ranges[idx].nr, extra_reserved_ranges[idx].sbdf.sbdf, diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index b7829dff4588..875eaeb90167 100644 --- 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 + * memory map. */ extern int iommu_get_extra_reserved_device_memory(iommu_grdm_t *func, void *ctxt);
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@invisiblethingslab.com> --- 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... Changes in v2: - move reserving to iommu_get_extra_reserved_device_memory() to cover all users of iommu_add_extra_reserved_device_memory() - change error handling to not panic, as in this code layout it can skip sending the pages to the IOMMU driver --- xen/drivers/passthrough/iommu.c | 19 +++++++++++++++++++ xen/include/xen/iommu.h | 5 ++++- 2 files changed, 23 insertions(+), 1 deletion(-)