Re: [PATCH] MdeModulePkg/PciBusDxe: make OPROM BAR degradation X64 specific

[Andrew Fish <afish@apple.com>]

> On Sep 15, 2016, at 8:28 AM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> 
> On Thu, Sep 15, 2016 at 03:54:56PM +0100, Ard Biesheuvel wrote:
>> The 'universal' PCI bus driver in MdeModulePkg contains a quirk to
>> degrade 64-bit PCI MMIO BARs to 32-bit in the presence of an option
>> ROM on the same PCI controller.
>> 
>> This quirk is highly specific to not just the X64 architecture in general,
>> but to the PC platform in particular, given that only X64 platforms that
>> require legacy PC BIOS compatibility require it. However, making the
>> quirk dependent on the presence of the legacy BIOS protocol met with
>> resistance, due to the fact that it introduces a dependency on the
>> IntelFrameworkModulePkg package.
>> 
>> So instead, make the quirk dependent on whether we are compiling for the
>> X64 architecture, by putting the #ifdef MDE_CPU_X64/#endif preprocessor
>> directives around it.
>> 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> I'm happy with the change, but it may be worth putting a comment in

Do we have to use an #ifdef? Seems like it should be a PCD. Is this for legacy BIOS Option ROM support, or for existing EFI ROMs?

Thanks,

Andrew Fish

> the code to the extent that:
> ---
> In order to reliably support legacy Option ROMs, which may not be
> 64-bit safe, X64 needs to ensure PCI MMIO BARs are forced down to use
> only the lower 32 bits.
> Other architectures, or indeed X64 without legacy option ROM support,
> can safely leave these in normal operation mode (and may require so in
> order to function).
> ---
> 
> For what it's worth:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> 
>> ---
>> 
>> Since nobody proposed any alternatives to the solution I proposed,
>> let's just make the quirk disappear on architectures that have no
>> use for it.
>> 
>> Note that forcing non-X64 architectures to install a driver that
>> produces the IncompatiblePciDevice protocol only to make the PCI
>> bus driver behave normally (and which, incidentally, can only be
>> produced once per platform, making it difficult to provide a default
>> implementation that can be widely reused) is not acceptable IMO.
>> 
>> If anything, this should require an IncompatiblePlatform protocol
>> that informs the PCI bus driver that a special quirk is required,
>> but I am aware that this is difficult to accomplish without breaking
>> backward compatibility. Hence this compromise.
>> 
>> MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 2 ++
>> 1 file changed, 2 insertions(+)
>> 
>> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
>> index b0632d53b82b..b4771a822d65 100644
>> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
>> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
>> @@ -1052,6 +1052,7 @@ DegradeResource (
>>   IN PCI_RESOURCE_NODE *PMem64Node
>>   )
>> {
>> +#ifdef MDE_CPU_X64
>>   PCI_IO_DEVICE        *PciIoDevice;
>>   LIST_ENTRY           *ChildDeviceLink;
>>   LIST_ENTRY           *ChildNodeLink;
>> @@ -1101,6 +1102,7 @@ DegradeResource (
>>     }
>>     ChildDeviceLink = ChildDeviceLink->ForwardLink;
>>   }
>> +#endif
>> 
>>   //
>>   // If firmware is in 32-bit mode,
>> -- 
>> 2.7.4
>> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel