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 AB78B81D43 for ; Tue, 29 Nov 2016 21:50:18 -0800 (PST) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga105.fm.intel.com with ESMTP; 29 Nov 2016 21:50:18 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,720,1473145200"; d="scan'208,217";a="37080809" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by orsmga005.jf.intel.com with ESMTP; 29 Nov 2016 21:50:17 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 29 Nov 2016 21:50:17 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.239]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.96]) with mapi id 14.03.0248.002; Wed, 30 Nov 2016 13:50:13 +0800 From: "Yao, Jiewen" To: "Yao, Jiewen" , 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+zhCAAEvuUA== Date: Wed, 30 Nov 2016 05:50:13 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C50386DCBF2@shsmsx102.ccr.corp.intel.com> References: <1480405175-61868-1-git-send-email-jiewen.yao@intel.com> <6b50ad2b-b01f-0345-bf20-6fcd875dccec@redhat.com> <74D8A39837DF1E4DA445A8C0B3885C50386DC7AD@shsmsx102.ccr.corp.intel.com> In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C50386DC7AD@shsmsx102.ccr.corp.intel.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 05:50:18 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable How about this: =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D UefiCpuPkg/PiSmmCpu: relax superpage protection on page split. PiSmmCpu driver may split page for page attribute request. Current logic not only propagates the super page attribute to the leaf page attribut, but also to the directory page attribute. However, the later might be wrong because we cannot clear protection without touching directory page attribute. The effective protection is the strictest combination across the levels. We should always clear protection on directory page and set protection on leaf page for easy clearing later. =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D Thank you Yao Jiewen From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao,= Jiewen Sent: Wednesday, November 30, 2016 9:48 AM To: Laszlo Ersek ; edk2-devel@ml01.01.org Cc: Kinney, Michael D ; Fan, Jeff Subject: Re: [edk2] [PATCH] UefiCpuPkg:PiSmmCpu: Set correct attribute on s= plit. Comments below: From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Wednesday, November 30, 2016 5:54 AM To: Yao, Jiewen >; edk2-d= evel@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel