On 10/26/17 03:41, Wang, Jian J wrote:
> Please see my comments below. Thanks.
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Wednesday, October 25, 2017 8:51 PM
>> To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org
>> Cc: Yao, Jiewen <jiewen....@intel.com>; Dong, Eric <eric.d...@intel.com>
>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/CpuDxe: Fix multiple entries of
>> RT_CODE in memory map
>>
>> On 10/25/17 03:33, Wang, Jian J wrote:
>>> Hi Laszlo,
>>>
>>> Thanks for the feedback. I'd like to explain a bit more here first and
>>> will update the commit message later.
>>>
>>> The multiple RT_CODE entries issue was reported by LUV test suite from
>>> https://01.org/linux-uefi-validation
>>>
>>> You're right this issue is caused by my commit c1cab54ce57c, which
>>> tried to fix the GCD issue in which you can't set paging related
>>> memory attributes through GCD service API. The reasons are that GCD
>>> will filter out all paging related memory attributes and also, the CPU
>>> driver didn't sync the paging attributes back to GCD. Sorry I don't
>>> know why GCD and cpu driver are implemented that way.
>>>
>>> My previous commit c1cab54ce57c fixed above issues but didn't make
>>> sure that all memory blocks share the same capabilities because I just
>>> added paging related memory capabilities to those memory block having
>>> some paging attributes set. This will in turn cause more than one
>>> RT_CODE entries in memory map because GCD reports memory to OS based
>>> on the memory block capability.
>>>
>>> Why multiple RT_CODE matters? It's because that Linux kernel would
>>> misplace the runtime service code segment and its data segment. What I
>>> know is this Linux issue had been fixed. That's why recent Linux
>>> distro won't encounter problems even if we report multiple RT_CODE
>>> memory to kernel.
>>
>> Ah, OK, I remember now.
>>
>> The point is that separate entries in the UEFI memmap may be mapped by
>> the OS to discontiguous virtual address ranges.
>>
>> However, if we take the UEFI memmap entries that belong to a single
>> runtime DXE driver, and unintentionally split those entries up (by
>> setting distinct capabilities for a subset of their pages), then the
>> runtime driver will break, because the linear address range that the
>> driver expects (from its original loading and relocation) will not be
>> kept linear by the OS.
>>
>>> I'm sorry that I cannot find the specific version of kernel which has
>>> such problem and I can't find any discussion related. Maybe Jiewen can
>>> provide more detailed information.
>>
>> It would be really helpful if you guys could name a guest kernel version
>> (or a GNU/Linux distro release) that is affected by this problem.
>>
>
> It seems following log history which mentioned the problem.
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=0ce3cc008ec04258b6a6314b09f1a6012810881a
Ah, exactly. I vaguely remembered that the same issue had originally
popped up in connection to the properties table.
Commit 0ce3cc008ec0 ("arm64/efi: Fix boot crash by not padding between
EFI_MEMORY_RUNTIME regions", 2015-09-25) was first released as part of
Linux v4.3.
>
>> Are there perhaps any conditions that this issue depends on, such as
>> PCDs for example? In other words, is it possible that various edk2
>> platform settings (in the DSC) can mask this issue?
>>
>
> In current code base, following PCDs may cause memory page attribute changes
>
> PcdImageProtectionPolicy
> PcdDxeNxMemoryProtectionPolicy
>
> Once the heap guard feature is checked in (you may notice my recent patches),
> there're three more PCDs:
>
> PcdHeapGuardPropertyMask
> PcdHeapGuardPoolType
> PcdHeapGuardPageType
>
>> Can you maybe present UEFI memmaps (dumped in the UEFI shell, or in
>> Linux) that show the problem, compared to this fix?
>>
>
> Before this patch (after c1cab54ce57c), the memory map looks like
>
> RT_Data 00000000BE20F000-00000000BE38EFFF 0000000000000180 800000000000000F
> RT_Code 00000000BE38F000-00000000BE46FFFF 00000000000000E1 800000000000000F
> RT_Code 00000000BE470000-00000000BE470FFF 0000000000000001 800000000000400F
> RT_Code 00000000BE471000-00000000BE472FFF 0000000000000002 800000000002000F
> RT_Code 00000000BE473000-00000000BE476FFF 0000000000000004 800000000000400F
> RT_Code 00000000BE477000-00000000BE478FFF 0000000000000002 800000000002000F
> RT_Code 00000000BE479000-00000000BE47CFFF 0000000000000004 800000000000400F
> RT_Code 00000000BE47D000-00000000BE47FFFF 0000000000000003 800000000002000F
> RT_Code 00000000BE480000-00000000BE483FFF 0000000000000004 800000000000400F
> RT_Code 00000000BE484000-00000000BE485FFF 0000000000000002 800000000002000F
> RT_Code 00000000BE486000-00000000BE489FFF 0000000000000004 800000000000400F
> RT_Code 00000000BE48A000-00000000BE48BFFF 0000000000000002 800000000002000F
> RT_Code 00000000BE48C000-00000000BE48EFFF 0000000000000003 800000000000400F
>
> You may notice that there're one RT_Data with 12 RT_Code entries listed.
> After this patch, it will be
>
> RT_Data 00000000BE20F000-00000000BE38EFFF 0000000000000180 800000000002600F
> RT_Code 00000000BE38F000-00000000BE48EFFF 0000000000000100 800000000002600F
>
>>> This patch will make sure that all memory block share the same paging
>>> capabilities. Because all memory are actually paged in current EDK2
>>> (at least IA processors), technically we're capable of setting any
>>> page of memory to read-only and/or non-executable. I think this fix is
>>> not only trying to avoid multiple RT_CODE memory map entries but also
>>> trying to make sure the memory capabilities in GCD service to reflect
>>> complete status of the real world.
>>
>> Are you saying that *any* firmware that carries commit c1cab54ce57c
>> should also receive this patch?
>>
>
> Yes.
>
>> If that's the case, then some kind of "reproducer" would be really nice
>> -- steps that you can run both with and without this patch, and the
>> output or the behavior will show the difference.
>>
>
> PcdImageProtectionPolicy is enabled by default. It can be "naturally"
> reproduced.
Thank you, Jian, your answers are very helpful!
Laszlo
>>>> -----Original Message-----
>>>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>>>> Sent: Tuesday, October 24, 2017 8:20 PM
>>>> To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org
>>>> Cc: Yao, Jiewen <jiewen....@intel.com>; Dong, Eric <eric.d...@intel.com>
>>>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/CpuDxe: Fix multiple entries of
>>>> RT_CODE in memory map
>>>>
>>>> On 10/23/17 08:50, Jian J Wang wrote:
>>>>> 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.
>>>>>
>>>>> Cc: Eric Dong <eric.d...@intel.com>
>>>>> Cc: Jiewen Yao <jiewen....@intel.com>
>>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>>> Signed-off-by: Jian J Wang <jian.j.w...@intel.com>
>>>>> ---
>>>>> UefiCpuPkg/CpuDxe/CpuPageTable.c | 14 +++++++++++---
>>>>> 1 file changed, 11 insertions(+), 3 deletions(-)
>>>>
>>>> Can you please explain in the commit message; what OSes are affected by
>>>> this issue, to your knowledge?
>>>>
>>>> Also, the code being patched seems to originate from commit c1cab54ce57c
>>>> ("UefiCpuPkg/CpuDxe: Fix out-of-sync issue in page attributes",
>>>> 2017-09-16). I vaguely recall that this commit was related to your "page
>>>> 0 protection" work.
>>>>
>>>> Can you please explain, in the commit message, under what circumstances
>>>> (PCD settings etc) the issue arises, and how we end up with multiple
>>>> RT_CODE entries in the memory map?
>>>>
>>>> (BTW, multiple RT_CODE entries in the memmap should be perfectly
>>>> normal... So I'm extra curious about the OSes that are not compatible
>>>> with that.)
>>>>
>>>> Thanks,
>>>> Laszlo
>>>>
>>>>> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
>>>> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
>>>>> index d312eb66f8..0802464b9d 100644
>>>>> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
>>>>> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
>>>>> @@ -829,6 +829,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 +855,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 +862,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));
>>>>>
>>>
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
