Laszlo, I prefer to use the longer name and use the exactly same name in internal structure as well. It can help other developers or perhaps myself easily read the code in future.
Regards, Ray >-----Original Message----- >From: edk2-devel [mailto:[email protected]] On Behalf Of Laszlo >Ersek >Sent: Tuesday, March 1, 2016 4:48 PM >To: Ni, Ruiyu <[email protected]>; [email protected]; [email protected] >Cc: Kinney, Michael D <[email protected]>; Justen, Jordan L ><[email protected]> >Subject: Re: [edk2] [PATCH v2] MdeModulePkg: PciHostBridgeDxe: don't assume >extended config space > >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). Negative as well please. Otherwise, it's a little bit hard to read the code. > >(1b) If NoExtConfSpace: that word won't fit in the available whitespace. >Do you want me to re-align the other INFO messages? yes please adjust the code for better alignment. > >(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)? I prefer to use NoExtendedConfigSpace. So yes please move all other comments a bit more to right. > >(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? Please use the exactly same name: NoExtendedConfigSpace. The benifite is for easy reading in future. > >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 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
