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

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

On 1/29/24 20:30, Tom Lendacky wrote:
> 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));

(1) %lu is better for formatting UINT64 (%ld is for INT64)

> +
> +  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 = {

With (1) updated:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

(feel free to include that in the proper posting at once)

Thanks!
Laszlo

> 
> 
>>
>> 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 (#114801): https://edk2.groups.io/g/devel/message/114801
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]
-=-=-=-=-=-=-=-=-=-=-=-