[PATCH 0/4] iommu/x86: fixes/improvements for unity range checks

[PATCH 0/4] iommu/x86: fixes/improvements for unity range checks

[Roger Pau Monne]

Hello,

Patch 1 is a bugfix for AMD-Vi IVMD range type checks.

Further patches unify the IVMD/RMRR checks into a common function, and
the last patch tightens up the condition to panic when attempting to
boot on a system that has RMRR or IVMD regions over memory that Xen uses
as RAM.

Thanks, Roger.

Roger Pau Monne (4):
  amd-vi: fix IVMD memory type checks
  iommu/x86: introduce a generic IVMD/RMRR range validity helper
  iommu/vt-d: switch to common RMRR checker
  iommu/x86: make unity range checking more strict

 xen/arch/x86/include/asm/iommu.h         |  3 ++
 xen/drivers/passthrough/amd/iommu_acpi.c | 33 ++---------------
 xen/drivers/passthrough/vtd/dmar.c       | 14 ++------
 xen/drivers/passthrough/x86/iommu.c      | 46 ++++++++++++++++++++++++
 4 files changed, 55 insertions(+), 41 deletions(-)

-- 
2.43.0

[PATCH 1/4] amd-vi: fix IVMD memory type checks

[Roger Pau Monne]

The current code that parses the IVMD blocks is relaxed with regard to the
restriction that such unity regions should always fall into memory ranges
marked as reserved in the memory map.

However the type checks for the IVMD addresses are inverted, and as a result
IVMD ranges falling into RAM areas are accepted.  Note that having such ranges
in the first place is a firmware bug, as IVMD should always fall into reserved
ranges.

Fixes: ed6c77ebf0c1 ('AMD/IOMMU: check / convert IVMD ranges for being / to be reserved')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: oxjo@proton.me
---
 xen/drivers/passthrough/amd/iommu_acpi.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c b/xen/drivers/passthrough/amd/iommu_acpi.c
index 2e3b83014beb..ca70f4f3ae2c 100644
--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -426,9 +426,14 @@ static int __init parse_ivmd_block(const struct acpi_ivrs_memory *ivmd_block)
                 return -EIO;
             }
 
-            /* Types which won't be handed out are considered good enough. */
-            if ( !(type & (RAM_TYPE_RESERVED | RAM_TYPE_ACPI |
-                           RAM_TYPE_UNUSABLE)) )
+            /*
+             * Types which aren't RAM are considered good enough.
+             * Note that a page being partially RESERVED, ACPI or UNUSABLE will
+             * force Xen into assuming the whole page as having that type in
+             * practice.
+             */
+            if ( type & (RAM_TYPE_RESERVED | RAM_TYPE_ACPI |
+                         RAM_TYPE_UNUSABLE) )
                 continue;
 
             AMD_IOMMU_ERROR("IVMD: page at %lx can't be converted\n", addr);
-- 
2.43.0

Re: [PATCH 1/4] amd-vi: fix IVMD memory type checks

[oxjo]

On Thursday, February 1st, 2024 at 18:01, Roger Pau Monne <roger.pau@citrix.com> wrote:

> The current code that parses the IVMD blocks is relaxed with regard to the
> restriction that such unity regions should always fall into memory ranges
> marked as reserved in the memory map.
>
> However the type checks for the IVMD addresses are inverted, and as a result
> IVMD ranges falling into RAM areas are accepted. Note that having such ranges
> in the first place is a firmware bug, as IVMD should always fall into reserved
> ranges.
>
> Fixes: ed6c77ebf0c1 ('AMD/IOMMU: check / convert IVMD ranges for being / to be reserved')
> Signed-off-by: Roger Pau Monné roger.pau@citrix.com
>
> ---
> Cc: oxjo@proton.me
> ---
> xen/drivers/passthrough/amd/iommu_acpi.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c b/xen/drivers/passthrough/amd/iommu_acpi.c
> index 2e3b83014beb..ca70f4f3ae2c 100644
> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
> @@ -426,9 +426,14 @@ static int __init parse_ivmd_block(const struct acpi_ivrs_memory ivmd_block)
> return -EIO;
> }
>
> - / Types which won't be handed out are considered good enough. /
> - if ( !(type & (RAM_TYPE_RESERVED | RAM_TYPE_ACPI |
> - RAM_TYPE_UNUSABLE)) )
> + /
> + * Types which aren't RAM are considered good enough.
> + * Note that a page being partially RESERVED, ACPI or UNUSABLE will
> + * force Xen into assuming the whole page as having that type in
> + * practice.
> + */
> + if ( type & (RAM_TYPE_RESERVED | RAM_TYPE_ACPI |
> + RAM_TYPE_UNUSABLE) )
> continue;
>
> AMD_IOMMU_ERROR("IVMD: page at %lx can't be converted\n", addr);

I tested the patch and it resolves the issue.
It eliminates the boot IVMD error message.
AMD-Vi is enabled and pci passthrough works.


Tested-by: oxjo <oxjo@proton.me>

Re: [PATCH 1/4] amd-vi: fix IVMD memory type checks

[Jan Beulich]

On 01.02.2024 18:01, Roger Pau Monne wrote:
> The current code that parses the IVMD blocks is relaxed with regard to the
> restriction that such unity regions should always fall into memory ranges
> marked as reserved in the memory map.
> 
> However the type checks for the IVMD addresses are inverted, and as a result
> IVMD ranges falling into RAM areas are accepted.  Note that having such ranges
> in the first place is a firmware bug, as IVMD should always fall into reserved
> ranges.
> 
> Fixes: ed6c77ebf0c1 ('AMD/IOMMU: check / convert IVMD ranges for being / to be reserved')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Regression with xhci console (was: [PATCH 1/4] amd-vi: fix IVMD memory type checks)

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1661 bytes --]

On Thu, Feb 01, 2024 at 06:01:56PM +0100, Roger Pau Monne wrote:
> The current code that parses the IVMD blocks is relaxed with regard to the
> restriction that such unity regions should always fall into memory ranges
> marked as reserved in the memory map.
> 
> However the type checks for the IVMD addresses are inverted, and as a result
> IVMD ranges falling into RAM areas are accepted.  Note that having such ranges
> in the first place is a firmware bug, as IVMD should always fall into reserved
> ranges.
> 
> Fixes: ed6c77ebf0c1 ('AMD/IOMMU: check / convert IVMD ranges for being / to be reserved')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

FYI Xen 4.17.3 with this patch (but not others in the series) causes
panic on boot on Framework 14 AMD laptop:

    (XEN)  [0000000044570000, 000000005077efff] (usable)
    ...
    (XEN) AMD-Vi: Warning: IVMD: [4f83f000,4f880000) is not (entirely) in reserved memory
    (XEN) AMD-Vi: Error: IVMD: page at 4f83f000 can't be converted
    ...
    (XEN) Xen BUG at drivers/passthrough/amd/iommu_init.c:1386

Full boot log at https://github.com/QubesOS/qubes-issues/issues/9030
4.17.2 worked fine.
I'll try the whole series (and the follow up one), but I don't expect
any difference.

This looks to be related to XHCI console, which does use the same API to
allow device to DMA into relevant buffers even when the rest of XHCI is
used by dom0 (or even other domain if 'share=yes' is used):
https://gitlab.com/xen-project/xen/-/blob/staging/xen/drivers/char/xhci-dbc.c?ref_type=heads#L1421-1424

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Regression with xhci console

[Jan Beulich]

On 11.03.2024 03:18, Marek Marczykowski-Górecki wrote:
> On Thu, Feb 01, 2024 at 06:01:56PM +0100, Roger Pau Monne wrote:
>> The current code that parses the IVMD blocks is relaxed with regard to the
>> restriction that such unity regions should always fall into memory ranges
>> marked as reserved in the memory map.
>>
>> However the type checks for the IVMD addresses are inverted, and as a result
>> IVMD ranges falling into RAM areas are accepted.  Note that having such ranges
>> in the first place is a firmware bug, as IVMD should always fall into reserved
>> ranges.
>>
>> Fixes: ed6c77ebf0c1 ('AMD/IOMMU: check / convert IVMD ranges for being / to be reserved')
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> FYI Xen 4.17.3 with this patch (but not others in the series) causes
> panic on boot on Framework 14 AMD laptop:
> 
>     (XEN)  [0000000044570000, 000000005077efff] (usable)
>     ...
>     (XEN) AMD-Vi: Warning: IVMD: [4f83f000,4f880000) is not (entirely) in reserved memory
>     (XEN) AMD-Vi: Error: IVMD: page at 4f83f000 can't be converted
>     ...
>     (XEN) Xen BUG at drivers/passthrough/amd/iommu_init.c:1386
> 
> Full boot log at https://github.com/QubesOS/qubes-issues/issues/9030
> 4.17.2 worked fine.
> I'll try the whole series (and the follow up one), but I don't expect
> any difference.
> 
> This looks to be related to XHCI console, which does use the same API to
> allow device to DMA into relevant buffers even when the rest of XHCI is
> used by dom0 (or even other domain if 'share=yes' is used):
> https://gitlab.com/xen-project/xen/-/blob/staging/xen/drivers/char/xhci-dbc.c?ref_type=heads#L1421-1424

Hmm, yes, this is what we get for allowing such (ab)use. I guess we need
to have iommu_add_extra_reserved_device_memory() actually convert the
region in question, to satisfy the later check. Care to make a patch?

Jan

[PATCH 2/4] iommu/x86: introduce a generic IVMD/RMRR range validity helper

[Roger Pau Monne]

IVMD and RMRR ranges are functionally equivalent, and as so could use the same
validity checker.

Move the IVMD to x86 common IOMMU code and adjust the function to take a pair
of [start, end) mfn parameters.

So far only the AMD-Vi side is adjusted to use the newly introduced helper, the
VT-d side will be adjusted in a further change.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/include/asm/iommu.h         |  3 ++
 xen/drivers/passthrough/amd/iommu_acpi.c | 38 ++------------------
 xen/drivers/passthrough/x86/iommu.c      | 46 ++++++++++++++++++++++++
 3 files changed, 52 insertions(+), 35 deletions(-)

diff --git a/xen/arch/x86/include/asm/iommu.h b/xen/arch/x86/include/asm/iommu.h
index 15a848ddc329..5c7e37331aad 100644
--- a/xen/arch/x86/include/asm/iommu.h
+++ b/xen/arch/x86/include/asm/iommu.h
@@ -135,6 +135,9 @@ struct page_info *__must_check iommu_alloc_pgtable(struct domain_iommu *hd,
                                                    uint64_t contig_mask);
 void iommu_queue_free_pgtable(struct domain_iommu *hd, struct page_info *pg);
 
+/* Check [start, end) unity map range for correctness. */
+bool iommu_unity_region_ok(mfn_t start, mfn_t end);
+
 #endif /* !__ARCH_X86_IOMMU_H__ */
 /*
  * Local variables:
diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c b/xen/drivers/passthrough/amd/iommu_acpi.c
index ca70f4f3ae2c..40468dbbccf3 100644
--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -405,41 +405,9 @@ static int __init parse_ivmd_block(const struct acpi_ivrs_memory *ivmd_block)
         return 0;
     }
 
-    if ( !e820_all_mapped(base, limit + PAGE_SIZE, E820_RESERVED) )
-    {
-        paddr_t addr;
-
-        AMD_IOMMU_WARN("IVMD: [%lx,%lx) is not (entirely) in reserved memory\n",
-                       base, limit + PAGE_SIZE);
-
-        for ( addr = base; addr <= limit; addr += PAGE_SIZE )
-        {
-            unsigned int type = page_get_ram_type(maddr_to_mfn(addr));
-
-            if ( type == RAM_TYPE_UNKNOWN )
-            {
-                if ( e820_add_range(addr, addr + PAGE_SIZE,
-                                    E820_RESERVED) )
-                    continue;
-                AMD_IOMMU_ERROR("IVMD: page at %lx couldn't be reserved\n",
-                                addr);
-                return -EIO;
-            }
-
-            /*
-             * Types which aren't RAM are considered good enough.
-             * Note that a page being partially RESERVED, ACPI or UNUSABLE will
-             * force Xen into assuming the whole page as having that type in
-             * practice.
-             */
-            if ( type & (RAM_TYPE_RESERVED | RAM_TYPE_ACPI |
-                         RAM_TYPE_UNUSABLE) )
-                continue;
-
-            AMD_IOMMU_ERROR("IVMD: page at %lx can't be converted\n", addr);
-            return -EIO;
-        }
-    }
+    if ( !iommu_unity_region_ok(maddr_to_mfn(base),
+                                maddr_to_mfn(limit + PAGE_SIZE)) )
+        return -EIO;
 
     if ( ivmd_block->header.flags & ACPI_IVMD_EXCLUSION_RANGE )
         exclusion = true;
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index c90755ff58fa..63d4cb898218 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -792,6 +792,52 @@ static int __init cf_check adjust_irq_affinities(void)
 }
 __initcall(adjust_irq_affinities);
 
+bool __init iommu_unity_region_ok(mfn_t start, mfn_t end)
+{
+    mfn_t addr;
+
+    if ( e820_all_mapped(mfn_to_maddr(start), mfn_to_maddr(end),
+                         E820_RESERVED) )
+        return true;
+
+    printk(XENLOG_WARNING "IOMMU: [%#" PRI_mfn " ,%#" PRI_mfn
+           ") is not (entirely) in reserved memory\n",
+           mfn_x(start), mfn_x(end));
+
+    for ( addr = start; !mfn_eq(addr, end); mfn_add(addr, 1) )
+    {
+        unsigned int type = page_get_ram_type(addr);
+
+        if ( type == RAM_TYPE_UNKNOWN )
+        {
+            if ( e820_add_range(mfn_to_maddr(addr),
+                                mfn_to_maddr(addr) + PAGE_SIZE, E820_RESERVED) )
+                continue;
+            printk(XENLOG_WARNING
+                   "IOMMU: page at %#" PRI_mfn " couldn't be reserved\n",
+                   mfn_x(addr));
+            return false;
+        }
+
+        /*
+         * Types which aren't RAM are considered good enough.
+         * Note that a page being partially RESERVED, ACPI or UNUSABLE will
+         * force Xen into assuming the whole page as having that type in
+         * practice.
+         */
+        if ( type & (RAM_TYPE_RESERVED | RAM_TYPE_ACPI |
+                     RAM_TYPE_UNUSABLE) )
+            continue;
+
+        printk(XENLOG_WARNING
+               "IOMMU: page at %#" PRI_mfn " can't be converted\n",
+               mfn_x(addr));
+        return false;
+    }
+
+    return true;
+}
+
 /*
  * Local variables:
  * mode: C
-- 
2.43.0

Re: [PATCH 2/4] iommu/x86: introduce a generic IVMD/RMRR range validity helper

[Jan Beulich]

On 01.02.2024 18:01, Roger Pau Monne wrote:
> IVMD and RMRR ranges are functionally equivalent, and as so could use the same
> validity checker.

May I suggest s/equivalent/similar/?

> Move the IVMD to x86 common IOMMU code and adjust the function to take a pair
> of [start, end) mfn parameters.

[start,end) ranges generally come with the problem of not allowing to
represent the full address space. While that isn't specifically a problem
here, seeing that both VT-d and V-i present inclusive ranges, how about
making the common function match that?

Jan

Re: [PATCH 2/4] iommu/x86: introduce a generic IVMD/RMRR range validity helper

[Roger Pau Monné]

On Tue, Feb 06, 2024 at 12:17:29PM +0100, Jan Beulich wrote:
> On 01.02.2024 18:01, Roger Pau Monne wrote:
> > IVMD and RMRR ranges are functionally equivalent, and as so could use the same
> > validity checker.
> 
> May I suggest s/equivalent/similar/?

Sure.

> > Move the IVMD to x86 common IOMMU code and adjust the function to take a pair
> > of [start, end) mfn parameters.
> 
> [start,end) ranges generally come with the problem of not allowing to
> represent the full address space. While that isn't specifically a problem
> here, seeing that both VT-d and V-i present inclusive ranges, how about
> making the common function match that?

OK, I can adjust the code that way.  I originally did it using non
inclusive because it looked like the code would be clearer.

Thanks, Roger.

[PATCH 3/4] iommu/vt-d: switch to common RMRR checker

[Roger Pau Monne]

Use the newly introduced generic unity map checker.

Also drop the message recommending the usage of iommu_inclusive_mapping: the
ranges would end up being mapped anyway even if some of the checks above
failed, regardless of whether iommu_inclusive_mapping is set.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/drivers/passthrough/vtd/dmar.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
index 07772f178fe6..005b42706a34 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -642,17 +642,9 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
            return -EEXIST;
        }
 
-    /* This check is here simply to detect when RMRR values are
-     * not properly represented in the system memory map and
-     * inform the user
-     */
-    if ( !e820_all_mapped(base_addr, end_addr + 1, E820_RESERVED) &&
-         !e820_all_mapped(base_addr, end_addr + 1, E820_NVS) &&
-         !e820_all_mapped(base_addr, end_addr + 1, E820_ACPI) )
-        printk(XENLOG_WARNING VTDPREFIX
-               " RMRR [%"PRIx64",%"PRIx64"] not in reserved memory;"
-               " need \"iommu_inclusive_mapping=1\"?\n",
-                base_addr, end_addr);
+    if ( !iommu_unity_region_ok(maddr_to_mfn(base_addr),
+                                maddr_to_mfn(end_addr + PAGE_SIZE)) )
+        return -EIO;
 
     rmrru = xzalloc(struct acpi_rmrr_unit);
     if ( !rmrru )
-- 
2.43.0

Re: [PATCH 3/4] iommu/vt-d: switch to common RMRR checker

[Jan Beulich]

On 01.02.2024 18:01, Roger Pau Monne wrote:
> Use the newly introduced generic unity map checker.
> 
> Also drop the message recommending the usage of iommu_inclusive_mapping: the
> ranges would end up being mapped anyway even if some of the checks above
> failed, regardless of whether iommu_inclusive_mapping is set.

I'm afraid I don't understand this: When not in an appropriate E820
region, you now even fail IOMMU initialization. Shouldn't such
failure only occur when inclusive mappings weren't requested? At
which point referring to that option is still relevant?

Further to this failing - in patch 2 shouldn't the respective log
messages then be XENLOG_ERR, matching the earlier use of
AMD_IOMMU_ERROR()?

> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -642,17 +642,9 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
>             return -EEXIST;
>         }
>  
> -    /* This check is here simply to detect when RMRR values are
> -     * not properly represented in the system memory map and
> -     * inform the user
> -     */
> -    if ( !e820_all_mapped(base_addr, end_addr + 1, E820_RESERVED) &&
> -         !e820_all_mapped(base_addr, end_addr + 1, E820_NVS) &&
> -         !e820_all_mapped(base_addr, end_addr + 1, E820_ACPI) )
> -        printk(XENLOG_WARNING VTDPREFIX
> -               " RMRR [%"PRIx64",%"PRIx64"] not in reserved memory;"
> -               " need \"iommu_inclusive_mapping=1\"?\n",
> -                base_addr, end_addr);
> +    if ( !iommu_unity_region_ok(maddr_to_mfn(base_addr),
> +                                maddr_to_mfn(end_addr + PAGE_SIZE)) )
> +        return -EIO;

Hmm, noticing only here, but applicable also to the earlier patch: The
"RMRR" (and there "IVMD") is lost, which removes some relevant context
information from the log messages. Can you add a const char* parameter
to the new helper, please?

Jan

Re: [PATCH 3/4] iommu/vt-d: switch to common RMRR checker

[Roger Pau Monné]

On Tue, Feb 06, 2024 at 12:28:07PM +0100, Jan Beulich wrote:
> On 01.02.2024 18:01, Roger Pau Monne wrote:
> > Use the newly introduced generic unity map checker.
> > 
> > Also drop the message recommending the usage of iommu_inclusive_mapping: the
> > ranges would end up being mapped anyway even if some of the checks above
> > failed, regardless of whether iommu_inclusive_mapping is set.
> 
> I'm afraid I don't understand this: When not in an appropriate E820
> region, you now even fail IOMMU initialization. Shouldn't such
> failure only occur when inclusive mappings weren't requested? At
> which point referring to that option is still relevant?

This is now better handled, since the VT-d code will use the same
logic as the AMD-Vi logic and attempt to 'convert' such bogus RMRR
regions so they can be safely used.  iommu_unity_region_ok() signals
the RMRR region is impossible to be used, and hence not even
iommu_inclusive_mapping would help in that case.  Also note that
iommu_inclusive_mapping is only applicable to PV, so the message was
already wrong in the PVH case.

> Further to this failing - in patch 2 shouldn't the respective log
> messages then be XENLOG_ERR, matching the earlier use of
> AMD_IOMMU_ERROR()?

Oh, indeed, I've converted them all to WARN, when the first one is
indeed WARN, but the following two are ERROR.

> > --- a/xen/drivers/passthrough/vtd/dmar.c
> > +++ b/xen/drivers/passthrough/vtd/dmar.c
> > @@ -642,17 +642,9 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
> >             return -EEXIST;
> >         }
> >  
> > -    /* This check is here simply to detect when RMRR values are
> > -     * not properly represented in the system memory map and
> > -     * inform the user
> > -     */
> > -    if ( !e820_all_mapped(base_addr, end_addr + 1, E820_RESERVED) &&
> > -         !e820_all_mapped(base_addr, end_addr + 1, E820_NVS) &&
> > -         !e820_all_mapped(base_addr, end_addr + 1, E820_ACPI) )
> > -        printk(XENLOG_WARNING VTDPREFIX
> > -               " RMRR [%"PRIx64",%"PRIx64"] not in reserved memory;"
> > -               " need \"iommu_inclusive_mapping=1\"?\n",
> > -                base_addr, end_addr);
> > +    if ( !iommu_unity_region_ok(maddr_to_mfn(base_addr),
> > +                                maddr_to_mfn(end_addr + PAGE_SIZE)) )
> > +        return -EIO;
> 
> Hmm, noticing only here, but applicable also to the earlier patch: The
> "RMRR" (and there "IVMD") is lost, which removes some relevant context
> information from the log messages. Can you add a const char* parameter
> to the new helper, please?

I debated myself whether to keep the RMRR/IVMD prefix, but I didn't
think there was a lot of value in it, since whether it's an RMRR or an
IVMD region can be deduced from previous messages.  Anyway will pass
the prefix as a function parameter.

Thanks, Roger.

Re: [PATCH 3/4] iommu/vt-d: switch to common RMRR checker

[Jan Beulich]

On 07.02.2024 10:01, Roger Pau Monné wrote:
> On Tue, Feb 06, 2024 at 12:28:07PM +0100, Jan Beulich wrote:
>> On 01.02.2024 18:01, Roger Pau Monne wrote:
>>> Use the newly introduced generic unity map checker.
>>>
>>> Also drop the message recommending the usage of iommu_inclusive_mapping: the
>>> ranges would end up being mapped anyway even if some of the checks above
>>> failed, regardless of whether iommu_inclusive_mapping is set.
>>
>> I'm afraid I don't understand this: When not in an appropriate E820
>> region, you now even fail IOMMU initialization. Shouldn't such
>> failure only occur when inclusive mappings weren't requested? At
>> which point referring to that option is still relevant?
> 
> This is now better handled, since the VT-d code will use the same
> logic as the AMD-Vi logic and attempt to 'convert' such bogus RMRR
> regions so they can be safely used.  iommu_unity_region_ok() signals
> the RMRR region is impossible to be used, and hence not even
> iommu_inclusive_mapping would help in that case.

Impossible only in so far as we don't know whether a such named region
would actually still be accessed post-boot. But yes, if it wouldn't be
accessed, there would also be no need for passing the extra option.

>  Also note that
> iommu_inclusive_mapping is only applicable to PV, so the message was
> already wrong in the PVH case.

This is a fair point, which probably wants mentioning as (partial)
justification. Plus iirc the intention was to get rid of that option
anyway, at some point.

Jan

[PATCH 4/4] iommu/x86: make unity range checking more strict

[Roger Pau Monne]

Currently when a unity range overlaps with memory being used as RAM by the
hypervisor the result would be that the IOMMU gets disabled.  However that's
not enough, as even with the IOMMU disabled the device will still access the
affected RAM areas.

Note that IVMD or RMRR ranges being placed over RAM is a firmware bug.

Doing so also allows to simplify the code and use a switch over the reported
memory type(s).

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/drivers/passthrough/x86/iommu.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index 63d4cb898218..9b977f84582f 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -806,10 +806,14 @@ bool __init iommu_unity_region_ok(mfn_t start, mfn_t end)
 
     for ( addr = start; !mfn_eq(addr, end); mfn_add(addr, 1) )
     {
-        unsigned int type = page_get_ram_type(addr);
-
-        if ( type == RAM_TYPE_UNKNOWN )
+        /*
+         * Any page that's at least partially of type RESERVED, UNUSABLE or
+         * ACPI will be considered by Xen of being all of that type, and hence
+         * the problematic pages are those that are fully holes or RAM.
+         */
+        switch ( page_get_ram_type(addr) )
         {
+        case RAM_TYPE_UNKNOWN:
             if ( e820_add_range(mfn_to_maddr(addr),
                                 mfn_to_maddr(addr) + PAGE_SIZE, E820_RESERVED) )
                 continue;
@@ -817,7 +821,10 @@ bool __init iommu_unity_region_ok(mfn_t start, mfn_t end)
                    "IOMMU: page at %#" PRI_mfn " couldn't be reserved\n",
                    mfn_x(addr));
             return false;
-        }
+
+        case RAM_TYPE_CONVENTIONAL:
+            panic("IOMMU: page at %#" PRI_mfn " overlaps RAM range\n",
+                  mfn_x(addr));
 
         /*
          * Types which aren't RAM are considered good enough.
@@ -825,14 +832,7 @@ bool __init iommu_unity_region_ok(mfn_t start, mfn_t end)
          * force Xen into assuming the whole page as having that type in
          * practice.
          */
-        if ( type & (RAM_TYPE_RESERVED | RAM_TYPE_ACPI |
-                     RAM_TYPE_UNUSABLE) )
-            continue;
-
-        printk(XENLOG_WARNING
-               "IOMMU: page at %#" PRI_mfn " can't be converted\n",
-               mfn_x(addr));
-        return false;
+        }
     }
 
     return true;
-- 
2.43.0

Re: [PATCH 4/4] iommu/x86: make unity range checking more strict

[Jan Beulich]

On 01.02.2024 18:01, Roger Pau Monne wrote:
> Currently when a unity range overlaps with memory being used as RAM by the
> hypervisor the result would be that the IOMMU gets disabled.  However that's
> not enough, as even with the IOMMU disabled the device will still access the
> affected RAM areas.

Hmm, no, I think this is going too far. Not the least because it is
s/will/may/. But also because if we really wanted such behavior, we
ought to also parse the respective ACPI tables when the "iommu=off".

> Note that IVMD or RMRR ranges being placed over RAM is a firmware bug.

As written this is wrong: They're typically in RAM, just that the E820
type for that range should not be RAM_TYPE_CONVENTIONAL.

> Doing so also allows to simplify the code and use a switch over the reported
> memory type(s).

I'm afraid this isn't right either: page_get_ram_type() can set
multiple bits in its output.

Jan

Re: [PATCH 4/4] iommu/x86: make unity range checking more strict

[Roger Pau Monné]

On Tue, Feb 06, 2024 at 12:49:08PM +0100, Jan Beulich wrote:
> On 01.02.2024 18:01, Roger Pau Monne wrote:
> > Currently when a unity range overlaps with memory being used as RAM by the
> > hypervisor the result would be that the IOMMU gets disabled.  However that's
> > not enough, as even with the IOMMU disabled the device will still access the
> > affected RAM areas.
> 
> Hmm, no, I think this is going too far. Not the least because it is
> s/will/may/. But also because if we really wanted such behavior, we
> ought to also parse the respective ACPI tables when the "iommu=off".

I guessed so, hence why it's the last patch in the series.  TBH I
think it's very unlikely that such system exist.

> > Note that IVMD or RMRR ranges being placed over RAM is a firmware bug.
> 
> As written this is wrong: They're typically in RAM, just that the E820
> type for that range should not be RAM_TYPE_CONVENTIONAL.

Hm, yes, ÇI should have written 'over a RAM range in the memory map'
or similar.

> > Doing so also allows to simplify the code and use a switch over the reported
> > memory type(s).
> 
> I'm afraid this isn't right either: page_get_ram_type() can set
> multiple bits in its output.

It can indeed.  But if the only bit set is RAM_TYPE_CONVENTIONAL then
the page will be handled as RAM, and that's where Xen would be in
trouble if a device is also using such page as a unity map.

If the page however is RAM_TYPE_CONVENTIONAL | RAM_TYPE_RESERVED then
the RESERVED type will take over the whole page, and it's no longer an
issue to have a unity range covering it.

Thanks, Roger.

Re: [PATCH 4/4] iommu/x86: make unity range checking more strict

[Jan Beulich]

On 07.02.2024 10:14, Roger Pau Monné wrote:
> On Tue, Feb 06, 2024 at 12:49:08PM +0100, Jan Beulich wrote:
>> On 01.02.2024 18:01, Roger Pau Monne wrote:
>>> Currently when a unity range overlaps with memory being used as RAM by the
>>> hypervisor the result would be that the IOMMU gets disabled.  However that's
>>> not enough, as even with the IOMMU disabled the device will still access the
>>> affected RAM areas.
>>
>> Hmm, no, I think this is going too far. Not the least because it is
>> s/will/may/. But also because if we really wanted such behavior, we
>> ought to also parse the respective ACPI tables when the "iommu=off".
> 
> I guessed so, hence why it's the last patch in the series.  TBH I
> think it's very unlikely that such system exist.

And you think so despite knowing that on some systems one needs to
manually specify RMRR regions?

>>> Doing so also allows to simplify the code and use a switch over the reported
>>> memory type(s).
>>
>> I'm afraid this isn't right either: page_get_ram_type() can set
>> multiple bits in its output.
> 
> It can indeed.  But if the only bit set is RAM_TYPE_CONVENTIONAL then
> the page will be handled as RAM, and that's where Xen would be in
> trouble if a device is also using such page as a unity map.
> 
> If the page however is RAM_TYPE_CONVENTIONAL | RAM_TYPE_RESERVED then
> the RESERVED type will take over the whole page, and it's no longer an
> issue to have a unity range covering it.

Okay, if this is intentional, than saying so in a code comment would
imo help quite a bit.

Jan