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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxxxx>
>>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> 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