Re: [PATCH 1/3] ArmPkg: add driver to force 64-bit MMIO BARs to be allocated above 4 GB

[Laszlo Ersek <lersek@redhat.com>]

On 09/12/16 12:23, Leif Lindholm wrote:
> On Mon, Sep 12, 2016 at 11:01:17AM +0100, Ard Biesheuvel wrote:
>> By default, the generic PciBusDxe driver uses the presence of a ROM BAR
>> as a hint that all MMIO BARs should be allocated in the 32-bit region,
>> even the ones that could be allocated above 4 GB. The reason is that
>> such a ROM BAR may contain code that runs in the context of a legacy
>> BIOS (i.e., a CSM), and allocating the 64-bit BARs above 4 GB may put
>> them out of reach.
>>
>> Of course, none of this matters on ARM, and so we can unconditionally
>> override this decision. So take the OVMF implementation of the
>> IncompatiblePciDeviceSupportDxe driver, rip out the bits that care
>> about the presence of a legacy BIOS, and wire it up to the ArmPkg PCI
>> PCDs.
> 
> While I agree this solves a real problem, we're now at a point where
> we're adding multiple new implementations of a library

s/library/protocol/

> to give
> standards-compliant platforms the ability to circumvent legacy
> workarounds in core code.
> 
> Should this not be the other way around?

Yes, it should be.

... IMO, the specific degradation logic in the PCI Bus driver is
simplistic; for example, if it finds a PCI oprom for the device, it
could parse it to see if there's a legacy BIOS image (too) in that
oprom, before summarily degrading the resource.

But, implementing that in PciBusDxe now, or even eliminating the quirk
completely, would require all downstreams to make up for it in their
IncompatiblePciDeviceSupportDxe drivers. Getting shortcuts into core
code is easy (initally), getting them out is not so much.

So, I completely agree with you, especially that we're abusing this
protocol as a resource allocation hint -- as the patches themselves say,
there's nothing "incompatible" about the devices we handle. But,
changing the status quo at this stage looks really impractical.

> And if not, is that not a policy decision that suggests the workaround
> belongs as a core library anyway?

Based on the PI spec (1.4a, vol 5, "11.5 Incompatible PCI Device Support
Protocol"), the protocol should be provided by a platform driver.

Now, PciHostBridgeDriverDxe is also a platform driver. I think it's
different from this case because PciHostBridgeDriverDxe has so much
common / shared logic that it could be meaningfully split into a
PciHostBridgeLib class, and a common driver base (linking against the
lib class). For IncompatiblePciDeviceSupportDxe however, at least on the
level that we work with it, there's zero common logic to factor out.
(Okay, yes, you could centralize one or two PcdGet() calls, and perhaps
one of the PCDs could be moved to a central .DEC file.)

I don't like this situation either, but it's how ABI specs designed for
the proprietary world work in practice, I guess.

Thanks
Laszlo