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.52375.1673521781667709388 for ; Thu, 12 Jan 2023 03:09:41 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=ajXGQG6W; 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=1673521780; 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=PFQ8np2g9IYoVSr5lvV0hnac+VlChc7RhrROxIqAlBw=; b=ajXGQG6W8sm8aCLBY8IVDcyOCsfed8W+PESbxzJLJ5ZPcmj6zmBx/25nciNQP3yXHY62NT q5JDHtgvP7wziZUM4bqRXH1/427AVaDg/ETWnmxB6/tXslC4SeN5GE0YVS4cIqj0CBPLja f3xuhk6Y+y802cIe/e4BP4asCwvyPgI= 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-235-GhWgjwgLMMKk8nPMECKgNQ-1; Thu, 12 Jan 2023 06:09:38 -0500 X-MC-Unique: GhWgjwgLMMKk8nPMECKgNQ-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 E50803C0253B; Thu, 12 Jan 2023 11:09:37 +0000 (UTC) Received: from [10.39.192.93] (unknown [10.39.192.93]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 8AA1F140EBF5; Thu, 12 Jan 2023 11:09:36 +0000 (UTC) Message-ID: Date: Thu, 12 Jan 2023 12:09:35 +0100 MIME-Version: 1.0 Subject: Re: [PATCH v2 2/4] OvmfPkg/PlatformInitLib: Add PlatformGetLowMemoryCB To: Gerd Hoffmann Cc: devel@edk2.groups.io, Pawel Polawski , Jiewen Yao , Oliver Steffen , Jordan Justen , Ard Biesheuvel References: <20230110082123.159521-1-kraxel@redhat.com> <20230110082123.159521-3-kraxel@redhat.com> <043b03d6-0a6b-c533-255b-24a7805d5cca@redhat.com> <20230111072925.l47t4ahgynqjyegq@sirius.home.kraxel.org> <4ace3789-7192-0c53-b4b2-f62f907176d0@redhat.com> <20230111152341.d3p6iy5pml7shfvk@sirius.home.kraxel.org> From: "Laszlo Ersek" In-Reply-To: <20230111152341.d3p6iy5pml7shfvk@sirius.home.kraxel.org> 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/11/23 16:23, Gerd Hoffmann wrote: > Some test case complained because the 80000000-afffffff range is io > address space (according to /proc/iomem) but not tagged as uncachable > in mtrr registers. Ah, very interesting! I didn't know there was a test case for this. (And now I'm curious, per the new BZ, whether this same test case complained if it saw us go deeper with the low mem amount -- e.g. the same situation arises between 0x7000_0000 and 0x8000_0000.) >>> With gigabyte-alignment being the common case these days it might make >>> sense to place the MMCONFIG area at 0xe0000000 instead ... >> >> I feel really unsafe about complicating this code even further. > > I think it should actually simplify things. All the inconsistencies we > have (as you outlined above) due to the hole punching and edk2 > supporting only a single range for 32bit mmio should go away, and we > will have less address space layout differences between q35 and pc. We've tried 0xE000_0000 in the past, in commit 75136b29541b. But had to revert it in commit eb4d62b0779c, due to 0xE000_0000 tickling a bug in QEMU. The bug tickling was actually reported by you :) See . The current 0xB000_0000 value comes from commit b07de0974b65a, which was a replacement for 75136b29541b (after the revert -- for re-fixing the original issue , which 75136b29541b intended to fix in the first place). > > We'll set LowMemory -> 4G to UC via mtrr (both pc and q35) This is problematic, as LowMemory can have any kind of "resolution" (i.e. an effectively unrestricted count of 1-bits in its binary representation). That's a problem because MtrrLib's algorithm (probably justifiedly) cannot find enough variable MTRRs to cover the boundary precisely -- and will fail. This is why our current logic exists for i440fx, in PlatformQemuUc32BaseInitialization(), rounding up Uc32Base from LowMemory. (Well, if you mean to keep the same logic for both i440fx adn q35, that's OK then.) > > We'll use LowMemory -> 0xFBFFFFFF (pc) or > LowMemory -> 0xdfffffff (q35) as 32bit mmio window. This is precisely the situation (32-bit MMIO aperture below EXBAR) that triggered the QEMU bug described in TianoCore#1859 and commit eb4d62b0779c. I don't know if that QEMU bug is now fixed (and how far in the QEMU release history the breakage goes back). At the time of the edk2 revert commit eb4d62b0779c (2019-JUN-03), QEMU's latest release was v4.0.0 (2019-APR-23). Release v4.1.0 would follow at 2019-AUG-15. > We'll use 0xe0000000 -> 0xeffffffff for mmconfig (q35 only). > > Qemu will add 0xf0000000 -> 0xFBFFFFFF to the PCI bus _CRS so > linux could use it but the firmware wouldn't do anything with > it (q35 only). If QEMU only *added* this range to the _CRS, that would be fine, but the QEMU issue in TianoCore#1859 was that QEMU *moved* the aperture to this range in the _CRS, effectively invalidating all the firmware-assigned BARs (which would now all fall outside of the _CRS). If you can ascertain that this problem is no longer relevant in any QEMU releases that are still in use, then I won't try to block this direction. In that case, it might be sufficient to just "re-play" commit 75136b29541b. (Note however that the MTRR setup was tied to the approach taken, please refer to commits 39b9a5ffe661 and 49edde15230a.) Either way, this has been a brittle area of OVMF platform code, and I'd feel real uncomfortable, providing an explicit ACK. ... Luckily, you wouldn't need an ACK from me :) Laszlo