[Intel-gfx] [PATCH 1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not

[Ville Syrjälä]

On Tue, Nov 14, 2017 at 06:44:51PM -0200, Paulo Zanoni wrote:
> Em Ter, 2017-11-14 às 22:34 +0200, Ville Syrjälä escreveu:
> > On Tue, Nov 14, 2017 at 06:12:41PM -0200, Paulo Zanoni wrote:
> > > Em Qui, 2017-11-02 às 17:17 +0200, Ville Syrjala escreveu:
> > > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > 
> > > > Apparently there are some machines that put semi-sensible looking
> > > > values
> > > > into the stolen "reserved" base and size, except those values are
> > > > actually
> > > > outside the stolen memory. There is a bit in the register which
> > > > supposedly could tell us whether the reserved area is even
> > > > enabled or
> > > > not. Let's check for that before we go trusting the base and
> > > > size.
> > > 
> > > If this is such a problem since g4x, why didn't we spot it earlier?
> > > It
> > > would be nice if you could explain in the commit message (or at
> > > least
> > > in this email) what are the consequences you're seeing that made
> > > you
> > > realize about this problem. Did something actually explode? I'm
> > > genuinely curious.
> > 
> > CI is getting ping-ping on the SNB shards. Apparently the BIOS leaves
> > some garbage in the the base+size fields of the register, and
> > sometimes
> > that results in the reserved area landing inside stolen (at which
> > point
> > we just reduce the size of stolen and continue), and sometimes it
> > lands
> > somewhere outside (at which point we just disable stolen entirely).
> > Hence the FBC tests sometimes find enough stolen, sometimes not.
> 
> Couldn't it be that we're failing to read the "size of stolen"
> registers instead of the "stolen reserved stuff" regs? All of the
> stolen registers have a history of controversy...

I think stolen itself looks correct. IIRC it was somewhere above
the 3 GiB mark and neatly inside a e820 reserved region.

The reserved region (at least in the logs I looked at) was somewhere
around the 17 MiB mark, right on top of normal RAM according to e820.
No sane BIOS would put it there.

> 
> 
> > 
> > > 
> > > 
> > > Now talking about the patch itself:
> > > 
> > > If we're going to assume random bits instead of a full-zero in
> > > (possibly) uninitialized stuff, shouldn't we first check bit 1,
> > > which
> > > is supposed to tell us if the whole reg is locked or not?
> > 
> > Not sure we can trust the BIOS to always set the lock bit. I suppose
> > it
> > really should, but I don't recall seeing anything in the docs saying
> > that not setting the lock bit would somehow disable the reserved
> > area.
> 
> The way things are worded, it suggests that if the lock bit is zero,
> everything else could be garbage. Bit 0 is only locked after bit 1 is
> locked. It doesn't make sense to me to trust bit 0 without trusting bit
> 1.

The hardware itself will trust bit 0 without bit 1. I think that's all
that really matters.

> 
> So my understanding is that as long as the enable bit is set the
> > hardware will prevent us from accessing the are, regardless of
> > whether
> > the settings have been locked down or not.
> > 
> > This wouldn't be the first lock bit some BIOS versions forget to set
> > BTW.
> 
> But it could be worth checking this now that you have access to the
> machines that have the random stuff...

Yeah. I guess it might be interesting to see the state of the lock bit
as well. I'll see if I can get someone to grab it for us (/me no longer
has any idea how to access those machines...).

-- 
Ville Syrjälä
Intel OTC