* [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