Re: [PATCH] drivers/char: mark XHCI DMA buffers reserved in memory map

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

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).
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.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature