Re: [edk2] [PATCH] OvmfPkg: prevent 64-bit MMIO BAR degradation if there is no CSM

["David Woodhouse" <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 4139 bytes --]

On Thu, 2019-06-20 at 14:57 +0200, Laszlo Ersek wrote:
> (updating Ray's email)
> 
> On 06/20/19 00:19, David Woodhouse wrote:
> > 
> > > the driver is thoroughly commented. See especially the
> > > DriverInitialize() function. Can you determine which one(s) of those
> > > statements doesn't / don't hold any longer?
> > > 
> > > Or maybe IncompatiblePciDeviceSupportDxe works as before, but commit
> > > 065ae7d717f9 ("MdeModulePkg/PciBusDxe: make OPROM BAR degradation
> > > configurable", 2016-09-26) made a difference? (Adding Ard.)
> > > 
> > > I'm just guessing of course; a bisection could prove more effective.
> > 
> > I think I worked it out. The problem is that the nvme controller doesn't
> > have a ROM so it wasn't triggering the downgrade to 32-bit in the first
> > place.
> > 
> > By hacking IncompatiblePciDeviceSupportDxe to always return configuration
> > with 32+bit "granularity" I can boot. That does it for *all* devices, of
> > course... but I don't get the PCI class; only device/vendor IDs.
> 
> Doesn't this imply that we have a more generic problem in PciBusDxe?

Potentially so, yes. Although it's not clear the PciBusDxe itself
should be making such platform-dependent decisions. Calling out to
something like IncompatiblePciDeviceSupportDxe seems like the right
decision — but having an a priori default behaviour based on the
presence or otherwise of an OpROM doesn't. Just let the platform code
decide.


> AIUI, the current aperture degradation (for BAR allocation purposes) is
> meant to allow the device's own (specific) legacy option ROM to access
> the BARs. If the device has no option ROM, this particular goal falls
> away. (And OVMF's driver basically implements an override, in case the
> oprom does exist, but "legacy" is whole-sale unsupported by the platform.)
> 
> If the generic (device independent) CSM code -- *any* generic CSM in
> fact -- is expected to access BARs, then the logic in PciBusDxe is both
> too lax and too restrictive at the same time. It's too lax due to
> factors discussed previously (see the paragraph above, and commit
> 065ae7d717f9), and too restrictive because it misses the "CSM per se
> needs BAR access" case.

Right. AFAICT the criterion "has OpROM" was never really correct. It
should really have been "will be driven by legacy code".

Things that are natively supported by the CSM — including IDE, SATA,
VirtIO and NVMe 'disks' — all fall into that category whether they have
OpROMs or not. I think we've mostly just got away with it until now
because they haven't had 64-bit capable BARs.

> For working this around in OVMF, we'd have to change the OVMF driver's
> behavior from "force 64-bit, or keep silent", to "force 64-bit, or force
> 32-bit". This looks like a kludge that entirely supplants the PciBusDxe
> logic. So I'm not a fan of it.

Hm, I think the PciBusDxe logic *should* be "supplanted". It never made
sense for PciBusDxe to take a guess about what the platform might want,
then let the platform 'correct' it.

But of course it would be nice if PciBusDxe made a callout to platform
code and gave it all the information that might be needed — like the
PCI class, and the presence or otherwise of an OpROM. 

>  That said, if you can patch
> IncompatiblePciDeviceSupportDxe such that the change only affect the
> CSM_ENABLE build of OVMF observably, I guess I can live with it.

That's simple enough; we make CheckDevice() always return a
configuration, but with the granularity set to 32 if there's a CSM and
(as is currently the case) with it set to 64 if there's not.

Really, there should be a CSM call to say "do you want this device?",
and then the real trigger for degrading to 32-bit should be whether it
has an OpRom *or* that call returns true.

For now I've resorted to building with--pcd
gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size=0 — if we switch from
SeaBIOS to OVMF+SeaBIOS then I don't want random 32-bit guests breaking
because their BARs are suddenly unreachable, whether the device in
question is used by the BIOS or not.




[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]