From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f181.google.com (mail-pl1-f181.google.com [209.85.214.181]) by mx.groups.io with SMTP id smtpd.web11.108137.1673663744417518923 for ; Fri, 13 Jan 2023 18:35:44 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20210112 header.b=NJfIeGoE; spf=pass (domain: gmail.com, ip: 209.85.214.181, mailfrom: pedro.falcato@gmail.com) Received: by mail-pl1-f181.google.com with SMTP id d15so25216969pls.6 for ; Fri, 13 Jan 2023 18:35:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=qRUySYRC3HZ9s03xWQ+7fXx2hL0M2zWKSz+ldZHDkcc=; b=NJfIeGoEl3aXV0Fy4ut8pWPWdOiwmxd1GJVs04GknTPwtW/5DV496/eFEIQYfYoT7f 0ATit+hInG4M0LgPyUKDdlT9HGEmjKzu7GP/LTyfAGgikZti11Wqy5crzs0s4PxEfzxt BJleZcgoZ9buMj0k5Xvnyw8K92jH8RY29aU7rkyql6HriTUZSiTL0rwIcMH45RkFUpo+ oaz/2hPpMqlRrJyh6Xk7RaRoMciAota9QGRkwqkCzJHyWL/mpepatuwQi3tuCqMyIIgs ewJV7BoIX3uFqwqI4a+tu2yGqrZfaFnbs+pDqe1PhpPIlSGPNJX4OOvYztg3t2hPYJgV aXwA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=qRUySYRC3HZ9s03xWQ+7fXx2hL0M2zWKSz+ldZHDkcc=; b=xi+5e/ZBI66X6RejM9cCQvIBjwwYerj0D6fcneMVyj2ECwMF4/WwBrr9T49M6oIi5V rmCvSBUynvhnKyxiFVFDEaesPgHKE3zCKarC+q+znVChcU/RTuKsex0UthVXheUHrZVw Fp2kv8oEsSRK3S4vG7jR/fC3JZLSCepqbO23joPPrIoXszAeX6qa1wKYYdDNBQjFYMfg ibB7Olhb3FNiLrDth8JLxVwqNBtp/EZyFk4EzGUEGnGJOaCjp0KUKfWswcGeKhvSkLYq Y66p7xQ6TO2ArrBY2iuX48ArcXtD5JzTEvw6TUR98iizfue6Fx9da2eNKfqPP6qHWDF+ AjqQ== X-Gm-Message-State: AFqh2kpHvhfAH2CQqpgvo6GaUe2lsx2TYQMkiriKW8dnQXJtxEIoyNKw 1/us2/Iw1qSpEmMpJJzvI0tcyCB3glQMKz1XdQo4Tl+vPEsosw== X-Google-Smtp-Source: AMrXdXtFOrpn+0PSEfywbmGmQOHYZdUccn0TD2/O2sxZKHn4zZF98e+n8WTrvn/RrD3KhfVE28vg4E5GnKDXL1BmMVs= X-Received: by 2002:a17:90a:b294:b0:229:14a7:4046 with SMTP id c20-20020a17090ab29400b0022914a74046mr727063pjr.44.1673663743446; Fri, 13 Jan 2023 18:35:43 -0800 (PST) MIME-Version: 1.0 References: <5e1916c6669ee2a918a2c22b7698e96f7dbf5488.1673568149.git.lisik@google.com> In-Reply-To: <5e1916c6669ee2a918a2c22b7698e96f7dbf5488.1673568149.git.lisik@google.com> From: "Pedro Falcato" Date: Sat, 14 Jan 2023 02:35:29 +0000 Message-ID: Subject: Re: [edk2-devel] [PATCH] OvmfPkg: Create additional PML4 entries for large SEV-SNP VMs To: devel@edk2.groups.io, lisik@google.com Cc: Erdem Aktas , James Bottomley , "Yao, Jiewen" , Min Xu , Tom Lendacky , michael.roth@amd.com Content-Type: text/plain; charset="UTF-8" On Fri, Jan 13, 2023 at 5:02 PM Mikolaj Lisik via groups.io wrote: > > Edk2 was failing, rather than creating more PML4 entries, when they > weren't present in the initial memory acceptance flow. Because of that > VMs with more than 512G memory were crashing. This code fixes that. > > This change affects only SEV-SNP VMs. > > The code was tested by successfully booting a 512G SEV-SNP VM. > > Signed-off-by: Mikolaj Lisik > --- > .../X64/PeiDxeVirtualMemory.c | 18 ++++++++---------- > 1 file changed, 8 insertions(+), 10 deletions(-) > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c > index b9c0a5b25a..3dbff51ac2 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c > @@ -548,6 +548,7 @@ InternalMemEncryptSevCreateIdentityMap1G ( > PAGE_MAP_AND_DIRECTORY_POINTER *PageMapLevel4Entry; > PAGE_TABLE_1G_ENTRY *PageDirectory1GEntry; > UINT64 PgTableMask; > + UINT64 *NewPageTable; > UINT64 AddressEncMask; > BOOLEAN IsWpEnabled; > RETURN_STATUS Status; > @@ -602,15 +603,13 @@ InternalMemEncryptSevCreateIdentityMap1G ( > PageMapLevel4Entry = (VOID *)(Cr3BaseAddress & ~PgTableMask); > PageMapLevel4Entry += PML4_OFFSET (PhysicalAddress); > if (!PageMapLevel4Entry->Bits.Present) { > - DEBUG (( > - DEBUG_ERROR, > - "%a:%a: bad PML4 for Physical=0x%Lx\n", > - gEfiCallerBaseName, > - __FUNCTION__, > - PhysicalAddress > - )); > - Status = RETURN_NO_MAPPING; > - goto Done; > + NewPageTable = AllocatePageTableMemory(1); > + ASSERT(NewPageTable != NULL); Hi, (+cc OVMF SEV maintainers) This should not use an ASSERT as those can get (purposefully) deleted on release builds. Please do proper error handling like in the code block you deleted. > + SetMem (NewPageTable, EFI_PAGE_SIZE, 0); > + PageMapLevel4Entry->Uint64 = (UINT64)(UINTN)NewPageTable | AddressEncMask; > + PageMapLevel4Entry->Bits.Present = 1; > + PageMapLevel4Entry->Bits.MustBeZero = 0; > + PageMapLevel4Entry->Bits.ReadWrite = 1; Please swap the Present and ReadWrite lines. You should only set Present after everything else as to avoid explicit caching (TLB and paging structure) issues. Although this whole file is full of spotty behavior. ASSERTS can get deleted and the file is full of them. There are plenty of Present = 1 sets before setting other important bits like RW, which *will* cause you to get bad TLB entries if the CPU speculates a load/store to that address. Unions don't define how they write back data; meaning that they can write everything on one go, or write the u64 and then set bits individually, or write the u64 and then set the bits all at once. Bit fields' layouts are not specified in the standard and because of that struct S{int bit : 1; int bit2 : 1;}; doesn't guarantee bit and bit2 are contiguous. Union type punning is also undefined behavior in standard C (it is not in GNU C, no idea about MSVC). If everything was done correctly, there would be no need for the hacky CpuFlushTlb (); down there, as this file adds PTEs, and doesn't modify them. (now that I check, all of this insane behavior seems to have been inherited from MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c. Sorry for the rant.) *sigh* In any case, because of that CpuFlushTlb () your patch isn't wrong, but it should be changed into something more MMU-code natural. -- Pedro