Message ID | 1473685561-1418-1-git-send-email-ard.biesheuvel@linaro.org |
---|---|
State | New |
Headers | show |
On 12 September 2016 at 14:15, Yao, Jiewen <jiewen.yao@intel.com> wrote: > HI Ard > We should not let MdeModulePkg depend on IntelFrameworkPkg. > You patch violates the dependency rule. > I suggest we figure out other solution to resolve the problem. > Yes, please. And please keep us informed about any solutions you come up with. Thanks, Ard. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 12 September 2016 at 14:41, Ni, Ruiyu <ruiyu.ni@intel.com> wrote: > You could use IncompatiblePciDevice protocol. If you have no clue I will > give you more information my tomorrow. > That works around the problem, but does not solve it. MdeModulePkg is supposed to be generic/universal, and still, it contains a Intel/CSM specific hack to degrade 64-bit BARs of any device that has an option ROM. This is platform specific policy that does not belong in generic code, and by the same reasoning that Jiewen argues that MdeModulePkg should not depend on IntelFrameworkModulePkg, it should not implement legacy BIOS hacks. So what would you propose to factor out this functionality? Perhaps a PciPolicyLib, whose Null implementation does not contain the hack. Or a feature PCD that can be set to disable this behavior? In any case, the requirement to implement the IncompatiblePciDevice protocol to get normal resource allocation behavior is not the best way to deal with this IMO Thanks, Ard. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 09/12/16 15:16, Ard Biesheuvel wrote: > On 12 September 2016 at 14:15, Yao, Jiewen <jiewen.yao@intel.com> wrote: >> HI Ard >> We should not let MdeModulePkg depend on IntelFrameworkPkg. >> You patch violates the dependency rule. >> I suggest we figure out other solution to resolve the problem. >> > > Yes, please. And please keep us informed about any solutions you come up with. * One idea is to parse the PCI expansion ROM in order to see what image types are contained within. If there is no (Code type == 0x00) image in the oprom, then the oprom is useless for legacy boot anyway, so it shouldn't trigger degradation. Unfortunately, this wouldn't help a lot in practice, since it's surely going to be years before hw vendors migrate to pure UEFI oproms on their graphics and network cards. :( * Another idea is to check a dynamic PCD that the platform can set. New PCDs are frowned upon in MdeModulePkg however, so I don't expect this to be a popular fix. Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 13 September 2016 at 06:46, Ni, Ruiyu <ruiyu.ni@intel.com> wrote: > > > Regards, > Ray > >>-----Original Message----- >>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek >>Sent: Monday, September 12, 2016 9:49 PM >>To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Yao, Jiewen <jiewen.yao@intel.com> >>Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Tian, Feng <feng.tian@intel.com>; edk2-devel@lists.01.org <edk2-devel@ml01.01.org>; >>leif.lindholm@linaro.org; Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com> >>Subject: Re: [edk2] [PATCH] MdeModulePkg/PciBusDxe: make BAR degradation dependent on OPROM presence >> >>On 09/12/16 15:16, Ard Biesheuvel wrote: >>> On 12 September 2016 at 14:15, Yao, Jiewen <jiewen.yao@intel.com> wrote: >>>> HI Ard >>>> We should not let MdeModulePkg depend on IntelFrameworkPkg. >>>> You patch violates the dependency rule. >>>> I suggest we figure out other solution to resolve the problem. >>>> >>> >>> Yes, please. And please keep us informed about any solutions you come up with. >> >>* One idea is to parse the PCI expansion ROM in order to see what image >>types are contained within. If there is no (Code type == 0x00) image in >>the oprom, then the oprom is useless for legacy boot anyway, so it >>shouldn't trigger degradation. >> >>Unfortunately, this wouldn't help a lot in practice, since it's surely >>going to be years before hw vendors migrate to pure UEFI oproms on their >>graphics and network cards. :( > Yes the first idea doesn't work because it cannot solve all the problems: some > cards may still contain legacy option ROM. The resource degrade happens > before PciBus knows which option rom to run by platform. And we even met case > the card only contains legacy option rom but platform just don't want to dispatch > the legacy rom. > >>* Another idea is to check a dynamic PCD that the platform can set. New >>PCDs are frowned upon in MdeModulePkg however, so I don't expect this to >>be a popular fix. > > I agree with you. > The requirement of dynamic PCD actually indicates we have a missing interface > between platform and PciBus core driver. > I personally don't like dynamic PCD very much. It's equivalent to protocol. Then > why should we use PCD instead of protocol? To avoid changing spec?? > > The usage of PciIncompatibleDevice protocol is the solution we came up. > It's also the currently recommended way to solve this type of issue. > ECR 1529 introduced in PI 1.4A was initiated by this issue. > We are all aware that the PciIncompatibleDevice protocol can be used to work around this. But this means that, while this issue only exists on X64, all architectures need to install additional protocols and rely on runtime processing to disable a feature they did not want in the first place. Is the practice of degrading 64-bit MMIO BARs for option ROMs even in the specs? So how about a PCD feature flag? Or even simply a '#ifdef MDE_CPU_X64' around the block that degrades the 64-bit MMIO BARs if an option ROM is detected? Ideally, we should implement PciIncompatibleDevice protocol the other way around, and add a default implementation to IntelFrameworkModulePkg to performs the BAR degradation on platforms with a legacy BIOS and option ROMs -- Ard. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Adding Andrew, Mike, On Tue, Sep 13, 2016 at 08:43:39AM +0100, Ard Biesheuvel wrote: > On 13 September 2016 at 06:46, Ni, Ruiyu <ruiyu.ni@intel.com> wrote: > >>* Another idea is to check a dynamic PCD that the platform can set. New > >>PCDs are frowned upon in MdeModulePkg however, so I don't expect this to > >>be a popular fix. > > > > I agree with you. > > The requirement of dynamic PCD actually indicates we have a > > missing interface between platform and PciBus core driver. > > I personally don't like dynamic PCD very much. It's equivalent to > > protocol. Then why should we use PCD instead of protocol? To avoid > > changing spec?? > > > > The usage of PciIncompatibleDevice protocol is the solution we came up. > > It's also the currently recommended way to solve this type of issue. > > ECR 1529 introduced in PI 1.4A was initiated by this issue. > > We are all aware that the PciIncompatibleDevice protocol can be used > to work around this. But this means that, while this issue only exists > on X64, all architectures need to install additional protocols and > rely on runtime processing to disable a feature they did not want in > the first place. Is the practice of degrading 64-bit MMIO BARs for > option ROMs even in the specs? > > So how about a PCD feature flag? Or even simply a '#ifdef MDE_CPU_X64' > around the block that degrades the 64-bit MMIO BARs if an option ROM > is detected? Ideally, we should implement PciIncompatibleDevice > protocol the other way around, and add a default implementation to > IntelFrameworkModulePkg to performs the BAR degradation on platforms > with a legacy BIOS and option ROMs Yes, the situation of requiring an implementation of PciIncompatibleDevice to support an actually compatible device, in order to not require it for actually incompatible platforms seems at the very least somewhat semantically dubious to me. By order of my preference, I would say we could: - Invert the use of PciIncompatibleDevice with a default implementation for IntelFrameworkModulePkg - #ifdef MDE_CPU_X64 - Add a dynamic Pcd - Retain the semantically incorrect use of PciIncompatibleDevice, but provide a single implementation reusable across OvmfPkg and all !X64 platforms. Regards, Leif _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c index a463bea80f3d..857f3e11b6bd 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c @@ -49,6 +49,39 @@ GLOBAL_REMOVE_IF_UNREFERENCED EFI_PCI_HOTPLUG_REQUEST_PROTOCOL mPciHotPlugReques }; /** + Legacy BIOS installed callback + + @param[in] Event Event whose notification function is being invoked. + @param[in] Context Pointer to the notification function's context. + +**/ +STATIC +VOID +EFIAPI +LegacyBiosInstalledCallBack ( + IN EFI_EVENT Event, + IN VOID *Context + ) +{ + EFI_STATUS Status; + EFI_LEGACY_BIOS_PROTOCOL *LegacyBios; + + Status = gBS->LocateProtocol (&gEfiLegacyBiosProtocolGuid, + NULL /* Registration */, (VOID **)&LegacyBios); + if (EFI_ERROR (Status)) { + return; + } + + mLegacyBiosInstalled = TRUE; + + // + // Close the event and deregister this callback. + // + Status = gBS->CloseEvent (Event); + ASSERT_EFI_ERROR (Status); +} + +/** The Entry Point for PCI Bus module. The user code starts with this function. Installs driver module protocols and. Creates virtual device handles for ConIn, @@ -72,6 +105,7 @@ PciBusEntryPoint ( { EFI_STATUS Status; EFI_HANDLE Handle; + VOID *Registration; // // Initializes PCI devices pool @@ -91,6 +125,14 @@ PciBusEntryPoint ( ); ASSERT_EFI_ERROR (Status); + EfiCreateProtocolNotifyEvent ( + &gEfiLegacyBiosProtocolGuid, + TPL_CALLBACK, + LegacyBiosInstalledCallBack, + NULL, + &Registration + ); + if (FeaturePcdGet (PcdPciBusHotplugDeviceSupport)) { // // If Hot Plug is supported, install EFI PCI Hot Plug Request protocol. diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h index b12d7ec5032f..2bf5695476a1 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h @@ -321,6 +321,7 @@ extern EFI_PCI_PLATFORM_PROTOCOL *gPciPlatformProtocol; extern EFI_PCI_OVERRIDE_PROTOCOL *gPciOverrideProtocol; extern BOOLEAN mReserveIsaAliases; extern BOOLEAN mReserveVgaAliases; +extern BOOLEAN mLegacyBiosInstalled; /** Macro that checks whether device is a GFX device. diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf index 330ccc8cbffc..b843ccc49934 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf @@ -66,6 +66,7 @@ [Sources] [Packages] MdePkg/MdePkg.dec MdeModulePkg/MdeModulePkg.dec + IntelFrameworkPkg/IntelFrameworkPkg.dec [LibraryClasses] PcdLib @@ -95,6 +96,7 @@ [Protocols] gEfiPciRootBridgeIoProtocolGuid ## TO_START gEfiIncompatiblePciDeviceSupportProtocolGuid ## SOMETIMES_CONSUMES gEfiLoadFile2ProtocolGuid ## SOMETIMES_PRODUCES + gEfiLegacyBiosProtocolGuid ## SOMETIMES_CONSUMES [FeaturePcd] gEfiMdeModulePkgTokenSpaceGuid.PcdPciBusHotplugDeviceSupport ## CONSUMES diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c index b0632d53b82b..6637625b210d 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c @@ -17,9 +17,10 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. // // The default policy for the PCI bus driver is NOT to reserve I/O ranges for both ISA aliases and VGA aliases. // -BOOLEAN mReserveIsaAliases = FALSE; -BOOLEAN mReserveVgaAliases = FALSE; -BOOLEAN mPolicyDetermined = FALSE; +BOOLEAN mReserveIsaAliases = FALSE; +BOOLEAN mReserveVgaAliases = FALSE; +BOOLEAN mPolicyDetermined = FALSE; +BOOLEAN mLegacyBiosInstalled = FALSE; /** The function is used to skip VGA range. @@ -1058,48 +1059,50 @@ DegradeResource ( LIST_ENTRY *NextChildNodeLink; PCI_RESOURCE_NODE *ResourceNode; - // - // If any child device has both option ROM and 64-bit BAR, degrade its PMEM64/MEM64 - // requests in case that if a legacy option ROM image can not access 64-bit resources. - // - ChildDeviceLink = Bridge->ChildList.ForwardLink; - while (ChildDeviceLink != NULL && ChildDeviceLink != &Bridge->ChildList) { - PciIoDevice = PCI_IO_DEVICE_FROM_LINK (ChildDeviceLink); - if (PciIoDevice->RomSize != 0) { - if (!IsListEmpty (&Mem64Node->ChildList)) { - ChildNodeLink = Mem64Node->ChildList.ForwardLink; - while (ChildNodeLink != &Mem64Node->ChildList) { - ResourceNode = RESOURCE_NODE_FROM_LINK (ChildNodeLink); - NextChildNodeLink = ChildNodeLink->ForwardLink; - - if ((ResourceNode->PciDev == PciIoDevice) && - (ResourceNode->Virtual || !PciIoDevice->PciBar[ResourceNode->Bar].BarTypeFixed) - ) { - RemoveEntryList (ChildNodeLink); - InsertResourceNode (Mem32Node, ResourceNode); + if (mLegacyBiosInstalled) { + // + // If any child device has both option ROM and 64-bit BAR, degrade its PMEM64/MEM64 + // requests in case that if a legacy option ROM image can not access 64-bit resources. + // + ChildDeviceLink = Bridge->ChildList.ForwardLink; + while (ChildDeviceLink != NULL && ChildDeviceLink != &Bridge->ChildList) { + PciIoDevice = PCI_IO_DEVICE_FROM_LINK (ChildDeviceLink); + if (PciIoDevice->RomSize != 0) { + if (!IsListEmpty (&Mem64Node->ChildList)) { + ChildNodeLink = Mem64Node->ChildList.ForwardLink; + while (ChildNodeLink != &Mem64Node->ChildList) { + ResourceNode = RESOURCE_NODE_FROM_LINK (ChildNodeLink); + NextChildNodeLink = ChildNodeLink->ForwardLink; + + if ((ResourceNode->PciDev == PciIoDevice) && + (ResourceNode->Virtual || !PciIoDevice->PciBar[ResourceNode->Bar].BarTypeFixed) + ) { + RemoveEntryList (ChildNodeLink); + InsertResourceNode (Mem32Node, ResourceNode); + } + ChildNodeLink = NextChildNodeLink; } - ChildNodeLink = NextChildNodeLink; - } - } + } - if (!IsListEmpty (&PMem64Node->ChildList)) { - ChildNodeLink = PMem64Node->ChildList.ForwardLink; - while (ChildNodeLink != &PMem64Node->ChildList) { - ResourceNode = RESOURCE_NODE_FROM_LINK (ChildNodeLink); - NextChildNodeLink = ChildNodeLink->ForwardLink; - - if ((ResourceNode->PciDev == PciIoDevice) && - (ResourceNode->Virtual || !PciIoDevice->PciBar[ResourceNode->Bar].BarTypeFixed) - ) { - RemoveEntryList (ChildNodeLink); - InsertResourceNode (PMem32Node, ResourceNode); + if (!IsListEmpty (&PMem64Node->ChildList)) { + ChildNodeLink = PMem64Node->ChildList.ForwardLink; + while (ChildNodeLink != &PMem64Node->ChildList) { + ResourceNode = RESOURCE_NODE_FROM_LINK (ChildNodeLink); + NextChildNodeLink = ChildNodeLink->ForwardLink; + + if ((ResourceNode->PciDev == PciIoDevice) && + (ResourceNode->Virtual || !PciIoDevice->PciBar[ResourceNode->Bar].BarTypeFixed) + ) { + RemoveEntryList (ChildNodeLink); + InsertResourceNode (PMem32Node, ResourceNode); + } + ChildNodeLink = NextChildNodeLink; } - ChildNodeLink = NextChildNodeLink; - } - } + } + } + ChildDeviceLink = ChildDeviceLink->ForwardLink; } - ChildDeviceLink = ChildDeviceLink->ForwardLink; } //
The practice of unconditionally degrading 64-bit PCI MMIO BARs to 32-bit if the device in question happens to have an option ROM is based on platform constraints, not architectural constraints, and really only makes sense on Intel platforms that contain a CSM implementation. So let's copy the OVMF code that checks for the presence of the legacy BIOS protocol (&gEfiLegacyBiosProtocolGuid), and only perform the BAR degradation if this protocol is installed. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c | 42 ++++++++++ MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h | 1 + MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf | 2 + MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 83 ++++++++++---------- 4 files changed, 88 insertions(+), 40 deletions(-) -- 2.7.4 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel