lkml.org 
[lkml]   [2020]   [Apr]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v1 1/1] PCI/AER: Use _OSC negotiation to determine AER ownership
Date
On 4/28/2020 3:37 PM, Bjorn Helgaas wrote:
> [EXTERNAL EMAIL]
>
> [+to Mario, Austin, Rafael; Dell folks, I suspect this commit will
> break Dell servers but I'd like your opinion]
>
> <snip>
Thanks Bjorn, for the heads up. I checked with our server BIOS team and
they say that only checking _OSC for AER should work on our servers. We
always configure_OSC and the HEST FIRMWARE_FIRST flag to retain firmware
control of AER so either could be checked.

> I *really* want the patch because the current mix of using both _OSC
> and FIRMWARE_FIRST to determine AER capability ownership is a mess and
> getting worse, but I'm more and more doubtful.
>
> My contention is that if firmware doesn't want the OS to use the AER
> capability it should simply decline to grant control via _OSC.
I agree per spec that _OSC should be used and this was confirmed by the
ACPI working group. Alex had submitted a patch for us [2] to switch to
using _OSC to determine AER ownership following the decision in the ACPI
working group.

[2] https://lkml.org/lkml/2018/11/16/202

>
> But things like 0584396157ad ("PCI: PCIe AER: honor ACPI HEST FIRMWARE
> FIRST mode") [1] suggest that some machines grant AER control to the
> OS via _OSC, but still expect the OS to leave AER alone for certain
> devices.
AFAIK, no Dell server, including the 11G servers mentioned in that
patch, have granted control of AER via _OSC and set HEST FIRMWARE_FIRST
for some devices. I don't think this model is even support by the
ACPI/PCIe standards. Yes, you can set the bits that way, but there is
no text I've found that says how the OS/firmware should behave in that
scenario. In order to be interoperable, I think someone would need to
standardized how the OS/firmware would could co-ordinate in such a model.
>
> I think the FIRMWARE_FIRST language in the ACPI spec is really too
> vague to tell the OS not to use the AER Capability, but it seems like
> that's what commits like [1] rely on.
>
> The current _OSC definition (PCI Firmware r3.2) applies only to
> PNP0A03/PNP0A08 devices, but it's conceivable that it could be
> extended to other devices if we need per-device AER Capability
> ownership.
>
> [1] https://git.kernel.org/linus/0584396157ad
>
>> commit 8f8e42e7c2dd ("PCI/AER: Use only _OSC to determine AER ownership")
>> Author: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> Date: Mon Apr 27 18:25:13 2020 -0500
>>
>> PCI/AER: Use only _OSC to determine AER ownership
>>
>> Per the PCI Firmware spec, r3.2, sec 4.5.1, the OS can request control of
>> AER via bit 3 of the _OSC Control Field. In the returned value of the
>> Control Field:
>>
>> The firmware sets [bit 3] to 1 to grant control over PCI Express Advanced
>> Error Reporting. ... after control is transferred to the operating
>> system, firmware must not modify the Advanced Error Reporting Capability.
>> If control of this feature was requested and denied or was not requested,
>> firmware returns this bit set to 0.
>>
>> Previously the pci_root driver looked at the HEST FIRMWARE_FIRST bit to
>> determine whether to request ownership of the AER Capability. This was
>> based on ACPI spec v6.3, sec 18.3.2.4, and similar sections, which say
>> things like:
>>
>> Bit [0] - FIRMWARE_FIRST: If set, indicates that system firmware will
>> handle errors from this source first.
>>
>> Bit [1] - GLOBAL: If set, indicates that the settings contained in this
>> structure apply globally to all PCI Express Devices.
>>
>> These ACPI references don't say anything about ownership of the AER
>> Capability.
>>
>> Remove use of the FIRMWARE_FIRST bit and rely only on the _OSC bit to
>> determine whether we have control of the AER Capability.
>>
>> Link: https://lore.kernel.org/r/67af2931705bed9a588b5a39d369cb70b9942190.1587925636.git.sathyanarayanan.kuppuswamy@linux.intel.com
>> [bhelgaas: commit log, split patches]
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index ac8ad6cb82aa..9e235c1a75ff 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -483,13 +483,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>> if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
>> control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
>>
>> - if (pci_aer_available()) {
>> - if (aer_acpi_firmware_first())
>> - dev_info(&device->dev,
>> - "PCIe AER handled by firmware\n");
>> - else
>> - control |= OSC_PCI_EXPRESS_AER_CONTROL;
>> - }
>> + if (pci_aer_available())
>> + control |= OSC_PCI_EXPRESS_AER_CONTROL;
>>
>> /*
>> * Per the Downstream Port Containment Related Enhancements ECN to
>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>> index f4274d301235..efc26773cc6d 100644
>> --- a/drivers/pci/pcie/aer.c
>> +++ b/drivers/pci/pcie/aer.c
>> @@ -318,30 +318,6 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev)
>> aer_set_firmware_first(dev);
>> return dev->__aer_firmware_first;
>> }
>> -
>> -static bool aer_firmware_first;
>> -
>> -/**
>> - * aer_acpi_firmware_first - Check if APEI should control AER.
>> - */
>> -bool aer_acpi_firmware_first(void)
>> -{
>> - static bool parsed = false;
>> - struct aer_hest_parse_info info = {
>> - .pci_dev = NULL, /* Check all PCIe devices */
>> - .firmware_first = 0,
>> - };
>> -
>> - if (pcie_ports_native)
>> - return false;
>> -
>> - if (!parsed) {
>> - apei_hest_parse(aer_hest_parse, &info);
>> - aer_firmware_first = info.firmware_first;
>> - parsed = true;
>> - }
>> - return aer_firmware_first;
>> -}
>> #endif
>>
>> #define PCI_EXP_AER_FLAGS (PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
>> @@ -1523,7 +1499,7 @@ static struct pcie_port_service_driver aerdriver = {
>> */
>> int __init pcie_aer_init(void)
>> {
>> - if (!pci_aer_available() || aer_acpi_firmware_first())
>> + if (!pci_aer_available())
>> return -ENXIO;
>> return pcie_port_service_register(&aerdriver);
>> }
>> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>> index 2d155bfb8fbf..11c98875538a 100644
>> --- a/include/linux/pci-acpi.h
>> +++ b/include/linux/pci-acpi.h
>> @@ -125,10 +125,4 @@ static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>> static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
>> #endif /* CONFIG_ACPI */
>>
>> -#ifdef CONFIG_ACPI_APEI
>> -extern bool aer_acpi_firmware_first(void);
>> -#else
>> -static inline bool aer_acpi_firmware_first(void) { return false; }
>> -#endif
>> -
>> #endif /* _PCI_ACPI_H_ */



\
 
 \ /
  Last update: 2020-04-29 17:25    [W:0.081 / U:0.680 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site