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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 12 September 2016 at 13:29, Laszlo Ersek <lersek@redhat.com> wrote:
> 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.
>

True, but I have sent out a patch nonetheless. Let's at least have the
discussion.

> 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.
>