From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 9D6BB81CC3 for ; Tue, 29 Nov 2016 17:47:47 -0800 (PST) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga105.fm.intel.com with ESMTP; 29 Nov 2016 17:47:47 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,719,1473145200"; d="scan'208,217";a="37029443" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga005.jf.intel.com with ESMTP; 29 Nov 2016 17:47:46 -0800 Received: from FMSMSX109.amr.corp.intel.com (10.18.116.9) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 29 Nov 2016 17:47:46 -0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx109.amr.corp.intel.com (10.18.116.9) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 29 Nov 2016 17:47:46 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.239]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.138]) with mapi id 14.03.0248.002; Wed, 30 Nov 2016 09:47:42 +0800 From: "Yao, Jiewen" To: Laszlo Ersek , "edk2-devel@ml01.01.org" CC: "Kinney, Michael D" , "Fan, Jeff" Thread-Topic: [edk2] [PATCH] UefiCpuPkg:PiSmmCpu: Set correct attribute on split. Thread-Index: AQHSShPTzRs32mhpAkin+kkIeKUcp6Dv/AUAgAC+zhA= Date: Wed, 30 Nov 2016 01:47:42 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C50386DC7AD@shsmsx102.ccr.corp.intel.com> References: <1480405175-61868-1-git-send-email-jiewen.yao@intel.com> <6b50ad2b-b01f-0345-bf20-6fcd875dccec@redhat.com> In-Reply-To: <6b50ad2b-b01f-0345-bf20-6fcd875dccec@redhat.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 X-Content-Filtered-By: Mailman/MimeDel 2.1.21 Subject: Re: [PATCH] UefiCpuPkg:PiSmmCpu: Set correct attribute on split. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 30 Nov 2016 01:47:47 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Comments below: From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Wednesday, November 30, 2016 5:54 AM To: Yao, Jiewen ; edk2-devel@ml01.01.org Cc: Kinney, Michael D ; Fan, Jeff Subject: Re: [edk2] [PATCH] UefiCpuPkg:PiSmmCpu: Set correct attribute on s= plit. On 11/29/16 08:39, Jiewen Yao wrote: > PiSmmCpu driver may split page for page attribute request. > Current logic will propagate the super page attribute attribute. > However, it might be wrong because we cannot clear protection > without touch super page attribute. > > We should always clear protection on super page and set > protection on end page for easy clear later. > > Cc: Jeff Fan > > Cc: Michael D Kinney > > Cc: Laszlo Ersek > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jiewen Yao > > --- > UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpu= Pkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > index accc11e..d0f41a8 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > @@ -303,7 +303,7 @@ SplitPage ( > for (Index =3D 0; Index < SIZE_4KB / sizeof(UINT64); Index++) { > NewPageEntry[Index] =3D BaseAddress + SIZE_4KB * Index + ((*Page= Entry) & PAGE_PROGATE_BITS); > } > - (*PageEntry) =3D (UINT64)(UINTN)NewPageEntry + ((*PageEntry) & PAG= E_PROGATE_BITS); > + (*PageEntry) =3D (UINT64)(UINTN)NewPageEntry + PAGE_ATTRIBUTE_BITS= ; > return RETURN_SUCCESS; > } else { > return RETURN_UNSUPPORTED; > @@ -324,7 +324,7 @@ SplitPage ( > for (Index =3D 0; Index < SIZE_4KB / sizeof(UINT64); Index++) { > NewPageEntry[Index] =3D BaseAddress + SIZE_2MB * Index + IA32_PG= _PS + ((*PageEntry) & PAGE_PROGATE_BITS); > } > - (*PageEntry) =3D (UINT64)(UINTN)NewPageEntry + ((*PageEntry) & PAG= E_PROGATE_BITS); > + (*PageEntry) =3D (UINT64)(UINTN)NewPageEntry + PAGE_ATTRIBUTE_BITS= ; > return RETURN_SUCCESS; > } else { > return RETURN_UNSUPPORTED; > I had to stare a while at this, to get a superficial understanding :) But, it does seem to make sense (I checked PAGE_ATTRIBUTE_BITS and PAGE_PROGATE_BITS too, just to be sure). So, this change preserves the protection inheritance for the leaf pages, but clears NX and sets Dirty / Accessed / Writeable / Present on the relevant parent entry. (I see hat User mode access is enabled as well; I don't know why that is useful here.) [Jiewen] Yes. You are right. Some notes about the commit message: - we have "attribute attribute". I think we should either drop one of those words, or say "super page attribute to leaf page attribute". [Jiewen] Agree. I will update. - "end page" might be more clearly stated as "leaf page" (just a guess) [Jiewen] Agree. I will update. - I think it would be useful to mention, for the uninitiated like me :), that the effective protection is (apparently) the strictest combination across the levels. [Jiewen] Agree. I will update. - What do you think of the following subject line? UefiCpuPkg/PiSmmCpuDxeSmm: relax superpage protection on page split [Jiewen] Agree. I will update. Anyway, to the extent that I understand this, I agree: Acked-by: Laszlo Ersek > I gave the patch a bit of testing in my usual environment; it seems to cause no problems. [Jiewen] Thank you. Tested-by: Laszlo Ersek > Thanks Laszlo