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

[Laszlo Ersek <lersek@redhat.com>]

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
>