Re: [PATCH v2] x86/pci: Stop requiring ECAM to be declared in E820, ACPI or EFI

[Bjorn Helgaas]

On Fri, Jan 26, 2024 at 12:32:34PM -0600, Mario Limonciello wrote:
> On 1/25/2024 18:35, Bjorn Helgaas wrote:
> > On Wed, Jan 17, 2024 at 11:53:50AM -0600, Mario Limonciello wrote:
> > > On 12/15/2023 16:03, Mario Limonciello wrote:
> > > > commit 7752d5cfe3d1 ("x86: validate against acpi motherboard resources")
> > > > introduced checks for ensuring that MCFG table also has memory region
> > > > reservations to ensure no conflicts were introduced from a buggy BIOS.
> ...

> > > Any thoughts on this version since our last conversation on V1?
> > 
> > Just to let you know that I'm not ignoring this, and I'm trying to
> > formulate a useful response.
> 
> Thanks, I had been wondering.
> 
> FYI - we've also added another place to make noise about this ECAM
> issue in AMDGPU.  This warning should go into 6.9:
> 
> https://lore.kernel.org/amd-gfx/20240110101319.695169-1-Jun.Ma2@xxxxxxx/

Looks similar to the PCI core warning here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/probe.c?id=v6.7#n1134

The comment says it doesn't work for devices on the root bus, though.
Maybe it could be made to work there as well?

> > mmconfig-shared.c has grown into an
> > extremely complicated mess and is a continual source of problems, so
> > I'd really like to untangle it a tiny bit if we can.
> > 
> > One thing is that per spec, ACPI motherboard resources are the ONLY
> > way to reserve ECAM space.  I'd like to reclaim a little clarity about
> > that and separate out the E820 and EFI stuff as secondary hacks.  But
> > there's an insane amount of history that got us here.
> 
> I guess you know better than anyone here.  But if my idea is
> actually viable then the E820 and EFI stuff turn into "information
> only".

That would definitely be a good thing.  I would like it if that were
more obvious from reading the code because I spend waaay too much time
staring at that labyrinth.

> > The problem we have to avoid is assigning a BAR that overlaps ECAM.
> > We assign BARs for lots of reasons.  dGPU and resizable BARs are
> > examples but there are others, like hotplug and SR-IOV.  The fact that
> > Windows works is a red herring because it allocates BARs differently.
> 
> Have we actually observed a case that assigning the BAR overlaps
> ECAM region thus far or it's hypothetical?

Yes, it has happened.  There's an example in the commit log here:
https://git.kernel.org/linus/070909e56a7d ("x86/pci: Reserve ECAM if
BIOS didn't include it in PNP0C02 _CRS")

> > And of course, if there's any way to solve this safely without
> > adding yet another kernel parameter, that would be vastly
> > preferable.
> 
> The kernel isn't static though; something we could do is keep the
> parameter around a year or two to get the bug feedback loop of
> people testing it and then rip it out if nothing comes up.

Yeah.  It's pretty hard to remove those options though.  For example,
"pci=routeirq" was added in the pre-git era and probably isn't
necessary, but how do we know nobody uses it?

Bjorn