On 03/01/16 03:11, Ni, Ruiyu wrote:
> Laszlo,
>
> How about using a flag NoExtendedConfigSpace?
>
> Since for most cases the PCI CFG space limit is 0xFFF, so I think it’s
> better to choose a flag whose
>
> default value meets most of the cases. For a Boolean flag inside a
> structure when the structure
>
> is allocated using AllocateZeroPool, the flag value is FALSE.
Makes sense.
That gives rise to further questions though:
(1a) in the CreateRootBridge() function, do you want me to log the
setting in positive (ExtConfSpace: Yes / No), or in negative
(NoExtConfSpace: Yes / No).
(1b) If NoExtConfSpace: that word won't fit in the available whitespace.
Do you want me to re-align the other INFO messages?
(2) In the PCI_ROOT_BRIDGE structure definition
("MdeModulePkg/Include/Library/PciHostBridgeLib.h"),
"NoExtendedConfigSpace" will not fit without moving all the comments a
bit more to the right. Do you want me to do that, or should I use a
shorter variable name (NoExtConfigSpace for example)?
(3) In the internal structure (PCI_ROOT_BRIDGE_INSTANCE in
"MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h"), do you want me
to keep the exact same negative meaning as in the public header (i.e.,
call the field NoExtendedConfigSpace), or do you want me to call it
ExtendedConfigSpace and set it in CreateRootBridge() by negating the
PCI_ROOT_BRIDGE.NoExtendedConfigSpace field?
Thanks
Laszlo
>
>
>
> Regards,
>
> Ray
>
>
>
> *From:*Marcel Apfelbaum [mailto:[email protected]]
> *Sent:* Tuesday, March 1, 2016 1:30 AM
> *To:* Laszlo Ersek <[email protected]>; [email protected]
> *Cc:* Ni, Ruiyu <[email protected]>; Justen, Jordan L
> <[email protected]>
> *Subject:* Re: [PATCH v2] MdeModulePkg: PciHostBridgeDxe: don't assume
> extended config space
>
>
>
> On 02/29/2016 03:15 PM, Laszlo Ersek wrote:
>> The "pc" ("pc-i440fx-*") machine types of QEMU don't support extended
>> config space. Accordingly, OVMF will use the following library instances
>> in connection with the core PciHostBridgeDxe driver:
>>
>> BasePciSegmentLibPci [class: PciSegmentLib]
>> BasePciLibCf8 [class: PciLib]
>> BasePciCf8Lib [class: PciCf8Lib]
>>
>> Add a new field to the PCI_ROOT_BRIDGE structure so that
>> RootBridgeIoCheckParameter() can catch config space offsets above 0xFF on
>> such old (emulated) platforms.
>>
>> Cc: Ruiyu Ni <[email protected] <mailto:[email protected]>>
>> Cc: Jordan Justen <[email protected]
>> <mailto:[email protected]>>
>> Cc: Marcel Apfelbaum <[email protected] <mailto:[email protected]>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <[email protected] <mailto:[email protected]>>
>> ---
>>
>> Notes:
>> v2:
>> - replace the PCI config space "aperture" with a dedicated BOOLEAN flag
>> called ExtendedConfigSpace [Ray]
>> - simplify the commit message
>>
>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h | 1 +
>> MdeModulePkg/Include/Library/PciHostBridgeLib.h | 4 ++++
>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 4 +++-
>> 3 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
>> index b1e83f1c9089..f9839fed4628 100644
>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h
>> @@ -72,6 +72,7 @@ typedef struct {
>> PCI_ROOT_BRIDGE_APERTURE MemAbove4G;
>> PCI_ROOT_BRIDGE_APERTURE PMemAbove4G;
>> BOOLEAN DmaAbove4G;
>> + BOOLEAN ExtendedConfigSpace;
>> VOID *ConfigBuffer;
>> EFI_DEVICE_PATH_PROTOCOL *DevicePath;
>> CHAR16 *DevicePathStr;
>> diff --git a/MdeModulePkg/Include/Library/PciHostBridgeLib.h
>> b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
>> index b1dba0f754d7..fbf3be860c13 100644
>> --- a/MdeModulePkg/Include/Library/PciHostBridgeLib.h
>> +++ b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
>> @@ -34,6 +34,10 @@ typedef struct {
>> ///< and SetAttributes()
>> in EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL.
>> BOOLEAN DmaAbove4G; ///< DMA above 4GB memory.
>> ///< Set to TRUE when
>> root bridge supports DMA above 4GB memory.
>> + BOOLEAN ExtendedConfigSpace; ///< When TRUE, the root
>> bridge supports
>> + ///< Extended (4096-byte)
>> Configuration Space.
>> + ///< When FALSE, the root
>> bridge supports
>> + ///< 256-byte
>> Configuration Space only.
>> UINT64 AllocationAttributes; ///< Allocation
>> attributes.
>> ///< Refer to
>> EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM and
>> ///<
>> EFI_PCI_HOST_BRIDGE_MEM64_DECODE used by GetAllocAttributes()
>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>> index 332860eb3819..b0b1a2c18cca 100644
>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>> @@ -80,6 +80,7 @@ CreateRootBridge (
>> DEBUG ((EFI_D_INFO, "%s\n", DevicePathStr = ConvertDevicePathToText
>> (Bridge->DevicePath, FALSE, FALSE)));
>> DEBUG ((EFI_D_INFO, "Support/Attr: %lx / %lx\n", Bridge->Supports,
>> Bridge->Attributes));
>> DEBUG ((EFI_D_INFO, " DmaAbove4G: %s\n", Bridge->DmaAbove4G ? L"Yes" :
>> L"No"));
>> + DEBUG ((EFI_D_INFO, "ExtConfSpace: %s\n", Bridge->ExtendedConfigSpace ?
>> L"Yes" : L"No"));
>> DEBUG ((EFI_D_INFO, " AllocAttr: %lx (%s%s)\n",
>> Bridge->AllocationAttributes,
>> (Bridge->AllocationAttributes &
>> EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM) != 0 ? L"CombineMemPMem " : L"",
>> (Bridge->AllocationAttributes &
>> EFI_PCI_HOST_BRIDGE_MEM64_DECODE) != 0 ? L"Mem64Decode" : L""
>> @@ -155,6 +156,7 @@ CreateRootBridge (
>> RootBridge->Supports = Bridge->Supports;
>> RootBridge->Attributes = Bridge->Attributes;
>> RootBridge->DmaAbove4G = Bridge->DmaAbove4G;
>> + RootBridge->ExtendedConfigSpace = Bridge->ExtendedConfigSpace;
>> RootBridge->AllocationAttributes = Bridge->AllocationAttributes;
>> RootBridge->DevicePath = DuplicateDevicePath (Bridge->DevicePath);
>> RootBridge->DevicePathStr = DevicePathStr;
>> @@ -351,7 +353,7 @@ RootBridgeIoCheckParameter (
>> Address = PciRbAddr->Register;
>> }
>> Base = 0;
>> - Limit = 0xFFF;
>> + Limit = RootBridge->ExtendedConfigSpace ? 0xFFF : 0xFF;
>> }
>>
>> if (Address < Base) {
>>
>
>
> With my limited OVMF knowledge it looks OK to me.
>
> Reviewed-by: Marcel Apfelbaum <[email protected] <mailto:[email protected]>>
>
>
> Thanks,
> Marcel
>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
