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

["Lendacky, Thomas via groups.io" <thomas.lendacky=amd.com@groups.io>]

On 1/29/24 11:20, Laszlo Ersek wrote:
> 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.

Laszlo, if you mean something like this, I'll submit it as a proper patch:


OvmfPkg/IoMmuDxe: Provide an implementation for SetAttribute

From: Tom Lendacky <thomas.lendacky@amd.com>

A recent change to the PciIoMap() function now propagates the return code
from the IoMmu protocol SetAttribute() operation. The implementation of
this operation in OvmfPkg/IoMmuDxe/CcIoMmu.c returns EFI_UNSUPPORTED,
resulting in a failure to boot the guest.

Provide an implementation for SetAttribute() that validates that the IoMmu
access method being requested against the IoMmu mapping operation.

Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
  OvmfPkg/IoMmuDxe/CcIoMmu.c |   50 +++++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/IoMmuDxe/CcIoMmu.c b/OvmfPkg/IoMmuDxe/CcIoMmu.c
index b83a9690062b..8bd95bfc6b90 100644
--- a/OvmfPkg/IoMmuDxe/CcIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/CcIoMmu.c
@@ -751,7 +751,55 @@ IoMmuSetAttribute (
    IN UINT64                IoMmuAccess
    )
  {
-  return EFI_UNSUPPORTED;
+  MAP_INFO    *MapInfo;
+  EFI_STATUS  Status;
+
+  DEBUG ((DEBUG_VERBOSE, "%a: Mapping=0x%p Access=%ld\n", __func__, Mapping, IoMmuAccess));
+
+  if (Mapping == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  Status = EFI_SUCCESS;
+
+  //
+  // An IoMmuAccess value of 0 is always accepted, validate any non-zero value.
+  //
+  if (IoMmuAccess != 0) {
+    MapInfo = (MAP_INFO *)Mapping;
+
+    //
+    // The mapping operation already implied the access mode. Validate that
+    // the supplied access mode matches operation access mode.
+    //
+    switch (MapInfo->Operation) {
+      case EdkiiIoMmuOperationBusMasterRead:
+      case EdkiiIoMmuOperationBusMasterRead64:
+        if (IoMmuAccess != EDKII_IOMMU_ACCESS_READ) {
+          Status = EFI_INVALID_PARAMETER;
+        }
+        break;
+
+      case EdkiiIoMmuOperationBusMasterWrite:
+      case EdkiiIoMmuOperationBusMasterWrite64:
+        if (IoMmuAccess != EDKII_IOMMU_ACCESS_WRITE) {
+          Status = EFI_INVALID_PARAMETER;
+        }
+        break;
+
+      case EdkiiIoMmuOperationBusMasterCommonBuffer:
+      case EdkiiIoMmuOperationBusMasterCommonBuffer64:
+        if (IoMmuAccess != (EDKII_IOMMU_ACCESS_READ | EDKII_IOMMU_ACCESS_WRITE)) {
+          Status = EFI_INVALID_PARAMETER;
+        }
+        break;
+
+      default:
+        Status = EFI_UNSUPPORTED;
+    }
+  }
+
+  return Status;
  }
  
  EDKII_IOMMU_PROTOCOL  mIoMmu = {


> 
> 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 (#114738): https://edk2.groups.io/g/devel/message/114738
Mute This Topic: https://groups.io/mt/103881889/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-