Re: [edk2-devel] [PATCH] MdeModulePkg/PciBusDxe: Add feedback status for PciIoMap

["Laszlo Ersek" <lersek@redhat.com>]

On 1/26/24 18:56, Lendacky, Thomas via groups.io wrote:
> On 1/26/24 11:38, Tom Lendacky wrote:
>> +Min
>>
>> Adding Min to see if TDX is also experiencing issues around this
>> recent change.
>>
>> Thanks,
>> Tom
>>
>> On 1/26/24 11:21, Tom Lendacky wrote:
>>> On 1/22/24 00:47, Sheng Wei via groups.io wrote:
>>>> PciIoMap () need to feedback the status of
>>>> mIoMmuProtocol->SetAttribute () return value.
>>>>
>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4652
>>>
>>> I'm still investigating, but this commit breaks booting all types of
>>> SEV guests. Without this patch, there is a boot device mapping and
>>> the Grub menu is displayed. But with this patch, I receive:
>>>
>>> map: No mapping found.
>>> Press ESC in 1 seconds to skip startup.nsh or any other key to continue.
>>>
>>> and then drop to the shell prompt.
> 
> The IOMMU protocol is installed under OVMF when either SEV or TDX is
> active. The SetAttribute() function of this implementation has always
> returned EFI_UNSUPPORTED, which is now being passed pack to the caller
> of PciIoMap() and thus causing a failure.
> 
> Should the SetAttribute() function in OvmfPkg/IoMmuDxe/CcIoMmu.c return
> success by default?

I don't understand why the EDKII_IOMMU_PROTOCOL.SetAttribute() member
function exists in the first place. The documentation on
EDKII_IOMMU_SET_ATTRIBUTE says that having specified BusMasterRead
earlier as Operation for Map() corresponds to EDKII_IOMMU_ACCESS_READ,
BusMasterWrite to EDKII_IOMMU_ACCESS_WRITE, and BusMasterCommonBuffer to
EDKII_IOMMU_ACCESS_READ|EDKII_IOMMU_ACCESS_WRITE. This makes no sense to
me because (a) these settings carry zero new information related to the
earlier Map() call, and (b) the Map() call will have set up the
IOMMU/memory config already, based on Operation.

In PciIoMap() [MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c], there is first a
call to RootBridgeIoMap()
[MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c], which in turn
calls Map() on the IOMMU protocol. Thus, I don't see why PciBusDxe has
to do anything extra here, in the first place. I wouldn't call
EDKII_IOMMU_PROTOCOL.SetAttribute(); what's more, I'd argue that
EDKII_IOMMU_PROTOCOL.SetAttribute() itself is mostly useless. (Not sure
about "ACPI" devices; i.e. not PCI(e)).

Yet more confusion is that in the documentation of
EDKII_IOMMU_SET_ATTRIBUTE [MdeModulePkg/Include/Protocol/IoMmu.h], the
EFI_SUCCESS return status is described as "The IoMmuAccess is set for
the memory range specified by DeviceAddress and Length" -- but there
aren't even parameters called "DeviceAddress" and "Length"!

Then *even more* problems: EFI_INVALID_PARAMETER is supposed to be
returned for DeviceHandle being invalid, EFI_UNSUPPORTED is supposed to
be reutrned for DeviceHandle being unknown. So not only does the IOMMU
driver have to recognize handles it doesn't know about, it even has to
*distinguish* "plain unknown" from "invalid". That makes no sense at
all. The IOMMU protocol has no use for a DeviceHandle anywhere else in
the first place.

Either way, the right thing to do in OvmfPkg/IoMmuDxe should be:

- in IoMmuMap() [OvmfPkg/IoMmuDxe/CcIoMmu.c], we already save the
requested operation in MapInfo->Operation (note that this is not
EFI_PCI_IO_PROTOCOL_OPERATION, but EDKII_IOMMU_OPERATION: the former is
mapped to the latter in the root bridge protocol impl., IIRC, but the
latter also encodes 32-bit vs. 64-bit in the enum!)

- therefore in IoMmuSetAttribute() [OvmfPkg/IoMmuDxe/CcIoMmu.c], we
should just ignore DeviceHandle, cast Mapping back to MAP_INFO (we could
make an attempt to look it up first in "mMapInfos", but that's a total
waste of time: just don't pass in crap, and then you won't get undefined
behavior), and then compare MapInfo->Operation against IoMmuAccess.

For operations Read and Read64, accept EDKII_IOMMU_ACCESS_READ.

For operations Write and Write64, accept EDKII_IOMMU_ACCESS_WRITE.

For operations CommonBuffer and CommonBuffer64, accept
EDKII_IOMMU_ACCESS_READ|EDKII_IOMMU_ACCESS_WRITE.

Always accept 0 as IoMmuAccess, too [*].

"Accept" means do nothing, and return EFI_SUCCESS.

Reject anything else with EFI_INVALID_PARAMETER or EFI_UNSUPPORTED.

This would at least allow for a superficial consistency check, without
(a) incurring large performance penalty (such as list walking) and (b)
turning the SetAttribute() member function into a total joke. (Really,
IMO it should not even exist at the protocol specification level!)

[*] OK, you might want to know why we should accept 0 too. Here's why:
we have *another* mIoMmuProtocol->SetAttribute() call, namely in
PciIoUnmap() [MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c]. It passes in
IoMmuAccess=0.

Here's the sad bit: while this patch -- now commit 049695a0b1e2 --
propagates the return status of mIoMmuProtocol->SetAttribute() out of
PciIoMap(), the patch doesn't do the same for the other such call in
PciIoUnmap(). That seems like a bug (omission) itself, but once that one
gets fixed, we'll need to permit IoMmuAccess=0 too in OVMF's IOMMU driver.

I guess I could live with OVMF's IoMmuMap () doing nothing,
successfully, rather than doing nothing, unsuccessfully, *if*
EDKII_IOMMU_PROTOCOL.SetAttribute() had some public documentation rooted
in reality, *and* if PciBusDxe called that function with proper error
checking everywhere. Otherwise it's just another terrible, mis-specified
API, that platforms must route around.

Wow I didn't expect to be worked up this much about it, but here I am. I
dunno, seeing crap interfaces just makes me unreasonably irate.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114718): https://edk2.groups.io/g/devel/message/114718
Mute This Topic: https://groups.io/mt/103881889/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-