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

[Jan Beulich <jbeulich@xxxxxxxx>]

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