From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web11.165.1673373335121587897 for ; Tue, 10 Jan 2023 09:55:35 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=RT/wvOaI; spf=pass (domain: redhat.com, ip: 170.10.129.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1673373334; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=2p8AN8B9FZsgOZKDo8gTUpYwI7dkdE4yZhBq1b1i3qk=; b=RT/wvOaIW3q8fDtNXx9cqumbXgbaCH2fueu+g06UVOVN1KwWQ0Jnvf0bkMx8sj6KnGyP9S aBjQqhyHdmlev08+/Vn2hQ+j0np22lriMJNYB4Wcbec6xaUQuBQAAFFs4Z0zjzi9vv+Nyo eHpTeDLXFNuvFrU1OtxCPWEunO4SQAc= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-477-O8YloIPwO42RY19jGBOg-A-1; Tue, 10 Jan 2023 12:55:33 -0500 X-MC-Unique: O8YloIPwO42RY19jGBOg-A-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 8F5AA3C38FF8; Tue, 10 Jan 2023 17:55:32 +0000 (UTC) Received: from [10.39.192.22] (unknown [10.39.192.22]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 27FCE140EBF4; Tue, 10 Jan 2023 17:55:30 +0000 (UTC) Message-ID: Date: Tue, 10 Jan 2023 18:55:29 +0100 MIME-Version: 1.0 Subject: Re: [PATCH v2 4/4] OvmfPkg/PlatformInitLib: Add PlatformReservationConflictCB To: Gerd Hoffmann , devel@edk2.groups.io Cc: Pawel Polawski , Jiewen Yao , Oliver Steffen , Jordan Justen , Ard Biesheuvel References: <20230110082123.159521-1-kraxel@redhat.com> <20230110082123.159521-5-kraxel@redhat.com> From: "Laszlo Ersek" In-Reply-To: <20230110082123.159521-5-kraxel@redhat.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.7 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 1/10/23 09:21, Gerd Hoffmann wrote: > Add PlatformReservationConflictCB() callback function for use with > PlatformScanE820(). It checks whenever the 64bit PCI MMIO window > overlaps with a reservation from qemu. If so move down the MMIO window > to resolve the conflict. > > This happens on (virtal) AMD machines with 1TB address space, (1) s/virtal/virtual/ > because the AMD IOMMU uses an address window just below 1GB. (2) s/1GB/1TB/? > > Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=4251 > Signed-off-by: Gerd Hoffmann > --- > OvmfPkg/Library/PlatformInitLib/MemDetect.c | 41 +++++++++++++++++++++ > 1 file changed, 41 insertions(+) > > diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c > index 83a219581a1b..f12d48cad755 100644 > --- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c > +++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c > @@ -202,6 +202,46 @@ PlatformAddHobCB ( > } > } > > +/** > + Check whenever the 64bit PCI MMIO window overlaps with a reservation > + from qemu. If so move down the MMIO window to resolve the conflict. > + > + This happens on (virtal) AMD machines with 1TB address space, > + because the AMD IOMMU uses an address window just below 1GB. (3) Same two typos, I think, as in the commit message. > +**/ > +VOID > +PlatformReservationConflictCB ( (4) should be STATIC etc, maybe use E820 in the name rathern than Platform, etc... up to you. > + IN EFI_E820_ENTRY64 *E820Entry, > + IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob > + ) > +{ > + UINT64 IntersectionBase = MAX ( > + E820Entry->BaseAddr, > + PlatformInfoHob->PcdPciMmio64Base > + ); > + UINT64 IntersectionEnd = MIN ( > + E820Entry->BaseAddr + E820Entry->Length, > + PlatformInfoHob->PcdPciMmio64Base + > + PlatformInfoHob->PcdPciMmio64Size > + ); (5) Locals should not be initialized (per my last memories of the edk2 coding style). > + UINT64 NewBase; > + > + if (IntersectionBase >= IntersectionEnd) { > + return; // no overlap > + } > + > + NewBase = (PlatformInfoHob->PcdPciMmio64Base - > + PlatformInfoHob->PcdPciMmio64Size); (6) This appears a typo; we'll want NewBase + PcdPciMmio64Size == E820Entry->BaseAddr where the LHS stands for the exclusive end of the moved MMIO aperture, with unchanged size, and the RHS stands for the inclusive base of the (unmovable) reservation. The above formula ensures that the intersection be empty, without changing sizes, or moving the reservation. Then, reordering the formula for an assignment, we get NewBase = E820Entry->BaseAddr - PcdPciMmio64Size; as an assignment. So, yes, a typo. > + DEBUG (( > + DEBUG_INFO, > + "%a: move mmio: 0x%Lx => %Lx\n", (7) pls consider DEBUG_VERBOSE, like before (8) usage of the 0x prefix in the debug message is inconsistent, we should prefix the NewBase output with it, too > + __FUNCTION__, > + PlatformInfoHob->PcdPciMmio64Base, > + NewBase > + )); > + PlatformInfoHob->PcdPciMmio64Base = NewBase; > +} (9) Do we need any other checks or maybe assertions that we're only conflicting with a reserved area, and/or that the subtraction for NewBase does not underflow? I don't think we can "armor" this very well, I'm just pondering if there are any egregious misunderstandings between QEMU and the firmware that we might want to catch here. If not, that's OK of course. > + > /** > Iterate over the RAM entries in QEMU's fw_cfg E820 RAM map, call the > passed callback for each entry. > @@ -638,6 +678,7 @@ PlatformDynamicMmioWindow ( > DEBUG ((DEBUG_INFO, "%a: MMIO Space 0x%Lx (%Ld GB)\n", __func__, MmioSpace, RShiftU64 (MmioSpace, 30))); > PlatformInfoHob->PcdPciMmio64Size = MmioSpace; > PlatformInfoHob->PcdPciMmio64Base = AddrSpace - MmioSpace; > + PlatformScanE820 (PlatformReservationConflictCB, PlatformInfoHob); > } else { > DEBUG ((DEBUG_INFO, "%a: using classic mmio window\n", __func__)); > } Looks fine. Thanks, Laszlo