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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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

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.