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

["Roger Pau Monné" <roger.pau@citrix.com>]

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.