On 12.03.2024 15:49, Marek Marczykowski-Górecki wrote: > On Tue, Mar 12, 2024 at 03:37:15PM +0100, Jan Beulich wrote: >> On 12.03.2024 15:24, Marek Marczykowski-Górecki wrote: >>> On Tue, Mar 12, 2024 at 11:53:46AM +0100, Jan Beulich wrote: >>>> On 12.03.2024 11:24, Roger Pau Monné wrote: >>>>>> --- 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, >>>> >>>> +1 >>>> >>>>> but I'm >>>>> unsure whether the intention is for the interface to always be used >>>>> against RAM. >>>> >>>> I think we can work from that assumption for now. >>> >>> One more question - what should be the error handling in this case? >> >> My first reaction here is - please first propose something that's >> sensible from your perspective, and then we can go from there. That's >> generally easier that discussion without seeing involved code. >> However, ... >> >>> At >>> this stage, if reserving fails, I can still skip giving this range to >>> the IOMMU driver, which (most likely) will result in IOMMU faults and >>> in-operational device (xhci console). Since I don't know (theoretically) >>> what driver requested the range, the error message can only contain an >>> address and device, so will be a bit less actionable for the user >>> (although it should be quite easy to notice the BDF being the XHCI one). >>> >>> Panic surely is safer option, but less user friendly, especially since >>> (due to the above) I cannot give explicit hint to disable XHCI console. >> >> ... reading this I was meaning to ... >> >>> And kinda independently - I'm tempted to add another field to `struct >>> extra_reserved_range` (and an argument to >>> `iommu_add_extra_reserved_device_memory()`) - textual description, for >>> the error reporting purpose. >> >> ... suggest minimally this. We may want to go farther, though: The party >> registering the range could also supply a callback, to be invoked in >> case registration fails. That callback could then undo whatever is >> necessary in order to not use the memory range in question. >> >> That said - isn't all of this over-engineering, as the allocated memory >> range must have come from a valid RAM region? In which case a simple >> BUG_ON() may be all that's needed (and will never trigger in practice, >> unless we truly screwed up somewhere)? > > In this case (with a single use of > iommu_add_extra_reserved_device_memory()), it will be valid RAM. But > reserving may fail for other reasons too, for example overflow of the > E820 map. > > Undoing things certainly is possible, but quite complicated (none of the > involved serial console APIs support disabling/unregistering a console).
True. I was rather thinking of disabling the actual I/O paths. Jan > Given the simplicity of the workaround user can do (not enabling xhci > console), I don't think it's worth going this route. > > Anyway, I'll post v2 with some version of the above and we can continue > discussion there. >
