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

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

[David Woodhouse]

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

On Thu, 2016-05-19 at 00:12 +0200, Laszlo Ersek wrote:
> According to edk2 commit
> 
>   "MdeModulePkg/PciBus: do not improperly degrade resource"
> 
> and to the EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL definition in the
> Platform Init 1.4a specification, a platform can provide such a protocol
> in order to influence the PCI resource allocation performed by the PCI Bus
> driver.
> 
> In particular it is possible instruct the PCI Bus driver, with a
> "wildcard" hint, to allocate the 64-bit MMIO BARs of a device in 64-bit
> address space, regardless of whether the device features an option ROM.
> 
> (By default, the PCI Bus driver considers an option ROM reason enough for
> allocating the 64-bit MMIO BARs in 32-bit address space. It cannot know if
> BDS will launch a legacy boot option, and under legacy boot, a legacy BIOS
> binary from a combined option ROM could be dispatched, and fail to access
> MMIO BARs in 64-bit address space.)
> 
> In platform code we can ascertain whether a CSM is present or not. If not,
> then legacy BIOS binaries in option ROMs can't be dispatched, hence the
> BAR degradation is detrimental, and we should prevent it. This is expected
> to conserve the 32-bit address space for 32-bit MMIO BARs.
> 
> The driver added in this patch could be simplified based on the following
> facts:
> 
> - In the Ia32 build, the 64-bit MMIO aperture is always zero-size, hence
>   the driver will exit immediately. Therefore the driver could be omitted
>   from the Ia32 build.
> 
> - In the Ia32X64 and X64 builds, the driver could be omitted if CSM_ENABLE
>   was defined (because in that case the degradation would be justified).
>   On the other hand, if CSM_ENABLE was undefined, then the driver could be
>   included, and it could provide the hint unconditionally (without looking
>   for the Legacy BIOS protocol).
> 
> These short-cuts are not taken because they would increase the differences
> between the OVMF DSC/FDF files. If we can manage without extreme
> complexity, we should use dynamic logic (vs. build time configuration),
> plus keep conditional compilation to a minimum.
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

This (commit 855743f717745) appears not to be working any more. I see
NVMe controllers' BARs being assigned above 4GiB where the CSM can't
reach them.



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

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

[Laszlo Ersek]

Hi David,

On 06/19/19 14:50, David Woodhouse wrote:
> On Thu, 2016-05-19 at 00:12 +0200, Laszlo Ersek wrote:
>> According to edk2 commit
>>
>>   "MdeModulePkg/PciBus: do not improperly degrade resource"
>>
>> and to the EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL definition in the
>> Platform Init 1.4a specification, a platform can provide such a protocol
>> in order to influence the PCI resource allocation performed by the PCI Bus
>> driver.
>>
>> In particular it is possible instruct the PCI Bus driver, with a
>> "wildcard" hint, to allocate the 64-bit MMIO BARs of a device in 64-bit
>> address space, regardless of whether the device features an option ROM.
>>
>> (By default, the PCI Bus driver considers an option ROM reason enough for
>> allocating the 64-bit MMIO BARs in 32-bit address space. It cannot know if
>> BDS will launch a legacy boot option, and under legacy boot, a legacy BIOS
>> binary from a combined option ROM could be dispatched, and fail to access
>> MMIO BARs in 64-bit address space.)
>>
>> In platform code we can ascertain whether a CSM is present or not. If not,
>> then legacy BIOS binaries in option ROMs can't be dispatched, hence the
>> BAR degradation is detrimental, and we should prevent it. This is expected
>> to conserve the 32-bit address space for 32-bit MMIO BARs.
>>
>> The driver added in this patch could be simplified based on the following
>> facts:
>>
>> - In the Ia32 build, the 64-bit MMIO aperture is always zero-size, hence
>>   the driver will exit immediately. Therefore the driver could be omitted
>>   from the Ia32 build.
>>
>> - In the Ia32X64 and X64 builds, the driver could be omitted if CSM_ENABLE
>>   was defined (because in that case the degradation would be justified).
>>   On the other hand, if CSM_ENABLE was undefined, then the driver could be
>>   included, and it could provide the hint unconditionally (without looking
>>   for the Legacy BIOS protocol).
>>
>> These short-cuts are not taken because they would increase the differences
>> between the OVMF DSC/FDF files. If we can manage without extreme
>> complexity, we should use dynamic logic (vs. build time configuration),
>> plus keep conditional compilation to a minimum.
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> 
> This (commit 855743f717745) appears not to be working any more. I see
> NVMe controllers' BARs being assigned above 4GiB where the CSM can't
> reach them.

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.

Thanks
Laszlo

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

[David Woodhouse]

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

-- 
dwmw2

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

[Laszlo Ersek]

(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?

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.

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

Thanks
Laszlo

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

[David Woodhouse]

[-- 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 --]