From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id E3CEB941CC9 for ; Tue, 30 Jan 2024 16:37:09 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=POfv2SCeQCSX4xAKcs4ObgyMZukko2ygnJia7SDfl/A=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1706632628; v=1; b=ZZhqk36WFqLFocpb7rxS57JMWFmCSVIhiNI8wJ0YeWLbiGkFUqLRjj3jFLX3t/yDMU6N3vb5 8HzV/sM0+0EpSSK/UY8xKEa3Fp7zo5nFugDta8pgAelb7tBn6ehNTsukb+/ECITrio5wi6XV6i4 coIy+NiBFjGOnYq0hVS2vjN8= X-Received: by 127.0.0.2 with SMTP id ZESwYY7687511xKZ8FMpdLme; Tue, 30 Jan 2024 08:37:08 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web10.641.1706632627648941665 for ; Tue, 30 Jan 2024 08:37:07 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-50-obIrRfiWMcq5W3w7qMwhGw-1; Tue, 30 Jan 2024 11:37:05 -0500 X-MC-Unique: obIrRfiWMcq5W3w7qMwhGw-1 X-Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id AF07E82A69D; Tue, 30 Jan 2024 16:37:04 +0000 (UTC) X-Received: from [10.39.192.28] (unknown [10.39.192.28]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 4880E483EC1; Tue, 30 Jan 2024 16:37:03 +0000 (UTC) Message-ID: <73bc4894-6019-d573-2759-2231e07960fa@redhat.com> Date: Tue, 30 Jan 2024 17:37:02 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PciBusDxe: Add feedback status for PciIoMap To: Tom Lendacky , devel@edk2.groups.io, w.sheng@intel.com, "Xu, Min M" Cc: Ray Ni , Huang Jenny , Chiang Chris References: <20240122064706.2059-1-w.sheng@intel.com> <686c8bd7-c0de-9429-1577-f436c347c314@amd.com> <451fee11-e982-802f-c30b-1b90b2251d70@redhat.com> <3474f3a1-f90e-4e61-a7fc-5c659ce150c6@amd.com> From: "Laszlo Ersek" In-Reply-To: <3474f3a1-f90e-4e61-a7fc-5c659ce150c6@amd.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.9 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: r37dhEidS2Rk3R7U0Oaqus7hx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=ZZhqk36W; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io 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 > > 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 > --- >  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 (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] -=-=-=-=-=-=-=-=-=-=-=-