| Message ID | 20231215220343.22523-1-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v2] x86/pci: Stop requiring ECAM to be declared in E820, ACPI or EFI | expand |
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. > > This has proceeded over time to add other types of reservation checks > for ACPI PNP resources and EFI MMIO memory type. The PCI firmware spec > does say that these checks are only required when the operating system > doesn't comprehend the firmware region: > > ``` > If the operating system does not natively comprehend reserving the MMCFG > region, the MMCFG region must be reserved by firmware. The address range > reported in the MCFG table or by _CBA method (see Section 4.1.3) must be > reserved by declaring a motherboard resource. For most systems, the > motherboard resource would appear at the root of the ACPI namespace > (under \_SB) in a node with a _HID of EISAID (PNP0C02), and the resources > in this case should not be claimed in the root PCI bus’s _CRS. The > resources can optionally be returned in Int15 E820h or EFIGetMemoryMap > as reserved memory but must always be reported through ACPI as a > motherboard resource. > ``` > > Running this check causes problems with accessing extended PCI > configuration space on OEM laptops that don't specify the region in PNP > resources or in the EFI memory map. That later manifests as problems with > dGPU and accessing resizable BAR. Similar problems don't exist in Windows > 11 with exact same laptop/firmware stack. > > Due to the stability of the Windows ecosystem that x86 machines participate > it is unlikely that using the region specified in the MCFG table as > a reservation will cause a problem. The possible worst circumstance could > be that a buggy BIOS causes a larger hole in the memory map that is > unusable for devices than intended. > > Change the default behavior to keep the region specified in MCFG even if > it's not specified in another source. This is expected to improve > machines that otherwise couldn't access PCI extended configuration space. > > In case this change causes problems, add a kernel command line parameter > that can restore the previous behavior. > > Link: https://members.pcisig.com/wg/PCI-SIG/document/15350 > PCI Firmware Specification 3.3 > Section 4.1.2 MCFG Table Description Note 2 > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- Bjorn, Any thoughts on this version since our last conversation on V1? Thanks, > v1->v2: > * Rebase on pci/next > * Add an escape hatch > * Reword commit message > --- > .../admin-guide/kernel-parameters.txt | 6 ++++++ > arch/x86/pci/mmconfig-shared.c | 19 +++++++++++++++---- > 2 files changed, 21 insertions(+), 4 deletions(-) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index 65731b060e3f..eacd0c0521c2 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -1473,6 +1473,12 @@ > (in particular on some ATI chipsets). > The kernel tries to set a reasonable default. > > + enforce_ecam_resv [X86] > + Enforce requiring an ECAM reservation specified in > + BIOS for PCI devices. > + This parameter is only valid if CONFIG_PCI_MMCONFIG > + is enabled. > + > enforcing= [SELINUX] Set initial enforcing status. > Format: {"0" | "1"} > See security/selinux/Kconfig help text. > diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c > index 0cc9520666ef..aee117c6bbf9 100644 > --- a/arch/x86/pci/mmconfig-shared.c > +++ b/arch/x86/pci/mmconfig-shared.c > @@ -34,6 +34,15 @@ static DEFINE_MUTEX(pci_mmcfg_lock); > > LIST_HEAD(pci_mmcfg_list); > > +static bool enforce_ecam_resv __read_mostly; > +static int __init parse_ecam_options(char *str) > +{ > + enforce_ecam_resv = true; > + > + return 1; > +} > +__setup("enforce_ecam_resv", parse_ecam_options); > + > static void __init pci_mmconfig_remove(struct pci_mmcfg_region *cfg) > { > if (cfg->res.parent) > @@ -569,10 +578,12 @@ static void __init pci_mmcfg_reject_broken(int early) > > list_for_each_entry(cfg, &pci_mmcfg_list, list) { > if (!pci_mmcfg_reserved(NULL, cfg, early)) { > - pr_info("not using ECAM (%pR not reserved)\n", > - &cfg->res); > - free_all_mmcfg(); > - return; > + pr_info("ECAM %pR not reserved, %s\n", &cfg->res, > + enforce_ecam_resv ? "ignoring" : "using anyway"); > + if (enforce_ecam_resv) { > + free_all_mmcfg(); > + return; > + } > } > } > } > > base-commit: 67e04d921cb6902e8c2abdbf748279d43f25213e
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. > > > > This has proceeded over time to add other types of reservation checks > > for ACPI PNP resources and EFI MMIO memory type. The PCI firmware spec > > does say that these checks are only required when the operating system > > doesn't comprehend the firmware region: > > > > ``` > > If the operating system does not natively comprehend reserving the MMCFG > > region, the MMCFG region must be reserved by firmware. The address range > > reported in the MCFG table or by _CBA method (see Section 4.1.3) must be > > reserved by declaring a motherboard resource. For most systems, the > > motherboard resource would appear at the root of the ACPI namespace > > (under \_SB) in a node with a _HID of EISAID (PNP0C02), and the resources > > in this case should not be claimed in the root PCI bus’s _CRS. The > > resources can optionally be returned in Int15 E820h or EFIGetMemoryMap > > as reserved memory but must always be reported through ACPI as a > > motherboard resource. > > ``` > > > > Running this check causes problems with accessing extended PCI > > configuration space on OEM laptops that don't specify the region in PNP > > resources or in the EFI memory map. That later manifests as problems with > > dGPU and accessing resizable BAR. Similar problems don't exist in Windows > > 11 with exact same laptop/firmware stack. > > > > Due to the stability of the Windows ecosystem that x86 machines participate > > it is unlikely that using the region specified in the MCFG table as > > a reservation will cause a problem. The possible worst circumstance could > > be that a buggy BIOS causes a larger hole in the memory map that is > > unusable for devices than intended. > > > > Change the default behavior to keep the region specified in MCFG even if > > it's not specified in another source. This is expected to improve > > machines that otherwise couldn't access PCI extended configuration space. > > > > In case this change causes problems, add a kernel command line parameter > > that can restore the previous behavior. > > > > Link: https://members.pcisig.com/wg/PCI-SIG/document/15350 > > PCI Firmware Specification 3.3 > > Section 4.1.2 MCFG Table Description Note 2 > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > --- > > Bjorn, > > 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. 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. 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. It's also little bit of a burr under my saddle to add things for a problem on unspecified machines, where I don't even know whether the machines are already in the field or the firmware could still be fixed. And of course, if there's any way to solve this safely without adding yet another kernel parameter, that would be vastly preferable. Sorry, nothing actionable here but wanted to let you know that it's keeping me awake at night ;) Bjorn > > v1->v2: > > * Rebase on pci/next > > * Add an escape hatch > > * Reword commit message > > --- > > .../admin-guide/kernel-parameters.txt | 6 ++++++ > > arch/x86/pci/mmconfig-shared.c | 19 +++++++++++++++---- > > 2 files changed, 21 insertions(+), 4 deletions(-) > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > > index 65731b060e3f..eacd0c0521c2 100644 > > --- a/Documentation/admin-guide/kernel-parameters.txt > > +++ b/Documentation/admin-guide/kernel-parameters.txt > > @@ -1473,6 +1473,12 @@ > > (in particular on some ATI chipsets). > > The kernel tries to set a reasonable default. > > + enforce_ecam_resv [X86] > > + Enforce requiring an ECAM reservation specified in > > + BIOS for PCI devices. > > + This parameter is only valid if CONFIG_PCI_MMCONFIG > > + is enabled. > > + > > enforcing= [SELINUX] Set initial enforcing status. > > Format: {"0" | "1"} > > See security/selinux/Kconfig help text. > > diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c > > index 0cc9520666ef..aee117c6bbf9 100644 > > --- a/arch/x86/pci/mmconfig-shared.c > > +++ b/arch/x86/pci/mmconfig-shared.c > > @@ -34,6 +34,15 @@ static DEFINE_MUTEX(pci_mmcfg_lock); > > LIST_HEAD(pci_mmcfg_list); > > +static bool enforce_ecam_resv __read_mostly; > > +static int __init parse_ecam_options(char *str) > > +{ > > + enforce_ecam_resv = true; > > + > > + return 1; > > +} > > +__setup("enforce_ecam_resv", parse_ecam_options); > > + > > static void __init pci_mmconfig_remove(struct pci_mmcfg_region *cfg) > > { > > if (cfg->res.parent) > > @@ -569,10 +578,12 @@ static void __init pci_mmcfg_reject_broken(int early) > > list_for_each_entry(cfg, &pci_mmcfg_list, list) { > > if (!pci_mmcfg_reserved(NULL, cfg, early)) { > > - pr_info("not using ECAM (%pR not reserved)\n", > > - &cfg->res); > > - free_all_mmcfg(); > > - return; > > + pr_info("ECAM %pR not reserved, %s\n", &cfg->res, > > + enforce_ecam_resv ? "ignoring" : "using anyway"); > > + if (enforce_ecam_resv) { > > + free_all_mmcfg(); > > + return; > > + } > > } > > } > > } > > > > base-commit: 67e04d921cb6902e8c2abdbf748279d43f25213e >
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. >>> >>> This has proceeded over time to add other types of reservation checks >>> for ACPI PNP resources and EFI MMIO memory type. The PCI firmware spec >>> does say that these checks are only required when the operating system >>> doesn't comprehend the firmware region: >>> >>> ``` >>> If the operating system does not natively comprehend reserving the MMCFG >>> region, the MMCFG region must be reserved by firmware. The address range >>> reported in the MCFG table or by _CBA method (see Section 4.1.3) must be >>> reserved by declaring a motherboard resource. For most systems, the >>> motherboard resource would appear at the root of the ACPI namespace >>> (under \_SB) in a node with a _HID of EISAID (PNP0C02), and the resources >>> in this case should not be claimed in the root PCI bus’s _CRS. The >>> resources can optionally be returned in Int15 E820h or EFIGetMemoryMap >>> as reserved memory but must always be reported through ACPI as a >>> motherboard resource. >>> ``` >>> >>> Running this check causes problems with accessing extended PCI >>> configuration space on OEM laptops that don't specify the region in PNP >>> resources or in the EFI memory map. That later manifests as problems with >>> dGPU and accessing resizable BAR. Similar problems don't exist in Windows >>> 11 with exact same laptop/firmware stack. >>> >>> Due to the stability of the Windows ecosystem that x86 machines participate >>> it is unlikely that using the region specified in the MCFG table as >>> a reservation will cause a problem. The possible worst circumstance could >>> be that a buggy BIOS causes a larger hole in the memory map that is >>> unusable for devices than intended. >>> >>> Change the default behavior to keep the region specified in MCFG even if >>> it's not specified in another source. This is expected to improve >>> machines that otherwise couldn't access PCI extended configuration space. >>> >>> In case this change causes problems, add a kernel command line parameter >>> that can restore the previous behavior. >>> >>> Link: https://members.pcisig.com/wg/PCI-SIG/document/15350 >>> PCI Firmware Specification 3.3 >>> Section 4.1.2 MCFG Table Description Note 2 >>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >>> --- >> >> Bjorn, >> >> 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@amd.com/ It will at least be interesting to see how many people come out of the woodwork to report that new warning. > 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". > > 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? I would think that if the reservation is made by ECAM first, then any conflict for any reason that tries to assign it will just get a smaller BAR, but not necessarily a functional problem. But that's also part of why I was thinking kernel command line for us to have the escape hatch. > > It's also little bit of a burr under my saddle to add things for a > problem on unspecified machines, where I don't even know whether the > machines are already in the field or the firmware could still be > fixed. Of the two machines I know of: * One of them has been fixed by a BIOS change before it's final production stage. * The other is still affected. Here is the info for the still affected one. It's been shipping already. Alienware Alienware m18 R1 AMD/0RU01H, BIOS 1.2.2 04/21/2023 > > 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. > > Sorry, nothing actionable here but wanted to let you know that it's > keeping me awake at night ;) :) > > Bjorn > >>> v1->v2: >>> * Rebase on pci/next >>> * Add an escape hatch >>> * Reword commit message >>> --- >>> .../admin-guide/kernel-parameters.txt | 6 ++++++ >>> arch/x86/pci/mmconfig-shared.c | 19 +++++++++++++++---- >>> 2 files changed, 21 insertions(+), 4 deletions(-) >>> >>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt >>> index 65731b060e3f..eacd0c0521c2 100644 >>> --- a/Documentation/admin-guide/kernel-parameters.txt >>> +++ b/Documentation/admin-guide/kernel-parameters.txt >>> @@ -1473,6 +1473,12 @@ >>> (in particular on some ATI chipsets). >>> The kernel tries to set a reasonable default. >>> + enforce_ecam_resv [X86] >>> + Enforce requiring an ECAM reservation specified in >>> + BIOS for PCI devices. >>> + This parameter is only valid if CONFIG_PCI_MMCONFIG >>> + is enabled. >>> + >>> enforcing= [SELINUX] Set initial enforcing status. >>> Format: {"0" | "1"} >>> See security/selinux/Kconfig help text. >>> diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c >>> index 0cc9520666ef..aee117c6bbf9 100644 >>> --- a/arch/x86/pci/mmconfig-shared.c >>> +++ b/arch/x86/pci/mmconfig-shared.c >>> @@ -34,6 +34,15 @@ static DEFINE_MUTEX(pci_mmcfg_lock); >>> LIST_HEAD(pci_mmcfg_list); >>> +static bool enforce_ecam_resv __read_mostly; >>> +static int __init parse_ecam_options(char *str) >>> +{ >>> + enforce_ecam_resv = true; >>> + >>> + return 1; >>> +} >>> +__setup("enforce_ecam_resv", parse_ecam_options); >>> + >>> static void __init pci_mmconfig_remove(struct pci_mmcfg_region *cfg) >>> { >>> if (cfg->res.parent) >>> @@ -569,10 +578,12 @@ static void __init pci_mmcfg_reject_broken(int early) >>> list_for_each_entry(cfg, &pci_mmcfg_list, list) { >>> if (!pci_mmcfg_reserved(NULL, cfg, early)) { >>> - pr_info("not using ECAM (%pR not reserved)\n", >>> - &cfg->res); >>> - free_all_mmcfg(); >>> - return; >>> + pr_info("ECAM %pR not reserved, %s\n", &cfg->res, >>> + enforce_ecam_resv ? "ignoring" : "using anyway"); >>> + if (enforce_ecam_resv) { >>> + free_all_mmcfg(); >>> + return; >>> + } >>> } >>> } >>> } >>> >>> base-commit: 67e04d921cb6902e8c2abdbf748279d43f25213e >>
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@amd.com/ 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
On 1/26/2024 13:29, Bjorn Helgaas wrote: > 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@amd.com/ > > 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? IMO it's not loud enough either. I think it's better to keep the both, here's my logic: If someone has this problem that prompted this series the first thing they notice is problems "with the GPU". They'll probably start looking at the kernel log for ERR and WARN related to the GPU. > >>> 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") But so in this case; if there was a full ECAM reservation made from MMCFG instead then Linux wouldn't have tried to put it on top of that space. > >>> 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? Detect it's in use and drop a notice() or higher into the logs like this: "pci=irq has been deprecated and is planned to be removed from the kernel on YY/ZZZZ. If you need this for your system to work, please raise an email to linux-pci@vger.kernel.org" If you give it ~2 years, that gives enough time to get through about 2 LTS kernels. People who need it by then but chose not to report it still have several LTS kernels to fall back on.
[+cc Ivan in case there's opportunity to improve FWTS] 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. > > > > This has proceeded over time to add other types of reservation checks > > for ACPI PNP resources and EFI MMIO memory type. The PCI firmware spec > > does say that these checks are only required when the operating system > > doesn't comprehend the firmware region: > > > > ``` > > If the operating system does not natively comprehend reserving the MMCFG > > region, the MMCFG region must be reserved by firmware. The address range > > reported in the MCFG table or by _CBA method (see Section 4.1.3) must be > > reserved by declaring a motherboard resource. For most systems, the > > motherboard resource would appear at the root of the ACPI namespace > > (under \_SB) in a node with a _HID of EISAID (PNP0C02), and the resources > > in this case should not be claimed in the root PCI bus’s _CRS. The > > resources can optionally be returned in Int15 E820h or EFIGetMemoryMap > > as reserved memory but must always be reported through ACPI as a > > motherboard resource. > > ``` > > > > Running this check causes problems with accessing extended PCI > > configuration space on OEM laptops that don't specify the region in PNP > > resources or in the EFI memory map. That later manifests as problems with > > dGPU and accessing resizable BAR. Similar problems don't exist in Windows > > 11 with exact same laptop/firmware stack. > > > > Due to the stability of the Windows ecosystem that x86 machines participate > > it is unlikely that using the region specified in the MCFG table as > > a reservation will cause a problem. The possible worst circumstance could > > be that a buggy BIOS causes a larger hole in the memory map that is > > unusable for devices than intended. > > > > Change the default behavior to keep the region specified in MCFG even if > > it's not specified in another source. This is expected to improve > > machines that otherwise couldn't access PCI extended configuration space. > > > > In case this change causes problems, add a kernel command line parameter > > that can restore the previous behavior. > > > > Link: https://members.pcisig.com/wg/PCI-SIG/document/15350 > > PCI Firmware Specification 3.3 > > Section 4.1.2 MCFG Table Description Note 2 > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > --- > > Bjorn, > > Any thoughts on this version since our last conversation on V1? I really want to clarify the dmesg logging such that it's clear that PNP0C02 reservation is the only valid way to reserve the space described by MCFG. Obviously we have to retain the fallbacks, but I think there should be FW_BUG logging in that case. We currently only do FW_INFO for missing PNP0C02 reservations. I think we should try to change FWTS so it validates MCFG addresses against the PNP0C02 reservations required by spec, instead of searching E820 for them. The spec doesn't require MCFG regions to be in E820, and I think searching there encourages the wrong behavior. It probably also doesn't work at all on arm64, since it doesn't have E820 at all. The /sys/devices/pnp0/00:xx/resources files and "system 00:xx: [mem ...] has been reserved" lines in dmesg would be much better places to check. Bjorn
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 65731b060e3f..eacd0c0521c2 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -1473,6 +1473,12 @@ (in particular on some ATI chipsets). The kernel tries to set a reasonable default. + enforce_ecam_resv [X86] + Enforce requiring an ECAM reservation specified in + BIOS for PCI devices. + This parameter is only valid if CONFIG_PCI_MMCONFIG + is enabled. + enforcing= [SELINUX] Set initial enforcing status. Format: {"0" | "1"} See security/selinux/Kconfig help text. diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c index 0cc9520666ef..aee117c6bbf9 100644 --- a/arch/x86/pci/mmconfig-shared.c +++ b/arch/x86/pci/mmconfig-shared.c @@ -34,6 +34,15 @@ static DEFINE_MUTEX(pci_mmcfg_lock); LIST_HEAD(pci_mmcfg_list); +static bool enforce_ecam_resv __read_mostly; +static int __init parse_ecam_options(char *str) +{ + enforce_ecam_resv = true; + + return 1; +} +__setup("enforce_ecam_resv", parse_ecam_options); + static void __init pci_mmconfig_remove(struct pci_mmcfg_region *cfg) { if (cfg->res.parent) @@ -569,10 +578,12 @@ static void __init pci_mmcfg_reject_broken(int early) list_for_each_entry(cfg, &pci_mmcfg_list, list) { if (!pci_mmcfg_reserved(NULL, cfg, early)) { - pr_info("not using ECAM (%pR not reserved)\n", - &cfg->res); - free_all_mmcfg(); - return; + pr_info("ECAM %pR not reserved, %s\n", &cfg->res, + enforce_ecam_resv ? "ignoring" : "using anyway"); + if (enforce_ecam_resv) { + free_all_mmcfg(); + return; + } } } }
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. This has proceeded over time to add other types of reservation checks for ACPI PNP resources and EFI MMIO memory type. The PCI firmware spec does say that these checks are only required when the operating system doesn't comprehend the firmware region: ``` If the operating system does not natively comprehend reserving the MMCFG region, the MMCFG region must be reserved by firmware. The address range reported in the MCFG table or by _CBA method (see Section 4.1.3) must be reserved by declaring a motherboard resource. For most systems, the motherboard resource would appear at the root of the ACPI namespace (under \_SB) in a node with a _HID of EISAID (PNP0C02), and the resources in this case should not be claimed in the root PCI bus’s _CRS. The resources can optionally be returned in Int15 E820h or EFIGetMemoryMap as reserved memory but must always be reported through ACPI as a motherboard resource. ``` Running this check causes problems with accessing extended PCI configuration space on OEM laptops that don't specify the region in PNP resources or in the EFI memory map. That later manifests as problems with dGPU and accessing resizable BAR. Similar problems don't exist in Windows 11 with exact same laptop/firmware stack. Due to the stability of the Windows ecosystem that x86 machines participate it is unlikely that using the region specified in the MCFG table as a reservation will cause a problem. The possible worst circumstance could be that a buggy BIOS causes a larger hole in the memory map that is unusable for devices than intended. Change the default behavior to keep the region specified in MCFG even if it's not specified in another source. This is expected to improve machines that otherwise couldn't access PCI extended configuration space. In case this change causes problems, add a kernel command line parameter that can restore the previous behavior. Link: https://members.pcisig.com/wg/PCI-SIG/document/15350 PCI Firmware Specification 3.3 Section 4.1.2 MCFG Table Description Note 2 Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- v1->v2: * Rebase on pci/next * Add an escape hatch * Reword commit message --- .../admin-guide/kernel-parameters.txt | 6 ++++++ arch/x86/pci/mmconfig-shared.c | 19 +++++++++++++++---- 2 files changed, 21 insertions(+), 4 deletions(-) base-commit: 67e04d921cb6902e8c2abdbf748279d43f25213e