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

[Jan Beulich <jbeulich@xxxxxxxx>]

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