[PATCH v2 0/2] iommu/x86: unify RMRR/IVMD range checks

[PATCH v2 0/2] iommu/x86: unify RMRR/IVMD range checks

[Roger Pau Monne]

Hello,

The following patches unify the check for the RMRR/IVMD ranges in a
common function, using the IVMD as the basis.

Thanks, Roger.

Roger Pau Monne (2):
  iommu/x86: introduce a generic IVMD/RMRR range validity helper
  iommu/vt-d: switch to common RMRR checker

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

-- 
2.43.0

[PATCH v2 1/2] 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>
---
Changes since v1:
 - Fix usage of ERR/WARN in messages.
 - Pass region as an inclusive range.
 - Pass log message prefix.
---
 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..2b97cb442edd 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(const char *prefix, 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..3f5508eba049 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("IVMD", maddr_to_mfn(base),
+                                maddr_to_mfn(limit)) )
+        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..d867490e25dc 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(const char *prefix, mfn_t start, mfn_t end)
+{
+    mfn_t addr;
+
+    if ( e820_all_mapped(mfn_to_maddr(start), mfn_to_maddr(end) + PAGE_SIZE,
+                         E820_RESERVED) )
+        return true;
+
+    printk(XENLOG_WARNING "%s: [%#" PRI_mfn " ,%#" PRI_mfn
+           "] is not (entirely) in reserved memory\n",
+           prefix, mfn_x(start), mfn_x(end));
+
+    for ( addr = start; mfn_x(addr) <= mfn_x(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_ERR
+                   "%s: page at %#" PRI_mfn " couldn't be reserved\n",
+                   prefix, 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_ERR
+               "%s: page at %#" PRI_mfn " can't be converted\n",
+               prefix, mfn_x(addr));
+        return false;
+    }
+
+    return true;
+}
+
 /*
  * Local variables:
  * mode: C
-- 
2.43.0

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

[Jan Beulich]

On 07.02.2024 16:34, Roger Pau Monne wrote:
> 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>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with one minor remark:

> --- 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(const char *prefix, mfn_t start, mfn_t end)
> +{
> +    mfn_t addr;
> +
> +    if ( e820_all_mapped(mfn_to_maddr(start), mfn_to_maddr(end) + PAGE_SIZE,
> +                         E820_RESERVED) )
> +        return true;
> +
> +    printk(XENLOG_WARNING "%s: [%#" PRI_mfn " ,%#" PRI_mfn
> +           "] is not (entirely) in reserved memory\n",

Would you mind if I re-flowed this to

    printk(XENLOG_WARNING
           "%s: [%#" PRI_mfn " ,%#" PRI_mfn "] is not (entirely) in reserved memory\n",

while committing?

Jan

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

[Roger Pau Monné]

On Mon, Feb 12, 2024 at 03:34:59PM +0100, Jan Beulich wrote:
> On 07.02.2024 16:34, Roger Pau Monne wrote:
> > 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>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with one minor remark:
> 
> > --- 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(const char *prefix, mfn_t start, mfn_t end)
> > +{
> > +    mfn_t addr;
> > +
> > +    if ( e820_all_mapped(mfn_to_maddr(start), mfn_to_maddr(end) + PAGE_SIZE,
> > +                         E820_RESERVED) )
> > +        return true;
> > +
> > +    printk(XENLOG_WARNING "%s: [%#" PRI_mfn " ,%#" PRI_mfn
> > +           "] is not (entirely) in reserved memory\n",
> 
> Would you mind if I re-flowed this to
> 
>     printk(XENLOG_WARNING
>            "%s: [%#" PRI_mfn " ,%#" PRI_mfn "] is not (entirely) in reserved memory\n",
> 
> while committing?

Sure.  FWIW, when the line contains format specifiers I take that as a
point where splitting might be acceptable, since it already prevents
proper grepping.  Will try to remember to attempt to not split the
line.

Thanks, Roger.

[PATCH v2 2/2] 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.  Plus such option
is not supported for PVH, and it's deprecated.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Adjust to changes in the previous patch.
 - Expand commit message.
---
 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..76aade816c08 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("RMRR", maddr_to_mfn(base_addr),
+                                maddr_to_mfn(end_addr)) )
+        return -EIO;
 
     rmrru = xzalloc(struct acpi_rmrr_unit);
     if ( !rmrru )
-- 
2.43.0

Re: [PATCH v2 2/2] iommu/vt-d: switch to common RMRR checker

[Jan Beulich]

On 07.02.2024 16:34, 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.  Plus such option
> is not supported for PVH, and it's deprecated.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

[regression] Re: [PATCH v2 2/2] iommu/vt-d: switch to common RMRR checker

[Andrew Cooper]

On 12/02/2024 2:38 pm, Jan Beulich wrote:
> On 07.02.2024 16:34, 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.  Plus such option
>> is not supported for PVH, and it's deprecated.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

XenRT says no.

It's not clear exactly what's going on here, but the latest resync with
staging (covering only today's pushed changes) suffered 4 failures to
boot, on a mix of Intel hardware (SNB, SKL, SKX and CLX).

All 4 triple-fault-like things where following a log message about an RMRR:

(XEN) RMRR: [0x0e8 ,0x0e8] is not (entirely) in reserved memory

not being in reserved memory.


First of all - fix this printk() to print full addresses, not frame
numbers.  It's obnoxious to cross reference with the E820.

In the example above, 0xe8000 is regular RAM in:

(XEN)  [0000000000000000, 000000000009d3ff] (usable)


In another example,

(XEN) RMRR: [0x4d800 ,0x4ffff] is not (entirely) in reserved memory

is a hole between:

(XEN)  [000000004d3ff000, 000000004d3fffff] (usable)
(XEN)  [00000000e0000000, 00000000efffffff] (reserved)

We should also explicitly render holes when printing the E820, because
that's also unnecessarily hard to spot.


It's very likely something in this series, but the link to Intel might
just be chance of which hardware got selected, and I've got no clue why
there's a reset with no further logging out of Xen...

~Andrew

Re: [regression] Re: [PATCH v2 2/2] iommu/vt-d: switch to common RMRR checker

[Jan Beulich]

On 13.02.2024 23:37, Andrew Cooper wrote:
> On 12/02/2024 2:38 pm, Jan Beulich wrote:
>> On 07.02.2024 16:34, 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.  Plus such option
>>> is not supported for PVH, and it's deprecated.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> XenRT says no.
> 
> It's not clear exactly what's going on here, but the latest resync with
> staging (covering only today's pushed changes) suffered 4 failures to
> boot, on a mix of Intel hardware (SNB, SKL, SKX and CLX).
> 
> All 4 triple-fault-like things where following a log message about an RMRR:
> 
> (XEN) RMRR: [0x0e8 ,0x0e8] is not (entirely) in reserved memory
> 
> not being in reserved memory.
> 
> 
> First of all - fix this printk() to print full addresses, not frame
> numbers.  It's obnoxious to cross reference with the E820.

Perhaps better indeed. The stray blank before the comma also wants dropping.
And while looking over the patch again, "mfn_t addr;" also isn't very
helpful - the variable would better be named mfn.

> In the example above, 0xe8000 is regular RAM in:
> 
> (XEN)  [0000000000000000, 000000000009d3ff] (usable)

Well, no, E8000 is outside of that range, and I'm inclined to guess it's
the SNB where you saw that. Iirc my SNB has such an RMRR range, too. (Or
was it the Westmere?)

> In another example,
> 
> (XEN) RMRR: [0x4d800 ,0x4ffff] is not (entirely) in reserved memory
> 
> is a hole between:
> 
> (XEN)  [000000004d3ff000, 000000004d3fffff] (usable)
> (XEN)  [00000000e0000000, 00000000efffffff] (reserved)
> 
> We should also explicitly render holes when printing the E820, because
> that's also unnecessarily hard to spot.

I disagree here - both "ends" of a hole are easily visible from the
neighboring ranges.

> It's very likely something in this series, but the link to Intel might
> just be chance of which hardware got selected, and I've got no clue why
> there's a reset with no further logging out of Xen...

I second this - even after looking closely at the patches again, I can't
make a connection between them and the observed behavior. Didn't yet look
at what, if anything, osstest may have to say. Do I understand correctly
that the cited log messages are the last sign of life prior to the
systems rebooting?

Jan

Re: [regression] Re: [PATCH v2 2/2] iommu/vt-d: switch to common RMRR checker

[Roger Pau Monné]

On Wed, Feb 14, 2024 at 08:45:28AM +0100, Jan Beulich wrote:
> On 13.02.2024 23:37, Andrew Cooper wrote:
> > On 12/02/2024 2:38 pm, Jan Beulich wrote:
> >> On 07.02.2024 16:34, 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.  Plus such option
> >>> is not supported for PVH, and it's deprecated.
> >>>
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> > 
> > XenRT says no.
> > 
> > It's not clear exactly what's going on here, but the latest resync with
> > staging (covering only today's pushed changes) suffered 4 failures to
> > boot, on a mix of Intel hardware (SNB, SKL, SKX and CLX).
> > 
> > All 4 triple-fault-like things where following a log message about an RMRR:
> > 
> > (XEN) RMRR: [0x0e8 ,0x0e8] is not (entirely) in reserved memory
> > 
> > not being in reserved memory.
> > 
> > 
> > First of all - fix this printk() to print full addresses, not frame
> > numbers.  It's obnoxious to cross reference with the E820.
> 
> Perhaps better indeed. The stray blank before the comma also wants dropping.
> And while looking over the patch again, "mfn_t addr;" also isn't very
> helpful - the variable would better be named mfn.

I can adjust those in the fix, see below.

> > It's very likely something in this series, but the link to Intel might
> > just be chance of which hardware got selected, and I've got no clue why
> > there's a reset with no further logging out of Xen...
> 
> I second this - even after looking closely at the patches again, I can't
> make a connection between them and the observed behavior. Didn't yet look
> at what, if anything, osstest may have to say. Do I understand correctly
> that the cited log messages are the last sign of life prior to the
> systems rebooting?

I've found it:

    for ( addr = start; mfn_x(addr) <= mfn_x(end); mfn_add(addr, 1) )

Should be:

    for ( addr = start; mfn_x(addr) <= mfn_x(end); addr = mfn_add(addr, 1) )

mfn_add() doesn't modify the parameter.  Will see about making those
helpers __must_check in order to avoid this happening in the future.

Re: [regression] Re: [PATCH v2 2/2] iommu/vt-d: switch to common RMRR checker

[Andrew Cooper]

On 14/02/2024 8:45 am, Roger Pau Monné wrote:
> On Wed, Feb 14, 2024 at 08:45:28AM +0100, Jan Beulich wrote:
>> On 13.02.2024 23:37, Andrew Cooper wrote:
>>> On 12/02/2024 2:38 pm, Jan Beulich wrote:
>>>> On 07.02.2024 16:34, 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.  Plus such option
>>>>> is not supported for PVH, and it's deprecated.
>>>>>
>>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>> XenRT says no.
>>>
>>> It's not clear exactly what's going on here, but the latest resync with
>>> staging (covering only today's pushed changes) suffered 4 failures to
>>> boot, on a mix of Intel hardware (SNB, SKL, SKX and CLX).
>>>
>>> All 4 triple-fault-like things where following a log message about an RMRR:
>>>
>>> (XEN) RMRR: [0x0e8 ,0x0e8] is not (entirely) in reserved memory
>>>
>>> not being in reserved memory.
>>>
>>>
>>> First of all - fix this printk() to print full addresses, not frame
>>> numbers.  It's obnoxious to cross reference with the E820.
>> Perhaps better indeed. The stray blank before the comma also wants dropping.
>> And while looking over the patch again, "mfn_t addr;" also isn't very
>> helpful - the variable would better be named mfn.
> I can adjust those in the fix, see below.
>
>>> It's very likely something in this series, but the link to Intel might
>>> just be chance of which hardware got selected, and I've got no clue why
>>> there's a reset with no further logging out of Xen...
>> I second this - even after looking closely at the patches again, I can't
>> make a connection between them and the observed behavior. Didn't yet look
>> at what, if anything, osstest may have to say. Do I understand correctly
>> that the cited log messages are the last sign of life prior to the
>> systems rebooting?
> I've found it:
>
>     for ( addr = start; mfn_x(addr) <= mfn_x(end); mfn_add(addr, 1) )
>
> Should be:
>
>     for ( addr = start; mfn_x(addr) <= mfn_x(end); addr = mfn_add(addr, 1) )
>
> mfn_add() doesn't modify the parameter.  Will see about making those
> helpers __must_check in order to avoid this happening in the future.

There's only a single thing in this function which wants an mfn_t. 
Everything else is operating on raw paddr_t's.  I'd suggest converting
types at the start and using plain numbers.

Also, while I hate to nitpick, iommu_unity_region_ok() really ought to
be iommu_check_unity_region().  It is not a predicate (given the
additional fixups), so the function name shouldn't read as one.

Also, the "not (entirely) in reserved memory" line ought to have an ";
adjusting" on the end to make it clear that it's making an adjustment in
light of finding the range not reserved.

Finally, the "can't be converted" error should render type, even if only
in numeric form.

What do we do when there's a region that's marked as RAM?

As to the triple-fault-like nature, given that it's an infinite loop, I
expect that it was our test automation getting unhappy and power cycling
the systems after seeing no signs of starting the installer.

~Andrew

Re: [regression] Re: [PATCH v2 2/2] iommu/vt-d: switch to common RMRR checker

[Roger Pau Monné]

On Wed, Feb 14, 2024 at 09:00:25AM +0000, Andrew Cooper wrote:
> On 14/02/2024 8:45 am, Roger Pau Monné wrote:
> > On Wed, Feb 14, 2024 at 08:45:28AM +0100, Jan Beulich wrote:
> >> On 13.02.2024 23:37, Andrew Cooper wrote:
> >>> On 12/02/2024 2:38 pm, Jan Beulich wrote:
> >>>> On 07.02.2024 16:34, 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.  Plus such option
> >>>>> is not supported for PVH, and it's deprecated.
> >>>>>
> >>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> >>> XenRT says no.
> >>>
> >>> It's not clear exactly what's going on here, but the latest resync with
> >>> staging (covering only today's pushed changes) suffered 4 failures to
> >>> boot, on a mix of Intel hardware (SNB, SKL, SKX and CLX).
> >>>
> >>> All 4 triple-fault-like things where following a log message about an RMRR:
> >>>
> >>> (XEN) RMRR: [0x0e8 ,0x0e8] is not (entirely) in reserved memory
> >>>
> >>> not being in reserved memory.
> >>>
> >>>
> >>> First of all - fix this printk() to print full addresses, not frame
> >>> numbers.  It's obnoxious to cross reference with the E820.
> >> Perhaps better indeed. The stray blank before the comma also wants dropping.
> >> And while looking over the patch again, "mfn_t addr;" also isn't very
> >> helpful - the variable would better be named mfn.
> > I can adjust those in the fix, see below.
> >
> >>> It's very likely something in this series, but the link to Intel might
> >>> just be chance of which hardware got selected, and I've got no clue why
> >>> there's a reset with no further logging out of Xen...
> >> I second this - even after looking closely at the patches again, I can't
> >> make a connection between them and the observed behavior. Didn't yet look
> >> at what, if anything, osstest may have to say. Do I understand correctly
> >> that the cited log messages are the last sign of life prior to the
> >> systems rebooting?
> > I've found it:
> >
> >     for ( addr = start; mfn_x(addr) <= mfn_x(end); mfn_add(addr, 1) )
> >
> > Should be:
> >
> >     for ( addr = start; mfn_x(addr) <= mfn_x(end); addr = mfn_add(addr, 1) )
> >
> > mfn_add() doesn't modify the parameter.  Will see about making those
> > helpers __must_check in order to avoid this happening in the future.
> 
> There's only a single thing in this function which wants an mfn_t. 
> Everything else is operating on raw paddr_t's.  I'd suggest converting
> types at the start and using plain numbers.

I don't have a strong opinion, can do (but then likely as a followup
patch).

> Also, while I hate to nitpick, iommu_unity_region_ok() really ought to
> be iommu_check_unity_region().  It is not a predicate (given the
> additional fixups), so the function name shouldn't read as one.

I'm afraid those two read the same to me.  I can change, but I don't
see how the additional fixups modify how the function should be
named.

> Also, the "not (entirely) in reserved memory" line ought to have an ";
> adjusting" on the end to make it clear that it's making an adjustment in
> light of finding the range not reserved.
> 
> Finally, the "can't be converted" error should render type, even if only
> in numeric form.

This was all inherited from the previous IVMD code.

> What do we do when there's a region that's marked as RAM?

We fail to initialize the IOMMU, which is what we did previously.  In
v1 of this series there was a further patch that would panic Xen if
such overlap was found.  That however raises the question if we need
to parse IVMD/RMRR regions even when the IOMMU is disabled, so that
the panic would also be triggered even when not using the IOMMU (as
the device will still be accessing the regions in the RMRR/IVMD
ranges).

Thanks, Roger.

Re: [regression] Re: [PATCH v2 2/2] iommu/vt-d: switch to common RMRR checker

[Jan Beulich]

On 14.02.2024 09:45, Roger Pau Monné wrote:
> On Wed, Feb 14, 2024 at 08:45:28AM +0100, Jan Beulich wrote:
>> On 13.02.2024 23:37, Andrew Cooper wrote:
>>> It's very likely something in this series, but the link to Intel might
>>> just be chance of which hardware got selected, and I've got no clue why
>>> there's a reset with no further logging out of Xen...
>>
>> I second this - even after looking closely at the patches again, I can't
>> make a connection between them and the observed behavior. Didn't yet look
>> at what, if anything, osstest may have to say. Do I understand correctly
>> that the cited log messages are the last sign of life prior to the
>> systems rebooting?
> 
> I've found it:
> 
>     for ( addr = start; mfn_x(addr) <= mfn_x(end); mfn_add(addr, 1) )
> 
> Should be:
> 
>     for ( addr = start; mfn_x(addr) <= mfn_x(end); addr = mfn_add(addr, 1) )
> 
> mfn_add() doesn't modify the parameter.  Will see about making those
> helpers __must_check in order to avoid this happening in the future.

Hmm, yes, it's not the first time this has happened. But even seeing the
flaw I still can't explain the observed behavior: The system ought to
hang then, not instantly reboot?

Jan

Re: [regression] Re: [PATCH v2 2/2] iommu/vt-d: switch to common RMRR checker

[Roger Pau Monné]

On Wed, Feb 14, 2024 at 10:01:43AM +0100, Jan Beulich wrote:
> On 14.02.2024 09:45, Roger Pau Monné wrote:
> > On Wed, Feb 14, 2024 at 08:45:28AM +0100, Jan Beulich wrote:
> >> On 13.02.2024 23:37, Andrew Cooper wrote:
> >>> It's very likely something in this series, but the link to Intel might
> >>> just be chance of which hardware got selected, and I've got no clue why
> >>> there's a reset with no further logging out of Xen...
> >>
> >> I second this - even after looking closely at the patches again, I can't
> >> make a connection between them and the observed behavior. Didn't yet look
> >> at what, if anything, osstest may have to say. Do I understand correctly
> >> that the cited log messages are the last sign of life prior to the
> >> systems rebooting?
> > 
> > I've found it:
> > 
> >     for ( addr = start; mfn_x(addr) <= mfn_x(end); mfn_add(addr, 1) )
> > 
> > Should be:
> > 
> >     for ( addr = start; mfn_x(addr) <= mfn_x(end); addr = mfn_add(addr, 1) )
> > 
> > mfn_add() doesn't modify the parameter.  Will see about making those
> > helpers __must_check in order to avoid this happening in the future.
> 
> Hmm, yes, it's not the first time this has happened. But even seeing the
> flaw I still can't explain the observed behavior: The system ought to
> hang then, not instantly reboot?

AFAICT, it was stuck in a loop without making progress until the CI
controller decided to reboot it.

Thanks, Roger.

Re: [regression] Re: [PATCH v2 2/2] iommu/vt-d: switch to common RMRR checker

[Andrew Cooper]

On 14/02/2024 8:45 am, Roger Pau Monné wrote:
> I've found it:
>
>     for ( addr = start; mfn_x(addr) <= mfn_x(end); mfn_add(addr, 1) )
>
> Should be:
>
>     for ( addr = start; mfn_x(addr) <= mfn_x(end); addr = mfn_add(addr, 1) )

Coverity did end up spotting this.

> New defect(s) Reported-by: Coverity Scan
> Showing 1 of 1 defect(s)
>
>
> ** CID 1592056:  Incorrect expression  (USELESS_CALL)
>
>
> ________________________________________________________________________________________________________
> *** CID 1592056:  Incorrect expression  (USELESS_CALL)
> /xen/drivers/passthrough/x86/iommu.c: 807 in iommu_unity_region_ok()
> 801             return true;
> 802     
> 803         printk(XENLOG_WARNING
> 804                "%s: [%#" PRI_mfn " ,%#" PRI_mfn "] is not (entirely) in reserved memory\n",
> 805                prefix, mfn_x(start), mfn_x(end));
> 806     
>>>>     CID 1592056:  Incorrect expression  (USELESS_CALL)
>>>>     Calling "mfn_add(addr, 1UL)" is only useful for its return value, which is ignored.
> 807         for ( addr = start; mfn_x(addr) <= mfn_x(end); mfn_add(addr, 1) )
> 808         {
> 809             unsigned int type = page_get_ram_type(addr);
> 810     
> 811             if ( type == RAM_TYPE_UNKNOWN )
> 812             {


~Andrew