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

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

On Tue, Mar 12, 2024 at 01:38:53PM +0100, Jan Beulich wrote:
> On 12.03.2024 13:02, 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
> > 
> > In that case, is the approach with
> > iommu_get_extra_reserved_device_memory() okay (and a comment that it
> > will have a side effect...) ?
> 
> Counter question: You not having gone that route despite our talking
> about it on Matrix must have a reason. When seeing this approach I
> concluded something got in the way. What's the deal?

I added a note about that in the patch (below commit message):
> As an alternative implementation I have considered changing
> iommu_get_extra_reserved_device_memory() to modify memory map. But that
> may hide (or cause) some other issues when this API will gain some more
> users in the future.

More specifically, if some future use would be on a non-RAM area, or
already reserved area. But if we can ignore those cases for now, I'm
fine with that approach.

> >>> 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.
> > 
> > Ok, I'll add a comment about that. I guess, if needed in the future,
> > iommu_add_extra_reserved_device_memory() can gain an extra parameter to
> > distinguish RAM from non-RAM mappings.
> > 
> > BTW should e820_change_range_type() return 1 in case of mapping already
> > having the right type? Otherwise, if one wants to use
> > iommu_add_extra_reserved_device_memory() on already reserved memory, the
> > e820_change_range_type() would fail.
> 
> You raised that question on Matrix yesterday, iirc, and I left it
> unanswered there because it takes archeology to find the answer (or at
> least get closer to one). And, please don't get me wrong, you could as
> well do that yourself. (My vague recollection from having dealt with
> similar code in Linux is that yes, in the example given the function
> ought to indeed have failed back then. Depending on present uses etc
> it may well be that we could reconsider, though.)

I sure can do some archaeology there, I was just hoping any of you would
know the answer already.

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

Attachment: signature.asc
Description: PGP signature