The IOMMU driver checks if RMRR/IVMD are marked as reserved in memory
map. This should be true for addresses coming from the firmware, but
when extra pages used by Xen itself are included in the mapping, those
are taken from usable RAM used. Mark those pages as reserved too.

Not marking the pages as reserved didn't caused issues before due to
another a bug in IOMMU driver code, that was fixed in 83afa3135830
("amd-vi: fix IVMD memory type checks").

Failing to reserve memory will lead to panic in IOMMU setup code. And
not including the page in IOMMU mapping will lead to broken console (on
due to IOMMU faults). Handling failure with panic() isn't the most user
friendly thing, but at this stage the alternative would require undoing
a lot of console init. Since the user can do it much easier - by simply
not enabling xhci console next time, say that and panic.

Fixes: 3a1a7b809ffa "drivers/char: mark DMA buffers as reserved for the XHCI"
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
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.

The reserve_e820_ram() is x86-specific. Is there some equivalent API for
ARM, or maybe even some abstract one? That said, I have no way to test
XHCI console on ARM, I don't know if such hardware even exists...
---
 xen/arch/x86/setup.c        |  3 +++
 xen/drivers/char/xhci-dbc.c | 22 ++++++++++++++++++++++
 xen/include/xen/serial.h    |  2 ++
 3 files changed, 27 insertions(+)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index a21984b1ccd8..8ab89b3710ed 100644
--- 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();
+
     xsm_multiboot_init(module_map, mbi);
 
     /*
diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
index 3bf389be7d0b..e31f3cba7838 100644
--- a/xen/drivers/char/xhci-dbc.c
+++ b/xen/drivers/char/xhci-dbc.c
@@ -31,6 +31,9 @@
 #include <asm/io.h>
 #include <asm/string.h>
 #include <asm/system.h>
+#ifdef CONFIG_X86
+#include <asm/e820.h>
+#endif
 
 /* uncomment to have dbc_uart_dump() debug function */
 /* #define DBC_DEBUG 1 */
@@ -1426,6 +1429,25 @@ void __init xhci_dbc_uart_init(void)
     }
 }
 
+void __init xhci_dbc_uart_reserve_ram(void)
+{
+    struct dbc *dbc = &dbc_uart.dbc;
+
+    if ( !dbc->enable )
+        return;
+
+#ifdef CONFIG_X86
+    if ( !reserve_e820_ram(
+            &e820,
+            virt_to_maddr(&dbc_dma_bufs),
+            virt_to_maddr(&dbc_dma_bufs) + sizeof(dbc_dma_bufs) - 1) )
+        panic("Failed to reserve XHCI DMA buffers (%"PRIx64"-%"PRIx64"), "
+              "disable xhci console to work around\n",
+              virt_to_maddr(&dbc_dma_bufs),
+              virt_to_maddr(&dbc_dma_bufs) + sizeof(dbc_dma_bufs) - 1);
+#endif
+}
+
 #ifdef DBC_DEBUG
 static void dbc_dump(struct dbc *dbc)
 {
diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
index 3d21207a3d7a..bb48eb8e8bd9 100644
--- a/xen/include/xen/serial.h
+++ b/xen/include/xen/serial.h
@@ -162,8 +162,10 @@ void ns16550_init(int index, struct ns16550_defaults *defaults);
 void ehci_dbgp_init(void);
 #ifdef CONFIG_XHCI
 void xhci_dbc_uart_init(void);
+void xhci_dbc_uart_reserve_ram(void);
 #else
 static void inline xhci_dbc_uart_init(void) {}
+static void inline xhci_dbc_uart_reserve_ram(void) {}
 #endif
 
 void arm_uart_init(void);
-- 
2.43.0

On Mon, Mar 11, 2024 at 09:33:11PM +0100, Marek Marczykowski-Górecki wrote:
> The IOMMU driver checks if RMRR/IVMD are marked as reserved in memory
> map. This should be true for addresses coming from the firmware, but
> when extra pages used by Xen itself are included in the mapping, those
> are taken from usable RAM used. Mark those pages as reserved too.
> 
> Not marking the pages as reserved didn't caused issues before due to
> another a bug in IOMMU driver code, that was fixed in 83afa3135830
> ("amd-vi: fix IVMD memory type checks").
> 
> Failing to reserve memory will lead to panic in IOMMU setup code. And
> not including the page in IOMMU mapping will lead to broken console (on
> due to IOMMU faults). Handling failure with panic() isn't the most user
> friendly thing, but at this stage the alternative would require undoing
> a lot of console init. Since the user can do it much easier - by simply
> not enabling xhci console next time, say that and panic.
> 
> Fixes: 3a1a7b809ffa "drivers/char: mark DMA buffers as reserved for the XHCI"
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
> 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.
> 
> The reserve_e820_ram() is x86-specific. Is there some equivalent API for
> ARM, or maybe even some abstract one? That said, I have no way to test
> XHCI console on ARM, I don't know if such hardware even exists...
> ---
>  xen/arch/x86/setup.c        |  3 +++
>  xen/drivers/char/xhci-dbc.c | 22 ++++++++++++++++++++++
>  xen/include/xen/serial.h    |  2 ++
>  3 files changed, 27 insertions(+)
> 
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index a21984b1ccd8..8ab89b3710ed 100644
> --- 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, but I'm
unsure whether the intention is for the interface to always be used
against RAM.

>      xsm_multiboot_init(module_map, mbi);
>  
>      /*
> diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
> index 3bf389be7d0b..e31f3cba7838 100644
> --- a/xen/drivers/char/xhci-dbc.c
> +++ b/xen/drivers/char/xhci-dbc.c
> @@ -31,6 +31,9 @@
>  #include <asm/io.h>
>  #include <asm/string.h>
>  #include <asm/system.h>
> +#ifdef CONFIG_X86
> +#include <asm/e820.h>
> +#endif
>  
>  /* uncomment to have dbc_uart_dump() debug function */
>  /* #define DBC_DEBUG 1 */
> @@ -1426,6 +1429,25 @@ void __init xhci_dbc_uart_init(void)
>      }
>  }
>  
> +void __init xhci_dbc_uart_reserve_ram(void)
> +{
> +    struct dbc *dbc = &dbc_uart.dbc;

const.  Or seeing as it's used only once you could just use
dbc_uart.dbc.enable.

> +
> +    if ( !dbc->enable )
> +        return;
> +
> +#ifdef CONFIG_X86
> +    if ( !reserve_e820_ram(
> +            &e820,
> +            virt_to_maddr(&dbc_dma_bufs),
> +            virt_to_maddr(&dbc_dma_bufs) + sizeof(dbc_dma_bufs) - 1) )

It would be best to align this up:
PAGE_ALIGN(virt_to_maddr(&dbc_dma_bufs) + sizeof(dbc_dma_bufs))

The '- 1' is wrong, as reserve_e820_ram() expects a non-inclusive end
parameter.

> +        panic("Failed to reserve XHCI DMA buffers (%"PRIx64"-%"PRIx64"), "

We usually represent inclusive memory ranges as "[%lx, %lx]".

> +              "disable xhci console to work around\n",
> +              virt_to_maddr(&dbc_dma_bufs),
> +              virt_to_maddr(&dbc_dma_bufs) + sizeof(dbc_dma_bufs) - 1);
> +#endif
> +}

Won't it be best to make the whole function guarded by CONFIG_X86? So
that attempts to use it on !X86 will get a build failure and clearly
notice some work is needed in order to use the function on other
arches?

Thanks, Roger.

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.

Jan

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

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.

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

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

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

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

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

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

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

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?

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

Jan

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

On Tue, Mar 12, 2024 at 02:09:14PM +0100, Marek Marczykowski-Górecki wrote:
> On Tue, Mar 12, 2024 at 01:38:53PM +0100, Jan Beulich wrote:
> > On 12.03.2024 13:02, Marek Marczykowski-Górecki wrote:
> > > 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.

None of the commit messages touching that code give the answer. But
looking around, most callers of reserve_e820_ram() ignore its return
value. One exception is reserving memory for kexec. I guess in that case
it may be intentional to fail if the area is reserved already, as it may
indicate it cannot be used for kexec. Is that correct?

There are also a couple of calls to e820_change_range_type() in tboot
code where it changes E820_RESERVED to E820_UNUSABLE. There, I guess
changing e820_change_range_type() behavior would be okay.

Anyway, since we agree to focus on RAM in this API for now, it shouldn't
matter anymore.

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

On 12.03.2024 14:22, Marek Marczykowski-Górecki wrote:
> On Tue, Mar 12, 2024 at 02:09:14PM +0100, Marek Marczykowski-Górecki wrote:
>> On Tue, Mar 12, 2024 at 01:38:53PM +0100, Jan Beulich wrote:
>>> On 12.03.2024 13:02, Marek Marczykowski-Górecki wrote:
>>>> 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.
> 
> None of the commit messages touching that code give the answer. But
> looking around, most callers of reserve_e820_ram() ignore its return
> value. One exception is reserving memory for kexec. I guess in that case
> it may be intentional to fail if the area is reserved already, as it may
> indicate it cannot be used for kexec. Is that correct?

I suppose so, yes.

> There are also a couple of calls to e820_change_range_type() in tboot
> code where it changes E820_RESERVED to E820_UNUSABLE. There, I guess
> changing e820_change_range_type() behavior would be okay.

Possibly, but please don't put much trust in the tboot code we have.

Jan