LGTM.

I wonder if the pci_cfgregopen problem reported for AMD Ryzen systems has a similar cause...

In D19833#425441, @avg wrote:

LGTM.

I wonder if the pci_cfgregopen problem reported for AMD Ryzen systems has a similar cause...

What is the Ryzen problem ? This issue can only occur when config space is accessed before pmap is initialized, and I am only aware of pci_early_quirks which do that, which might falls to MCFG access only on some Intels.

And, after thinking about this code more, why do we check for specific chipsets and read MCFGbase from the register ? amd64 requires ACPI, were that era machines so broken that ACPI did not reported MCFG base ? Reported PR with Pentium-IV Xeon and E7500 MCH has the line

PCIe: Memory Mapped configuration base @XXXX

So may be we should remove special handling of some Intel chipsets from amd64 pci_cfgregopen() instead of my hack.

In D19833#425585, @kib wrote:

In D19833#425441, @avg wrote:

LGTM.

I wonder if the pci_cfgregopen problem reported for AMD Ryzen systems has a similar cause...

What is the Ryzen problem ? This issue can only occur when config space is accessed before pmap is initialized, and I am only aware of pci_early_quirks which do that, which might falls to MCFG access only on some Intels.

I have seen multiple reports when people had to disable hw.pci.mcfg to be able to boot a Ryzen system.
Here is one example: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=231760
But I have never looked into details...
Although, it seems that John has.

In D19833#425621, @avg wrote:

I have seen multiple reports when people had to disable hw.pci.mcfg to be able to boot a Ryzen system.
Here is one example: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=231760

This seems to be completely unrelated.

Hmm, so scottl@ added this version of MMIO access before we looked at the MCFG table, and I'm not sure that the BIOS for these chipsets don't actually already provide the MCFG table. However, on systems where we rely on the MCFG table, we won't have parsed the table to setup the mapping until later anyway, so I think the change is ok. Alternatively, we could move the fallback for these chipsets into a separate function that we explicitly call at around the same time we parse MCFG and just do it later in boot once it's ok to map the table, but that's a lot more work (though maybe not if we just call it from the pmap code at the time that pmap_initialized is set?) Reading the commit message was a bit confusing to me at first, so I think it might be useful to say that this makes those chipsets act more like other chipsets where we use MCFG as with MCFG we don't enable mmio access until part-way through the boot.

Hmm, actually, isn't this disabling MMIO entirely on these chipsets? Since cfgmech is set to CFG_MECH_1 before the return, the calls to pci_cfgregopen() in the future will all return right away and these chipsets will never use mmio. To preserve use of mmio you would need to do the callback method where we invoke pcie_cfgregopen to switch from CFG_MECH_1 to CFG_MECH_PCIE after the direct-map is setup. We should probably move the MCFG handling out of acpi.c as well and find the MCFG table the way we find the MADT tables and just have a single callback after the direct-map is setup that looks for the MCFG table to setup the mmio window and if that doesn't exist, tries the fallbacks for old chipsets so we can ensure MCFG always has preference.

In D19833#425992, @jhb wrote:

Hmm, so scottl@ added this version of MMIO access before we looked at the MCFG table, and I'm not sure that the BIOS for these chipsets don't actually already provide the MCFG table.

At least on the machine of the person who reported the problem, there is MCFG table. Chipset MCH is E7500.

However, on systems where we rely on the MCFG table, we won't have parsed the table to setup the mapping until later anyway, so I think the change is ok. Alternatively, we could move the fallback for these chipsets into a separate function that we explicitly call at around the same time we parse MCFG and just do it later in boot once it's ok to map the table, but that's a lot more work (though maybe not if we just call it from the pmap code at the time that pmap_initialized is set?) Reading the commit message was a bit confusing to me at first, so I think it might be useful to say that this makes those chipsets act more like other chipsets where we use MCFG as with MCFG we don't enable mmio access until part-way through the boot.

In fact, in the later response I proposed to remove the fallback at all. We require ACPI on amd64, and it seems that at least one BIOS is not so much broken to not provide MCFG.

Hmm, actually, isn't this disabling MMIO entirely on these chipsets? Since cfgmech is set to CFG_MECH_1 before the return, the calls to pci_cfgregopen() in the future will all return right away and these chipsets will never use mmio.

I do not think so. acpi.c::acpi_enable_pcie() calls pciE_cfgregopen(), not pci_cfgregopen(). And pcie_cfgregopen() always set mode to CFGMECH_PCIE.

To preserve use of mmio you would need to do the callback method where we invoke pcie_cfgregopen to switch from CFG_MECH_1 to CFG_MECH_PCIE after the direct-map is setup. We should probably move the MCFG handling out of acpi.c as well and find the MCFG table the way we find the MADT tables and just have a single callback after the direct-map is setup that looks for the MCFG table to setup the mmio window and if that doesn't exist, tries the fallbacks for old chipsets so we can ensure MCFG always has preference.

I think this would be overkill.

In D19833#426028, @kib wrote:

In D19833#425992, @jhb wrote:

Hmm, so scottl@ added this version of MMIO access before we looked at the MCFG table, and I'm not sure that the BIOS for these chipsets don't actually already provide the MCFG table.

At least on the machine of the person who reported the problem, there is MCFG table. Chipset MCH is E7500.

However, on systems where we rely on the MCFG table, we won't have parsed the table to setup the mapping until later anyway, so I think the change is ok. Alternatively, we could move the fallback for these chipsets into a separate function that we explicitly call at around the same time we parse MCFG and just do it later in boot once it's ok to map the table, but that's a lot more work (though maybe not if we just call it from the pmap code at the time that pmap_initialized is set?) Reading the commit message was a bit confusing to me at first, so I think it might be useful to say that this makes those chipsets act more like other chipsets where we use MCFG as with MCFG we don't enable mmio access until part-way through the boot.

In fact, in the later response I proposed to remove the fallback at all. We require ACPI on amd64, and it seems that at least one BIOS is not so much broken to not provide MCFG.

I would probably be happy to just remove the fallback if we don't need it.

Hmm, actually, isn't this disabling MMIO entirely on these chipsets? Since cfgmech is set to CFG_MECH_1 before the return, the calls to pci_cfgregopen() in the future will all return right away and these chipsets will never use mmio.

I do not think so. acpi.c::acpi_enable_pcie() calls pciE_cfgregopen(), not pci_cfgregopen(). And pcie_cfgregopen() always set mode to CFGMECH_PCIE.

Ah, if it has MCFG, yes. However, if it doesn't have MCFG it does remove it. That is, my point is that your change is actually disabling the fallback entirely, but since you aren't removing it it doesn't like you are.

To preserve use of mmio you would need to do the callback method where we invoke pcie_cfgregopen to switch from CFG_MECH_1 to CFG_MECH_PCIE after the direct-map is setup. We should probably move the MCFG handling out of acpi.c as well and find the MCFG table the way we find the MADT tables and just have a single callback after the direct-map is setup that looks for the MCFG table to setup the mmio window and if that doesn't exist, tries the fallbacks for old chipsets so we can ensure MCFG always has preference.

I think this would be overkill.

Well, we should either remove the fallback outright, or we should fix it to actually be used. I think I would be fine with just removing it outright. We should remove it on i386 as well in that case since the same list of devices is duplicated in i386.

In D19833#426071, @jhb wrote:

Well, we should either remove the fallback outright, or we should fix it to actually be used. I think I would be fine with just removing it outright.

Yes, I will update the patch with removal in a minute.

We should remove it on i386 as well in that case since the same list of devices is duplicated in i386.

I would not do this. For i386 we still allow to run without ACPI, I remember. If user did configured the machine this way, not having PCIe extended config accessible might break things.