From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web12.10051.1592991745101612882 for ; Wed, 24 Jun 2020 02:42:25 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=H8I6ogbq; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1592991744; 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=Y+Jshz5+IshxX0BsBrB4x7YIOmfscCpbyQCsa2lyZsM=; b=H8I6ogbqxw5oszoeqXbcyUGuRkxv3X39BHdNT5DLo7/yxXzQmwdjO4wwts6aohukSb7i+S 53W8gnIYDiqt+mzGNWeTxHujuzU6uoh1zT5YEiiXWSXMViXSkkJLeAPoUcsE6TREzfdQsa zO41Cxf3kdVku+ai1Q7pn+O11e2Wtx8= 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-199-982hxAOxMbab-fZTnfbi1g-1; Wed, 24 Jun 2020 05:42:20 -0400 X-MC-Unique: 982hxAOxMbab-fZTnfbi1g-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 72F11800FEF; Wed, 24 Jun 2020 09:42:18 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-56.ams2.redhat.com [10.36.112.56]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7AE065C290; Wed, 24 Jun 2020 09:42:16 +0000 (UTC) Subject: Re: [PATCH V2 3/3] UefiCpuPkg: Add New Memory Attributes To: Oleksiy Yakovlev , devel@edk2.groups.io Cc: liming.gao@intel.com, michael.d.kinney@intel.com, dandan.bi@intel.com, ray.ni@intel.com, rahul1.kumar@intel.com, Felixp@ami.com, Eric Dong References: <20200623215556.80804-1-oleksiyy@ami.com> <20200623215556.80804-4-oleksiyy@ami.com> From: "Laszlo Ersek" Message-ID: <14289ecf-8658-f173-4d64-e3def781fc88@redhat.com> Date: Wed, 24 Jun 2020 11:42:15 +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: <20200623215556.80804-4-oleksiyy@ami.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 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 06/23/20 23:55, 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). > Use attributes bitmasks, defined in MdePkg. > > Signed-off-by: Oleksiy Yakovlev > --- > UefiCpuPkg/CpuDxe/CpuDxe.c | 11 ++++------- > UefiCpuPkg/CpuDxe/CpuDxe.h | 12 ------------ > UefiCpuPkg/CpuDxe/CpuPageTable.c | 6 +++--- > UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 2 +- > 4 files changed, 8 insertions(+), 23 deletions(-) > > diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c > index a571fc3..52cc26e 100644 > --- a/UefiCpuPkg/CpuDxe/CpuDxe.c > +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c > @@ -10,9 +10,6 @@ > #include "CpuMp.h" > #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) > - > // > // Global Variables > // > @@ -417,8 +414,8 @@ CpuSetMemoryAttributes ( > return EFI_SUCCESS; > } > > - CacheAttributes = Attributes & CACHE_ATTRIBUTE_MASK; > - MemoryAttributes = Attributes & MEMORY_ATTRIBUTE_MASK; > + CacheAttributes = Attributes & EFI_CACHE_ATTRIBUTE_MASK; > + MemoryAttributes = Attributes & EFI_MEMORY_ATTRIBUTE_MASK; > > if (Attributes != (CacheAttributes | MemoryAttributes)) { > return EFI_INVALID_PARAMETER; OK. > @@ -677,7 +674,7 @@ SetGcdMemorySpaceAttributes ( > gDS->SetMemorySpaceAttributes ( > RegionStart, > RegionLength, > - (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_CACHETYPE_MASK) | (MemorySpaceMap[Index].Capabilities & Attributes) > + (MemorySpaceMap[Index].Attributes & ~EFI_CACHE_ATTRIBUTE_MASK) | (MemorySpaceMap[Index].Capabilities & Attributes) > ); > } > > @@ -754,7 +751,7 @@ RefreshMemoryAttributesFromMtrr ( > gDS->SetMemorySpaceAttributes ( > MemorySpaceMap[Index].BaseAddress, > MemorySpaceMap[Index].Length, > - (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_CACHETYPE_MASK) | > + (MemorySpaceMap[Index].Attributes & ~EFI_CACHE_ATTRIBUTE_MASK) | > (MemorySpaceMap[Index].Capabilities & DefaultAttributes) > ); > } > diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.h b/UefiCpuPkg/CpuDxe/CpuDxe.h > index 9299eaa..9771ec8 100644 > --- a/UefiCpuPkg/CpuDxe/CpuDxe.h > +++ b/UefiCpuPkg/CpuDxe/CpuDxe.h > @@ -39,18 +39,6 @@ > #include > #include > > -#define EFI_MEMORY_CACHETYPE_MASK (EFI_MEMORY_UC | \ > - EFI_MEMORY_WC | \ > - EFI_MEMORY_WT | \ > - EFI_MEMORY_WB | \ > - EFI_MEMORY_UCE \ > - ) > - > -#define EFI_MEMORY_PAGETYPE_MASK (EFI_MEMORY_RP | \ > - EFI_MEMORY_XP | \ > - EFI_MEMORY_RO \ > - ) > - > #define HEAP_GUARD_NONSTOP_MODE \ > ((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT6|BIT4|BIT1|BIT0)) > BIT6) > (1) These changes have an additional effect. EFI_MEMORY_CACHETYPE_MASK does not include EFI_MEMORY_WP, but EFI_CACHE_ATTRIBUTE_MASK does. (1a) If that change is intentional, then this patch can remain as it is, but we need an extra patch prepended (i.e., inserted between v2 patches #2 and #3), for adding EFI_MEMORY_WP to EFI_MEMORY_CACHETYPE_MASK first. (1b) If the EFI_MEMORY_WP change is not intended (i.e., it is an oversight in this patch), then in every spot where we replace EFI_MEMORY_CACHETYPE_MASK with EFI_CACHE_ATTRIBUTE_MASK, we need to account for EFI_MEMORY_WP separately. ... After reading up on EFI_MEMORY_WP in the UEFI spec, I think it's (1a) -- meaning that, this patch is correct, in itself. But, we should still not hide the EFI_MEMORY_WP bugfix, for EFI_MEMORY_CACHETYPE_MASK, in this patch. So please insert a new patch just before this one, that does nothing other than include EFI_MEMORY_WP in EFI_MEMORY_CACHETYPE_MASK. The rest of the patch looks OK to me. Therefore, for this patch (in itself): Reviewed-by: Laszlo Ersek Eric, Ray, Rahul: correct me if I'm wrong, please. Thanks, Laszlo > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c > index 0a02cb3..06ee1b8 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_ATTRIBUTE_MASK) != 0) { > DEBUG ((DEBUG_ERROR, "Attributes(0x%lx) has unsupported bit\n", Attributes)); > return EFI_UNSUPPORTED; > } > @@ -1018,9 +1018,9 @@ RefreshGcdMemoryAttributesFromPaging ( > > Length = MIN (PageLength, MemorySpaceLength); > if (Attributes != (MemorySpaceMap[Index].Attributes & > - EFI_MEMORY_PAGETYPE_MASK)) { > + EFI_MEMORY_ATTRIBUTE_MASK)) { > NewAttributes = (MemorySpaceMap[Index].Attributes & > - ~EFI_MEMORY_PAGETYPE_MASK) | Attributes; > + ~EFI_MEMORY_ATTRIBUTE_MASK) | Attributes; > Status = gDS->SetMemorySpaceAttributes ( > BaseAddress, > Length, > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > index 9c5a92a..ebfc46a 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_ATTRIBUTE_MASK) == 0); > > ASSERT ((BaseAddress & (SIZE_4KB - 1)) == 0); > ASSERT ((Length & (SIZE_4KB - 1)) == 0); >