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 B5012AC1754 for ; Mon, 29 Jan 2024 17:20:33 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=VgGh8hVOKELSDemdvhP7Sz5scTRN4UFmqJd4pctc8Mg=; 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=1706548832; v=1; b=L9C83rVQfVWjqTAr9p18WJzyRRFzv94VagrXA0aaDqyVtRcZZ1YvyIytNBK/xVBG9k9wNeRk Wnw5iCfAlHQTriJnfE9wxYnDZHs0w1V4VhaYlv84nmTtY5I0J9EGFN28kzgq3d4LJl/KXyl5gbt Kv5osXxzgymjpfvS0L96ARVc= X-Received: by 127.0.0.2 with SMTP id 2zHlYY7687511xVGAB06Rtfo; Mon, 29 Jan 2024 09:20:32 -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.425.1706548831609350670 for ; Mon, 29 Jan 2024 09:20:31 -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-212-4QAv0z1oNHmDQwuHbWCE8A-1; Mon, 29 Jan 2024 12:20:26 -0500 X-MC-Unique: 4QAv0z1oNHmDQwuHbWCE8A-1 X-Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (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 55152106D022; Mon, 29 Jan 2024 17:20:25 +0000 (UTC) X-Received: from [10.39.192.97] (unknown [10.39.192.97]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 305B02026D66; Mon, 29 Jan 2024 17:20:24 +0000 (UTC) Message-ID: <451fee11-e982-802f-c30b-1b90b2251d70@redhat.com> Date: Mon, 29 Jan 2024 18:20:22 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PciBusDxe: Add feedback status for PciIoMap To: devel@edk2.groups.io, thomas.lendacky@amd.com, 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> From: "Laszlo Ersek" In-Reply-To: <686c8bd7-c0de-9429-1577-f436c347c314@amd.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.4 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: 3QxVmhG1cjwT2s2VbGYNNJKSx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=L9C83rVQ; 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/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=3D4652 >>> >>> 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. >=20 > 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. >=20 > 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=3D0. 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=3D0 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 -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-