On 02/27/16 03:13, Ni, Ruiyu wrote:
> Basically I don't agree to add a CFG space range field.
> See embedded reply in below.
>
> Regards,
> Ray
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Saturday, February 27, 2016 8:23 AM
>> To: edk2-devel-01 <edk2-de...@ml01.01.org>
>> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Justen, Jordan L
>> <jordan.l.jus...@intel.com>; Marcel Apfelbaum <mar...@redhat.com>
>> Subject: [PATCH 01/17] MdeModulePkg: PciHostBridgeDxe: don't assume extended
>> config space
>>
>> The "PcAtChipsetPkg/PciHostBridgeDxe" driver hard-codes the [0x00..0xFF]
>> range as valid config space offsets, in the RootBridgeIoCheckParameter()
>> function -- see the MAX_PCI_REG_ADDRESS macro. This driver uses IO ports
>> 0xCF8 / 0xCFC to access PCI config space.
>>
>> The (soon to be removed) "OvmfPkg/PciHostBridgeDxe" does the same.
>>
>> The "ArmVirtPkg/PciHostBridgeDxe" uses PCI Express (ECAM) instead, and
>> hard-codes the [0x00..0xFFF] range as valid config space offsets.
>>
>> The "MdeModulePkg/Bus/Pci/PciHostBridgeDxe" driver hard-codes the
>> [0x00..0xFFF] range as valid config space offsets, without actually
>> knowing if the platform uses PCI Express (ECAM) or plain PCI (0xCF8 /
>> 0xCFC). For this generalized driver, the config access method is
>> ultimately decided by the platform's PciSegmentLib instance.
>>
>> Quoting the "MdePkg/Include/Library/PciSegmentLib.h" library class file:
>>
>> These functions perform PCI configuration cycles using the default PCI
>> configuration access method. This may use I/O ports 0xCF8 and 0xCFC to
>> perform PCI configuration accesses, or it may use MMIO registers
>> relative to the PcdPciExpressBaseAddress, or it may use some alternate
>> access method. [...]
>>
>> Clearly the configuration access method determines the boundaries of the
>> config space as well, for each individual PCI function. However, the
>> PciSegmentLib class provides no API for PciHostBridgeDxe to query these
>> boundaries!
>>
>> In order to remedy this, add another PCI_ROOT_BRIDGE_APERTURE field to the
>> root bridge structures in both PciHostBridgeDxe and the PciHostBridgeLib
>> class. This way platforms can control the PCI config space boundaries for
>> the purposes of PciHostBridgeDxe, in accordance with their config access
>> methods.
>>
>> This bug causes the following symptoms with OVMF and QEMU:
>>
>> QEMU's i440fx machine type is a PCI (0xCF8 / 0xCFC) machine. It supports
>> physical device assignment. When assigning a physical EVGA GTX750 GPU to
>> the guest, the PCI bus driver correctly determines that the GPU is a PCIe
>> device, and tries to locate its extended capabilities (specifically, ARI),
>> starting at config space offset 0x100.
>>
>> When OVMF is built with "OvmfPkg/PciHostBridgeDxe", this config access is
>> caught and rejected by RootBridgeIoCheckParameter(), due to the limit
>> being 0xFF. (And the PCI bus driver gracefully recovers from this
>> rejection.)
>
> Does the platform expect the graceful recovery? I may treat it as a silent
> failure:)
> Some additional devices cannot come up due to this recovery/failure but
> platform
> developer without deep debugging cannot know the root cause, if ARI is
> disabled.
>
>>
>> However, "MdeModulePkg/Bus/Pci/PciHostBridgeDxe" lets the address through,
>> because its variant of the same function hard-codes 0xFFF as the config
>> space limit.
>>
>> Then RootBridgeIoPciAccess() sends the (invalid) address down the
>> following chain of libraries:
>>
>> BasePciSegmentLibPci [class: PciSegmentLib]
>> BasePciLibCf8 [class: PciLib]
>> BasePciCf8Lib [class: PciCf8Lib]
>>
>> Finally, ASSERT_INVALID_PCI_ADDRESS() in PciCf8Read32() realizes that
>> offset 0x100 is invalid for the 0xCF8 / 0xCFC config access method, and
>> the firmware halts.
>
> The silent failure is bad. So the assertion is a good to tell platform
> developer
> to choose the correct PciSegment library instance.
I agree that the assertion is good. BasePciCf8Lib should indeed catch
invalid addresses, and complain *loudly*.
However, my patch is not about suppressing this assertion. My patch
corrects an error in the RootBridgeIoCheckParameter() function in
"MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c".
The error in the RootBridgeIoCheckParameter() function is that it
accepts PCI config space accesses as valid up to register offset 0xFFF,
even if the platform doesn't support that. On a platform where only the
0xCF8 access method is available, accepting config space offsets up to
0xFFF is a bug. On such a platform, offsets >= 0x100 should be rejected
as invalid. That's all.
The rest of the commit message just describes how this error in
RootBridgeIoCheckParameter() leads to a failed assertion later. The
failed assertion is *justified*. It is the job of
RootBridgeIoCheckParameter() to recognize and catch the incorrect offset
*earlier*.
So, hard-coding 0xFFF in RootBridgeIoCheckParameter() is the bug. The
platform must be able to provide that value as a parameter. That's why I
listed the other host bridge drivers in the commit message, as examples:
in those drivers, the config access method and the maximum config offset
are *in sync*. In the case of this driver however, they are out of sync,
if the PciSegmentLib library can only provide 0xCF8 access.
Your suggestion that the platform developer choose the "correct"
PciSegmentLib instance is not valid -- the platform developer has zero
liberty in "choosing" the library. The platform (the hardware)
*dictates* that choice. If the platform is only capable of 0xCF8, and
incapable of PCI Express (ECAM), then 0xFFF as a config space limit is
invalid, plain and simple, and the PciSegmentLib *instance* cannot do
anything about it.
*Normally*, it would be the job of the PciSegmentLib *class* to provide
an interface where the maximum config space offset can be queried. Then
PciHostBridgeDxe could call that interface -- which would return 0xFF,
0xFFF, or whatever else matching the internals of the PciSegmentLib
*instance* -- and RootBridgeIoCheckParameter() could enforce *that* limit.
However, the PciSegmentLib *class* lacks such an API. So we need another
way to tell the limit to PciHostBridgeDxe.
Thus, this argument comprises two parts:
1. PciHostBridgeDxe needs to use a platform dependent config space
limit: 0xFF, 0xFFF, or something else that matches the platform's
PciSegmentLib instance. Correcting this bug is not optional, it is
unavoidable. Hard-coding an ECAM assumption in the driver is a bug.
2. The other question is *whence exactly* this information should come.
There are several alternatives here; let me list a few:
* Add a new API to PciSegmentLib that returns the limit.
* Or, add a new API to PciHostBridgeLib that returns the limit.
* Or, add a new PCD that communicates the limit.
* Or, in the driver, investigate PcdPciExpressBaseAddress (which is
the central PCD for the base of the MMCONFIG / ECAM area). If the
value is MAX_UINT64 -- which is obviously unusable as ECAM --, then
assume a limit of 0xFF. Otherwise, assume 0xFFF.
* Or, pass the limit as part of the structure that PciHostBridgeLib
already produces. This patch implements this alternative.
I'm absolutely fine with any one of the options under (2). But, (1) must
be fixed in *some* way.
Thanks
Laszlo
>
>>
>> Fix this by informing the generalized PciHostBridgeDxe driver about the
>> platform's config space boundaries, so that the driver can recognize and
>> reject out-of-bounds accesses in time.
>>
>> Cc: Ruiyu Ni <ruiyu...@intel.com>
>> Cc: Jordan Justen <jordan.l.jus...@intel.com>
>> Cc: Marcel Apfelbaum <mar...@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
>> ---
>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h | 1 +
>> MdeModulePkg/Include/Library/PciHostBridgeLib.h | 1 +
>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 6 ++++--
>> 3 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
>> index b1e83f1c9089..2dfc8963f8e9 100644
>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
>> @@ -71,6 +71,7 @@ typedef struct {
>> PCI_ROOT_BRIDGE_APERTURE PMem;
>> PCI_ROOT_BRIDGE_APERTURE MemAbove4G;
>> PCI_ROOT_BRIDGE_APERTURE PMemAbove4G;
>> + PCI_ROOT_BRIDGE_APERTURE Pci;
>> BOOLEAN DmaAbove4G;
>> VOID *ConfigBuffer;
>> EFI_DEVICE_PATH_PROTOCOL *DevicePath;
>> diff --git a/MdeModulePkg/Include/Library/PciHostBridgeLib.h
>> b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
>> index b1dba0f754d7..a9e9ca308c7c 100644
>> --- a/MdeModulePkg/Include/Library/PciHostBridgeLib.h
>> +++ b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
>> @@ -44,6 +44,7 @@ typedef struct {
>> PCI_ROOT_BRIDGE_APERTURE MemAbove4G; ///< MMIO aperture above
>> 4GB which can be used by the
>> root bridge.
>> PCI_ROOT_BRIDGE_APERTURE PMem; ///< Prefetchable MMIO
>> aperture below 4GB which can be
>> used by the root bridge.
>> PCI_ROOT_BRIDGE_APERTURE PMemAbove4G; ///< Prefetchable MMIO
>> aperture above 4GB which can be
>> used by the root bridge.
>> + PCI_ROOT_BRIDGE_APERTURE Pci; ///< PCI config space
>> range that is valid for the devices behind
>> the root bridge.
>> EFI_DEVICE_PATH_PROTOCOL *DevicePath; ///< Device path.
>> } PCI_ROOT_BRIDGE;
>>
>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>> index 332860eb3819..6b7bd74290a7 100644
>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>> @@ -90,6 +90,7 @@ CreateRootBridge (
>> DEBUG ((EFI_D_INFO, " MemAbove4G: %lx - %lx\n", Bridge->MemAbove4G.Base,
>> Bridge->MemAbove4G.Limit));
>> DEBUG ((EFI_D_INFO, " PMem: %lx - %lx\n", Bridge->PMem.Base,
>> Bridge->PMem.Limit));
>> DEBUG ((EFI_D_INFO, " PMemAbove4G: %lx - %lx\n", Bridge->PMemAbove4G.Base,
>> Bridge->PMemAbove4G.Limit));
>> + DEBUG ((EFI_D_INFO, " Pci: %lx - %lx\n", Bridge->Pci.Base,
>> Bridge->Pci.Limit));
>>
>> //
>> // Make sure Mem and MemAbove4G apertures are valid
>> @@ -168,6 +169,7 @@ CreateRootBridge (
>> CopyMem (&RootBridge->Io, &Bridge->Io, sizeof (PCI_ROOT_BRIDGE_APERTURE));
>> CopyMem (&RootBridge->Mem, &Bridge->Mem, sizeof
>> (PCI_ROOT_BRIDGE_APERTURE));
>> CopyMem (&RootBridge->MemAbove4G, &Bridge->MemAbove4G, sizeof
>> (PCI_ROOT_BRIDGE_APERTURE));
>> + CopyMem (&RootBridge->Pci, &Bridge->Pci, sizeof
>> (PCI_ROOT_BRIDGE_APERTURE));
>>
>>
>> for (Index = TypeIo; Index < TypeMax; Index++) {
>> @@ -350,8 +352,8 @@ RootBridgeIoCheckParameter (
>> } else {
>> Address = PciRbAddr->Register;
>> }
>> - Base = 0;
>> - Limit = 0xFFF;
>> + Base = RootBridge->Pci.Base;
>> + Limit = RootBridge->Pci.Limit;
>> }
>>
>> if (Address < Base) {
>> --
>> 1.8.3.1
>>
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
