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 E54857803EA for ; Wed, 31 Jan 2024 15:13:32 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=P9cujgRAmHPxCkRYKOEdGedBrVtsPBzAeZuSCT8+NEk=; 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=1706714011; v=1; b=CSMP0cIncPxXO+E9XtFCzoBmdjMXark+viYfeLrWWpJxyUyTm2PpSqXDZ/9iHTTNz3kwXtxg LYKjLWQYbrSiLr95cy9fcxWVoLstnZ08Wgw4JgS/1SU+MKfRu229jmnW9IfhJdSttI/UDCMzuSX UrOrV4t+o0IWlR8UUc8gta3c= X-Received: by 127.0.0.2 with SMTP id vC3nYY7687511xevN27vdHWa; Wed, 31 Jan 2024 07:13:31 -0800 X-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.web10.16318.1706714010863227076 for ; Wed, 31 Jan 2024 07:13: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-491-4N8BH7_FPlOOaVz3DAaXIQ-1; Wed, 31 Jan 2024 10:13:26 -0500 X-MC-Unique: 4N8BH7_FPlOOaVz3DAaXIQ-1 X-Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (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 62B88185A780; Wed, 31 Jan 2024 15:13:26 +0000 (UTC) X-Received: from [10.39.192.35] (unknown [10.39.192.35]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 6CF1D1C060B1; Wed, 31 Jan 2024 15:13:25 +0000 (UTC) Message-ID: Date: Wed, 31 Jan 2024 16:13:24 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 2/3] OvmfPkg/PlatformPei: rewrite page table calculation To: devel@edk2.groups.io, kraxel@redhat.com Cc: Oliver Steffen , Jiewen Yao , Ard Biesheuvel References: <20240131120000.358090-1-kraxel@redhat.com> <20240131120000.358090-3-kraxel@redhat.com> From: "Laszlo Ersek" In-Reply-To: <20240131120000.358090-3-kraxel@redhat.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.7 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: CgVbYMuLf81HcORPEAAjVjBPx7686176AA= 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=CSMP0cIn; 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/31/24 12:59, Gerd Hoffmann wrote: > Consider 5-level paging. Simplify calculation to make it easier to > understand. The new calculation is not 100% exact, but we only need > a rough estimate to reserve enough memory. >=20 > Signed-off-by: Gerd Hoffmann > --- > OvmfPkg/PlatformPei/MemDetect.c | 42 ++++++++++++++++++--------------- > 1 file changed, 23 insertions(+), 19 deletions(-) The cover letter should have explained that this series depends on the 5-level paging series -- or else, this one should be appended to that series. With no on-list connection between them, it's a mess for me to keep track of which one to merge first. >=20 > diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDet= ect.c > index 338798b54171..e22743b4bfaa 100644 > --- a/OvmfPkg/PlatformPei/MemDetect.c > +++ b/OvmfPkg/PlatformPei/MemDetect.c > @@ -184,8 +184,10 @@ GetPeiMemoryCap ( > BOOLEAN Page1GSupport; > UINT32 RegEax; > UINT32 RegEdx; > - UINT32 Pml4Entries; > - UINT32 PdpEntries; > + UINT32 Level5Pages; > + UINT32 Level4Pages; > + UINT32 Level3Pages; > + UINT32 Level2Pages; > UINTN TotalPages; > UINTN ApStacks; > UINTN MemoryCap; > @@ -203,8 +205,7 @@ GetPeiMemoryCap ( > // > // Dependent on physical address width, PEI memory allocations can be > // dominated by the page tables built for 64-bit DXE. So we key the ca= p off > - // of those. The code below is based on CreateIdentityMappingPageTable= s() in > - // "MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c". > + // of those. > // > Page1GSupport =3D FALSE; > if (PcdGetBool (PcdUse1GPageTable)) { > @@ -217,23 +218,26 @@ GetPeiMemoryCap ( > } > } > =20 > - if (PlatformInfoHob->PhysMemAddressWidth <=3D 39) { > - Pml4Entries =3D 1; > - PdpEntries =3D 1 << (PlatformInfoHob->PhysMemAddressWidth - 30); > - ASSERT (PdpEntries <=3D 0x200); > - } else { > - if (PlatformInfoHob->PhysMemAddressWidth > 48) { > - Pml4Entries =3D 0x200; > - } else { > - Pml4Entries =3D 1 << (PlatformInfoHob->PhysMemAddressWidth - 39); > - } > - > - ASSERT (Pml4Entries <=3D 0x200); > - PdpEntries =3D 512; > + // > + // This calculation doesn't return the *exact* number of page table > + // pages needed. But we only need a rough estimate to reserve > + // enough memory, being off by a few pages isn't much of problem. > + // So keep the calculation simple and easy to understand. > + // > + // Page size is 4k, which needs 12 address bits. > + // Page tables on all levels have 512 entries, mapping to 9 address bi= ts. > + // We use large pages (2M or 1G), so level 1 never exists. > + // Start with level 2, where a page covers 12 + 9 + 9 =3D 30 address b= its. > + // The calculation seems correct. On level 2, using 2MB page size for the physical address space, one entry covers 2MB of phys address space, and we have 512 entries in one 4KB page table page. This means one page table page covers 512 * 2MB =3D 1024MB =3D 1GB =3D 2^30 B. (1) The wording is difficult to follow though, for me anyway. How about this: ------- - A 4KB page accommodates the least significant 12 bits of the virtual address. - A page table entry at any level consumes 8 bytes, so a 4KB page table page (at any level) contains 512 entries, and accommodates 9 bits of the virtual address. - we minimally cover the phys address space with 2MB pages, so level 1 never exists. - If 1G paging is available, then level 2 doesn't exist either. - Start with level 2, where a page table page accommodates 9 + 9 + 12 =3D 30 bits of the virtual address (and covers 1GB of physical address space). ------- If you think this isn't any easier to follow, then feel free to stick with your description. > + Level2Pages =3D (1 << (PlatformInfoHob->PhysMemAddressWidth - 30)) + 1= ; General calculation: 2^PhysMemAddressWidth / 2^21 / 2^9 =3D=3D 2^(PhysMemAddressWidth - 21 - 9) =3D=3D 1 << (PhysMemAddressWidth - 30) OK. The left-shift of the signed int (INT32) constant "1" is also safe, given that (I think?) PhysMemAddressWidth cannot be larger than 57. So the shift count is at most 27, and 1 << 27 is safe. (2) Why do we need to add 1? I don't think there's a need for rounding up. The original dividend, 2^PhysMemAddressWidth, is an integral power of two, and PhysMemAddressWidth is minimally 36 (IIRC). (If PhysMemAddressWidth were less than 30, then the left shift would be undefined, to begin with.) > + Level3Pages =3D (Level2Pages >> 9) + 1; > + Level4Pages =3D (Level3Pages >> 9) + 1; > + Level5Pages =3D (Level4Pages >> 9) + 1; > + if (Page1GSupport) { > + Level2Pages =3D 0; > } (3) I'm sorry, these +1 additions *really* annoy me, not to mention the fact that we *include* those increments in the further shifting. Can we do: UINT64 End; UINT64 Level2Pages, Level3Pages, Level4Pages, Level5Pages; End =3D 1LLU << PlatformInfoHob->PhysMemAddressWidth; Level2Pages =3D Page1GSupport ? 0LLU : End >> 30; Level3Pages =3D MAX (End >> 39, 1LLU); Level4Pages =3D MAX (End >> 48, 1LLU); Level5Pages =3D 1; This doesn't seem any more complicated, and it's exact, I believe. > =20 > - TotalPages =3D Page1GSupport ? Pml4Entries + 1 : > - (PdpEntries + 1) * Pml4Entries + 1; > + TotalPages =3D Level5Pages + Level4Pages + Level3Pages + Level2Pages; > ASSERT (TotalPages <=3D 0x40201); (4) The ASSERT() is no longer correct, for two reasons: (a) it does not consider 5-level paging, (b) the calculation from the patch is not exact anyway. But, I think the method I'm proposing should be exact (discounting that with 5-level paging unavailable, Level5Pages should be set to zero). Assuming PhysMemAddressWidth is 57, and 1GB pages are not supported, we get= : Level2Pages =3D BIT27; Level3Pages =3D BIT18; Level4Pages =3D BIT9; Level5Pages =3D BIT0; therefore ASSERT (TotalPages <=3D 0x8040201); in other words, we only need to add BIT27 to the existing constant 0x40201, in the ASSERT(). > =20 > ApStacks =3D PlatformInfoHob->PcdCpuMaxLogicalProcessorNumber * PcdGe= t32 (PcdCpuApStackSize); Thanks 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 (#114905): https://edk2.groups.io/g/devel/message/114905 Mute This Topic: https://groups.io/mt/104073300/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-