Re: [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map

[Laszlo Ersek <lersek@redhat.com>]

On 11/08/17 04:13, Zeng, Star wrote:
> https://bugzilla.tianocore.org/show_bug.cgi?id=763 is submitted to track this.

I'd like to ask a question about the impact of BZ#763:

Regarding BZ#753 ("UEFI memory map regression (runtime code entry
splitting) introduced by c1cab54ce57c"), we seem to have agreed that any
firmware that has commit c1cab54ce57c will need the fix for BZ#753.
Otherwise the OS may get an invalid UEFI memory map, and if the OS is
not specifically prepared for that, it can crash.

My question is if the issue described in BZ#763 is similarly serious;
i.e., whether any firmware that has commit 14dde9e903bb
("MdeModulePkg/Core: Fix out-of-sync issue in GCD", 2017-09-19) should
urgently get the fix for BZ#763.

In other words, does BZ#763 have a direct OS-level impact that needs to
be fixed quickly?

Thanks!
Laszlo


> -----Original Message-----
> From: Wang, Jian J 
> Sent: Tuesday, November 7, 2017 8:55 AM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
> 
> Thanks for the review. And I agree that GCD.SetMemoryAttributes should be used all the time in DxeCore. Let's fix it in another patch.
> 
>> -----Original Message-----
>> From: Zeng, Star
>> Sent: Monday, November 06, 2017 5:16 PM
>> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
>> Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen 
>> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star 
>> <star.zeng@intel.com>
>> Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries 
>> of RT_CODE in memory map
>>
>> I am ok to this code approach.
>>
>> Acknowledged-by: Star Zeng <star.zeng@intel.com>
>>
>> Besides, I think except GCD services, DxeCore should not call gCpu-
>>> SetMemoryAttributes() directly, for example 
>>> ApplyMemoryProtectionPolicy(), it
>> should have no assumption of the CPU code (to set capabilities), and 
>> call GCD setcapabilities first, and then call GCD setattributes since
>> 14dde9e903bb9a719ebb8f3381da72b19509bc36 "MdeModulePkg/Core: Fix out- 
>> of-sync issue in GCD", otherwise there may be mismatch of page 
>> attributes between GCD and gCPU after 
>> RefreshGcdMemoryAttributesFromPaging() by the calling gCpu->SetMemoryAttributes() in ApplyMemoryProtectionPolicy().
>>
>> Anyway, that could be in a separated patch. :)
>>
>> Thanks,
>> Star
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
>> Jian J Wang
>> Sent: Friday, November 3, 2017 8:57 AM
>> To: edk2-devel@lists.01.org
>> Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen 
>> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
>> Subject: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of 
>> RT_CODE in memory map
>>
>>> v2
>>> a. Fix an issue which will cause setting capability failure if size is smaller
>>>    than a page.
>>
>> More than one entry of RT_CODE memory might cause boot problem for 
>> some old OSs. This patch will fix this issue to keep OS compatibility as much as possible.
>>
>> More detailed information, please refer to
>>     https://bugzilla.tianocore.org/show_bug.cgi?id=753
>>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
>> ---
>>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++++++++++++++----
>>  1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
>> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
>> index d312eb66f8..4a7827ebc9 100644
>> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
>> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
>> @@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging (
>>    PageLength    = 0;
>>
>>    for (Index = 0; Index < NumberOfDescriptors; Index++) {
>> -    if (MemorySpaceMap[Index].GcdMemoryType ==
>> EfiGcdMemoryTypeNonExistent) {
>> +    if (MemorySpaceMap[Index].GcdMemoryType ==
>> EfiGcdMemoryTypeNonExistent
>> +        || (MemorySpaceMap[Index].BaseAddress & EFI_PAGE_MASK) != 0
>> +        || (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0) {
>>        continue;
>>      }
>>
>> @@ -829,6 +831,15 @@ RefreshGcdMemoryAttributesFromPaging (
>>      // Sync real page attributes to GCD
>>      BaseAddress       = MemorySpaceMap[Index].BaseAddress;
>>      MemorySpaceLength = MemorySpaceMap[Index].Length;
>> +    Capabilities      = MemorySpaceMap[Index].Capabilities |
>> +                        EFI_MEMORY_PAGETYPE_MASK;
>> +    Status = gDS->SetMemorySpaceCapabilities (
>> +                    BaseAddress,
>> +                    MemorySpaceLength,
>> +                    Capabilities
>> +                    );
>> +    ASSERT_EFI_ERROR (Status);
>> +
>>      while (MemorySpaceLength > 0) {
>>        if (PageLength == 0) {
>>          PageEntry = GetPageTableEntry (&PagingContext, BaseAddress, 
>> &PageAttribute); @@ -846,7 +857,6 @@ 
>> RefreshGcdMemoryAttributesFromPaging (
>>          if (Attributes != (MemorySpaceMap[Index].Attributes &
>> EFI_MEMORY_PAGETYPE_MASK)) {
>>            DoUpdate = TRUE;
>>            Attributes |= (MemorySpaceMap[Index].Attributes & 
>> ~EFI_MEMORY_PAGETYPE_MASK);
>> -          Capabilities = Attributes | MemorySpaceMap[Index].Capabilities;
>>          } else {
>>            DoUpdate = FALSE;
>>          }
>> @@ -854,8 +864,8 @@ RefreshGcdMemoryAttributesFromPaging (
>>
>>        Length = MIN (PageLength, MemorySpaceLength);
>>        if (DoUpdate) {
>> -        gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
>> -        gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
>> +        Status = gDS->SetMemorySpaceAttributes (BaseAddress, Length,
>> Attributes);
>> +        ASSERT_EFI_ERROR (Status);
>>          DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] 
>> %016lx -  %016lx (%08lx -> %08lx)\r\n",
>>                               Index, BaseAddress, BaseAddress + Length - 1,
>>                               MemorySpaceMap[Index].Attributes, 
>> Attributes));
>> --
>> 2.14.1.windows.1
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel