Reviewed-by: Jeff Fan <jeff....@intel.com<mailto:jeff....@intel.com>> with new updating.
From: Yao, Jiewen Sent: Wednesday, November 30, 2016 1:50 PM To: Yao, Jiewen; Laszlo Ersek; edk2-de...@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-boun...@lists.01.org] On Behalf Of Yao, Jiewen Sent: Wednesday, November 30, 2016 9:48 AM To: Laszlo Ersek <ler...@redhat.com<mailto:ler...@redhat.com>>; edk2-de...@ml01.01.org<mailto:edk2-de...@ml01.01.org> Cc: Kinney, Michael D <michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>>; Fan, Jeff <jeff....@intel.com<mailto:jeff....@intel.com>> Subject: Re: [edk2] [PATCH] UefiCpuPkg:PiSmmCpu: Set correct attribute on split. Comments below: From: Laszlo Ersek [mailto:ler...@redhat.com] Sent: Wednesday, November 30, 2016 5:54 AM To: Yao, Jiewen <jiewen....@intel.com<mailto:jiewen....@intel.com>>; edk2-de...@ml01.01.org<mailto:edk2-de...@ml01.01.org> Cc: Kinney, Michael D <michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>>; Fan, Jeff <jeff....@intel.com<mailto:jeff....@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....@intel.com<mailto:jeff....@intel.com<mailto:jeff....@intel.com%3cmailto:jeff....@intel.com>>> > Cc: Michael D Kinney > <michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com%3cmailto:michael.d.kin...@intel.com>>> > Cc: Laszlo Ersek > <ler...@redhat.com<mailto:ler...@redhat.com<mailto:ler...@redhat.com%3cmailto:ler...@redhat.com>>> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jiewen Yao > <jiewen....@intel.com<mailto:jiewen....@intel.com<mailto:jiewen....@intel.com%3cmailto:jiewen....@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 <ler...@redhat.com<mailto:ler...@redhat.com<mailto:ler...@redhat.com%3cmailto:ler...@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 <ler...@redhat.com<mailto:ler...@redhat.com<mailto:ler...@redhat.com%3cmailto:ler...@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
