* [PATCH] UefiCpuPkg:PiSmmCpu: Set correct attribute on split. @ 2016-11-29 7:39 Jiewen Yao 2016-11-29 21:54 ` Laszlo Ersek 0 siblings, 1 reply; 6+ messages in thread From: Jiewen Yao @ 2016-11-29 7:39 UTC (permalink / raw) To: edk2-devel; +Cc: Jeff Fan, Michael D Kinney, Laszlo Ersek 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 <jeff.fan@intel.com> Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Jiewen Yao <jiewen.yao@intel.com> --- 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; -- 2.7.4.windows.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] UefiCpuPkg:PiSmmCpu: Set correct attribute on split. 2016-11-29 7:39 [PATCH] UefiCpuPkg:PiSmmCpu: Set correct attribute on split Jiewen Yao @ 2016-11-29 21:54 ` Laszlo Ersek 2016-11-30 1:47 ` Yao, Jiewen 0 siblings, 1 reply; 6+ messages in thread From: Laszlo Ersek @ 2016-11-29 21:54 UTC (permalink / raw) To: Jiewen Yao, edk2-devel; +Cc: Michael D Kinney, Jeff Fan 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 <jeff.fan@intel.com> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jiewen Yao <jiewen.yao@intel.com> > --- > 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.) 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". - "end page" might be more clearly stated as "leaf page" (just a guess) - 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. - What do you think of the following subject line? UefiCpuPkg/PiSmmCpuDxeSmm: relax superpage protection on page split Anyway, to the extent that I understand this, I agree: Acked-by: Laszlo Ersek <lersek@redhat.com> I gave the patch a bit of testing in my usual environment; it seems to cause no problems. Tested-by: Laszlo Ersek <lersek@redhat.com> Thanks Laszlo ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] UefiCpuPkg:PiSmmCpu: Set correct attribute on split. 2016-11-29 21:54 ` Laszlo Ersek @ 2016-11-30 1:47 ` Yao, Jiewen 2016-11-30 5:50 ` Yao, Jiewen 0 siblings, 1 reply; 6+ messages in thread From: Yao, Jiewen @ 2016-11-30 1:47 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel@ml01.01.org; +Cc: Kinney, Michael D, Fan, Jeff Comments below: From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Wednesday, November 30, 2016 5:54 AM To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@ml01.01.org Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Fan, Jeff <jeff.fan@intel.com> 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 <jeff.fan@intel.com<mailto:jeff.fan@intel.com>> > Cc: Michael D Kinney <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> > Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> > --- > 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 <lersek@redhat.com<mailto:lersek@redhat.com>> 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 <lersek@redhat.com<mailto:lersek@redhat.com>> Thanks Laszlo ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] UefiCpuPkg:PiSmmCpu: Set correct attribute on split. 2016-11-30 1:47 ` Yao, Jiewen @ 2016-11-30 5:50 ` Yao, Jiewen 2016-11-30 8:29 ` Laszlo Ersek 2016-11-30 8:35 ` Fan, Jeff 0 siblings, 2 replies; 6+ messages in thread From: Yao, Jiewen @ 2016-11-30 5:50 UTC (permalink / raw) To: Yao, Jiewen, Laszlo Ersek, edk2-devel@ml01.01.org Cc: Kinney, Michael D, Fan, Jeff 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. ================================ 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 <lersek@redhat.com>; edk2-devel@ml01.01.org Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Fan, Jeff <jeff.fan@intel.com> 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 <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; edk2-devel@ml01.01.org<mailto:edk2-devel@ml01.01.org> Cc: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Fan, Jeff <jeff.fan@intel.com<mailto:jeff.fan@intel.com>> 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 <jeff.fan@intel.com<mailto:jeff.fan@intel.com<mailto:jeff.fan@intel.com%3cmailto:jeff.fan@intel.com>>> > Cc: Michael D Kinney <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com%3cmailto:michael.d.kinney@intel.com>>> > Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com<mailto:lersek@redhat.com%3cmailto:lersek@redhat.com>>> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com<mailto:jiewen.yao@intel.com%3cmailto:jiewen.yao@intel.com>>> > --- > 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 <lersek@redhat.com<mailto:lersek@redhat.com<mailto:lersek@redhat.com%3cmailto:lersek@redhat.com>>> 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 <lersek@redhat.com<mailto:lersek@redhat.com<mailto:lersek@redhat.com%3cmailto:lersek@redhat.com>>> Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] UefiCpuPkg:PiSmmCpu: Set correct attribute on split. 2016-11-30 5:50 ` Yao, Jiewen @ 2016-11-30 8:29 ` Laszlo Ersek 2016-11-30 8:35 ` Fan, Jeff 1 sibling, 0 replies; 6+ messages in thread From: Laszlo Ersek @ 2016-11-30 8:29 UTC (permalink / raw) To: Yao, Jiewen, edk2-devel@ml01.01.org; +Cc: Kinney, Michael D, Fan, Jeff 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 <lersek@redhat.com>; edk2-devel@ml01.01.org > *Cc:* Kinney, Michael D <michael.d.kinney@intel.com>; Fan, Jeff > <jeff.fan@intel.com> > *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 <jiewen.yao@intel.com > <mailto:jiewen.yao@intel.com>>; edk2-devel@ml01.01.org > <mailto:edk2-devel@ml01.01.org> > Cc: Kinney, Michael D <michael.d.kinney@intel.com > <mailto:michael.d.kinney@intel.com>>; Fan, Jeff <jeff.fan@intel.com > <mailto:jeff.fan@intel.com>> > 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 <jeff.fan@intel.com<mailto:jeff.fan@intel.com > <mailto:jeff.fan@intel.com%3cmailto:jeff.fan@intel.com>>> >> Cc: Michael D Kinney <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com > <mailto:michael.d.kinney@intel.com%3cmailto:michael.d.kinney@intel.com>>> >> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com > <mailto:lersek@redhat.com%3cmailto:lersek@redhat.com>>> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com > <mailto:jiewen.yao@intel.com%3cmailto:jiewen.yao@intel.com>>> >> --- >> 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 <lersek@redhat.com<mailto:lersek@redhat.com > <mailto:lersek@redhat.com%3cmailto:lersek@redhat.com>>> > > 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 <lersek@redhat.com<mailto:lersek@redhat.com > <mailto:lersek@redhat.com%3cmailto:lersek@redhat.com>>> > > Thanks > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org> > https://lists.01.org/mailman/listinfo/edk2-devel > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] UefiCpuPkg:PiSmmCpu: Set correct attribute on split. 2016-11-30 5:50 ` Yao, Jiewen 2016-11-30 8:29 ` Laszlo Ersek @ 2016-11-30 8:35 ` Fan, Jeff 1 sibling, 0 replies; 6+ messages in thread From: Fan, Jeff @ 2016-11-30 8:35 UTC (permalink / raw) To: Yao, Jiewen, Laszlo Ersek, edk2-devel@ml01.01.org; +Cc: Kinney, Michael D Reviewed-by: Jeff Fan <jeff.fan@intel.com<mailto:jeff.fan@intel.com>> with new updating. From: Yao, Jiewen Sent: Wednesday, November 30, 2016 1:50 PM To: Yao, Jiewen; Laszlo Ersek; edk2-devel@ml01.01.org Cc: Kinney, Michael D; Fan, Jeff Subject: RE: [edk2] [PATCH] UefiCpuPkg:PiSmmCpu: Set correct attribute on split. 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. ================================ 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 <lersek@redhat.com<mailto:lersek@redhat.com>>; edk2-devel@ml01.01.org<mailto:edk2-devel@ml01.01.org> Cc: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Fan, Jeff <jeff.fan@intel.com<mailto:jeff.fan@intel.com>> 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 <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; edk2-devel@ml01.01.org<mailto:edk2-devel@ml01.01.org> Cc: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Fan, Jeff <jeff.fan@intel.com<mailto:jeff.fan@intel.com>> 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 <jeff.fan@intel.com<mailto:jeff.fan@intel.com<mailto:jeff.fan@intel.com%3cmailto:jeff.fan@intel.com>>> > Cc: Michael D Kinney <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com%3cmailto:michael.d.kinney@intel.com>>> > Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com<mailto:lersek@redhat.com%3cmailto:lersek@redhat.com>>> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com<mailto:jiewen.yao@intel.com%3cmailto:jiewen.yao@intel.com>>> > --- > 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 <lersek@redhat.com<mailto:lersek@redhat.com<mailto:lersek@redhat.com%3cmailto:lersek@redhat.com>>> 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 <lersek@redhat.com<mailto:lersek@redhat.com<mailto:lersek@redhat.com%3cmailto:lersek@redhat.com>>> Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-11-30 9:11 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-29 7:39 [PATCH] UefiCpuPkg:PiSmmCpu: Set correct attribute on split Jiewen Yao 2016-11-29 21:54 ` Laszlo Ersek 2016-11-30 1:47 ` Yao, Jiewen 2016-11-30 5:50 ` Yao, Jiewen 2016-11-30 8:29 ` Laszlo Ersek 2016-11-30 8:35 ` Fan, Jeff
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox