From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web10.1322.1592331120382090456 for ; Tue, 16 Jun 2020 11:12:00 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=ALHbRB/P; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1592331119; 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=6UmRbpU1iyH0Q7oKDrhMiVwOHbTMbT3EPM86TaipTgg=; b=ALHbRB/PLa8t2pQL3bE7A+3bnD2KYH7uw15izQ9zPFNuPPMv+dA548QaUQquq3eOiRs8YQ yjw804JABxOVKDMF2YNgkCERli/MYqKEG/CA1HuAc48PPzpLvPvYtWJuF/ZqLDEGbD3TkC Agx6zUgEukPquWyAd/13ODjW5TuLBBs= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-276-0jYyDNWXP9ezuRVKJPLHwg-1; Tue, 16 Jun 2020 14:11:55 -0400 X-MC-Unique: 0jYyDNWXP9ezuRVKJPLHwg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 5623D80F5DA; Tue, 16 Jun 2020 18:11:53 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-248.ams2.redhat.com [10.36.113.248]) by smtp.corp.redhat.com (Postfix) with ESMTP id C09977890A; Tue, 16 Jun 2020 18:11:50 +0000 (UTC) Subject: Re: [PATCH] UefiCpuPkg: Add New Memory Attributes To: Oleksiy Yakovlev , devel@edk2.groups.io Cc: eric.dong@intel.com, ray.ni@intel.com, rahul1.kumar@intel.com, Felixp@ami.com, Liming Gao , Michael Kinney , Jian J Wang , Hao A Wu , Dandan Bi References: <20200615214548.43792-1-oleksiyy@ami.com> From: "Laszlo Ersek" Message-ID: <56bcdfe1-43e8-e77e-bab0-b3669c206de4@redhat.com> Date: Tue, 16 Jun 2020 20:11:49 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200615214548.43792-1-oleksiyy@ami.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Hi Oleksiy, On 06/15/20 23:45, Oleksiy Yakovlev wrote: > Add usage of EFI_MEMORY_SP and EFI_MEMORY_CPU_CRYPTO > attributes introduced in UEFI 2.8 > (UEFI 2.8, mantis 1919 and 1872) > > Signed-off-by: Oleksiy Yakovlev > --- > UefiCpuPkg/CpuDxe/CpuDxe.c | 2 +- > UefiCpuPkg/CpuDxe/CpuDxe.h | 4 +++- > UefiCpuPkg/CpuDxe/CpuPageTable.c | 2 +- > UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 2 +- > 4 files changed, 6 insertions(+), 4 deletions(-) I suggest / request turning this patch into 5 patches: (a) The first patch should please correct a mistake in commit c18708d2f002 ("MdePkg-UefiSpec.h: Add UEFI 2.8 new memory attributes", 2019-11-04). Namely, in commit c18708d2f002, the EFI_MEMORY_CPU_CRYPTO macro's documentation includes the string "CPU?s", twice, in place of "CPU's". I don't understand how this happened. In the mailing list archive, I can only find Liming's confirmation that he pushed the patch: https://edk2.groups.io/g/devel/message/49893 but not the original patch posting. Note that, in the context quoted in that message (that is, the patch), the string was "CPU’s". That string did not use ASCII character 0x27, but U+2019 (RIGHT SINGLE QUOTATION MARK). So indeed the patch was incorrect. But the solution should not have been to replace U+2019 with "?", but to request a repost using ASCII 0x27. Either way, even though it is obviously not your mistake, can you please include a patch for replacing both "CPU?s" instances with "CPU's"? In file "MdePkg/Include/Uefi/UefiSpec.h". For the rest of the patches, please consider: $ git grep -E 'EFI_MEMORY_(RO|RP|XP) \| EFI_MEMORY_(RO|RP|XP) \| EFI_MEMORY_(RO|RP|XP)' MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c:#define MEMORY_ATTRIBUTE_MASK (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO) MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c:#define MEMORY_PAGE_ATTRIBUTES (EFI_MEMORY_XP | EFI_MEMORY_RP | EFI_MEMORY_RO) UefiCpuPkg/CpuDxe/CpuDxe.c:#define MEMORY_ATTRIBUTE_MASK (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO) UefiCpuPkg/CpuDxe/CpuPageTable.c: if ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP)) != 0) { UefiCpuPkg/CpuDxe/CpuPageTable.c: Capabilities = EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP; UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c: ASSERT ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP)) == 0); This output tells us the following: - the bitmask (RP|XP|RO) is *triplicated* between the macro definitions in: - MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c - MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c - UefiCpuPkg/CpuDxe/CpuDxe.c - "UefiCpuPkg/CpuDxe/CpuPageTable.c" open-codes the bitmask in two separate spots (rather than using MEMORY_ATTRIBUTE_MASK) - "UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c" open codes the bitmask also (rather than using any macro). (b) Therefore, the second patch should introduce a central macro for (RP|XP|RO) somewhere under MdePkg or MdeModulePkg. Perhaps it can even be a fixed-only PCD. (c) The third patch should replace all of the open coded bitmasks in MdeModulePkg (see the list above) with references to the new central macro (or PCD). (d) The fourth patch should do the same in UefiCpuPkg. (e) The final patch should modify the central macro to include EFI_MEMORY_SP and EFI_MEMORY_CPU_CRYPTO. This is just my opinion of course, please discuss it further with the MdePkg / MdeModulePkg / UefiCpuPkg maintainers (I've CC'd them). Thanks, Laszlo > > diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c > index a571fc3..55ca764 100644 > --- a/UefiCpuPkg/CpuDxe/CpuDxe.c > +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c > @@ -11,7 +11,7 @@ > #include "CpuPageTable.h" > > #define CACHE_ATTRIBUTE_MASK (EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB | EFI_MEMORY_UCE | EFI_MEMORY_WP) > -#define MEMORY_ATTRIBUTE_MASK (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO) > +#define MEMORY_ATTRIBUTE_MASK (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO | EFI_MEMORY_SP | EFI_MEMORY_CPU_CRYPTO) > > // > // Global Variables > diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.h b/UefiCpuPkg/CpuDxe/CpuDxe.h > index 9299eaa..37fb38e 100644 > --- a/UefiCpuPkg/CpuDxe/CpuDxe.h > +++ b/UefiCpuPkg/CpuDxe/CpuDxe.h > @@ -48,7 +48,9 @@ > > #define EFI_MEMORY_PAGETYPE_MASK (EFI_MEMORY_RP | \ > EFI_MEMORY_XP | \ > - EFI_MEMORY_RO \ > + EFI_MEMORY_RO | \ > + EFI_MEMORY_SP | \ > + EFI_MEMORY_CPU_CRYPTO \ > ) > > #define HEAP_GUARD_NONSTOP_MODE \ > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c > index 0a02cb3..d769e4a 100644 > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c > @@ -717,7 +717,7 @@ ConvertMemoryPageAttributes ( > return RETURN_INVALID_PARAMETER; > } > > - if ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP)) != 0) { > + if ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP | EFI_MEMORY_SP | EFI_MEMORY_CPU_CRYPTO)) != 0) { > DEBUG ((DEBUG_ERROR, "Attributes(0x%lx) has unsupported bit\n", Attributes)); > return EFI_UNSUPPORTED; > } > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > index 9c5a92a..94adf37 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > @@ -435,7 +435,7 @@ ConvertMemoryPageAttributes ( > EFI_PHYSICAL_ADDRESS MaximumSupportMemAddress; > > ASSERT (Attributes != 0); > - ASSERT ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP)) == 0); > + ASSERT ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP | EFI_MEMORY_SP | EFI_MEMORY_CPU_CRYPTO)) == 0); > > ASSERT ((BaseAddress & (SIZE_4KB - 1)) == 0); > ASSERT ((Length & (SIZE_4KB - 1)) == 0); >