| Message ID | 1456751755-15294-1-git-send-email-lersek@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
On 02/29/16 14:15, 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 <ruiyu.ni@intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Marcel Apfelbaum <marcel@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > > 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) { > I'll note that in <https://github.com/tianocore/edk2/issues/32> we're forming a plan to use ECAM on Q35. So in the longer term OVMF will not set the ExtendedConfigSpace field to *constant* FALSE -- in the longer term, on Q35 it will be set to TRUE. (The underlying PciLib instance will switch between 0xCF8 vs. MMCONFIG dynamically.) Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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:marcel.apfelbaum@gmail.com] > *Sent:* Tuesday, March 1, 2016 1:30 AM > *To:* Laszlo Ersek <lersek@redhat.com>; edk2-devel@ml01.01.org > *Cc:* Ni, Ruiyu <ruiyu.ni@intel.com>; Justen, Jordan L > <jordan.l.justen@intel.com> > *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 <ruiyu.ni@intel.com <mailto:ruiyu.ni@intel.com>> >> Cc: Jordan Justen <jordan.l.justen@intel.com <mailto:jordan.l.justen@intel.com>> >> Cc: Marcel Apfelbaum <marcel@redhat.com <mailto:marcel@redhat.com>> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>> >> --- >> >> 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 <marcel@redhat.com <mailto:marcel@redhat.com>> > > > Thanks, > Marcel > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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) {
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 <ruiyu.ni@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Marcel Apfelbaum <marcel@redhat.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- 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(-) -- 1.8.3.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel