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)? Jan
