From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by ml01.01.org (Postfix) with ESMTP id 7E8EC81C75 for ; Wed, 30 Nov 2016 01:11:07 -0800 (PST) Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E0B138EB45; Wed, 30 Nov 2016 08:29:14 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-51.phx2.redhat.com [10.3.116.51]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uAU8TDEf020690; Wed, 30 Nov 2016 03:29:13 -0500 To: "Yao, Jiewen" , "edk2-devel@ml01.01.org" References: <1480405175-61868-1-git-send-email-jiewen.yao@intel.com> <6b50ad2b-b01f-0345-bf20-6fcd875dccec@redhat.com> <74D8A39837DF1E4DA445A8C0B3885C50386DC7AD@shsmsx102.ccr.corp.intel.com> <74D8A39837DF1E4DA445A8C0B3885C50386DCBF2@shsmsx102.ccr.corp.intel.com> Cc: "Kinney, Michael D" , "Fan, Jeff" From: Laszlo Ersek Message-ID: <0b606e4d-9604-23b4-101d-ded267a276b4@redhat.com> Date: Wed, 30 Nov 2016 09:29:12 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.0 MIME-Version: 1.0 In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C50386DCBF2@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Wed, 30 Nov 2016 08:29:14 +0000 (UTC) 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 09:15:15 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 11/30/16 06:50, Yao, Jiewen wrote: > How about this: > > > > ================================ > > 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. > > ================================ Sounds good to me, thanks! Laszlo > 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 split. > > > > 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 split. > > 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/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c >> index accc11e..d0f41a8 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c >> @@ -303,7 +303,7 @@ SplitPage ( >> for (Index = 0; Index < SIZE_4KB / sizeof(UINT64); Index++) { >> NewPageEntry[Index] = BaseAddress + SIZE_4KB * Index + ((*PageEntry) & PAGE_PROGATE_BITS); >> } >> - (*PageEntry) = (UINT64)(UINTN)NewPageEntry + ((*PageEntry) & PAGE_PROGATE_BITS); >> + (*PageEntry) = (UINT64)(UINTN)NewPageEntry + PAGE_ATTRIBUTE_BITS; >> return RETURN_SUCCESS; >> } else { >> return RETURN_UNSUPPORTED; >> @@ -324,7 +324,7 @@ SplitPage ( >> for (Index = 0; Index < SIZE_4KB / sizeof(UINT64); Index++) { >> NewPageEntry[Index] = BaseAddress + SIZE_2MB * Index + IA32_PG_PS + ((*PageEntry) & PAGE_PROGATE_BITS); >> } >> - (*PageEntry) = (UINT64)(UINTN)NewPageEntry + ((*PageEntry) & PAGE_PROGATE_BITS); >> + (*PageEntry) = (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 >