Re: [PATCH] UefiCpuPkg:PiSmmCpu: Set correct attribute on split.

["Yao, Jiewen" <jiewen.yao@intel.com>]

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