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

[Laszlo Ersek <lersek@redhat.com>]

On 11/08/17 01:10, Wang, Jian J wrote:
> Hi Laszlo,
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Wednesday, November 08, 2017 1:14 AM
>> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
>> Cc: 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
>>
>> sorry about the late response
>>
>> On 11/03/17 01:57, Jian J Wang wrote:
>>>> 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;
>>>      }
>>
>> When exactly do the new conditions match?
>>
>> I thought the base addresses and the lengths in the GCD memory space map
>> are all page aligned. Is that not the case?
>>
>> If these conditions are just a sanity check (i.e. we never expect them
>> to fire), then should we perpahs turn them into ASSERT()s?
>>
> 
> I found that there's a mmio entry in memory map on OVMF which has size
> less than a page. I didn't encounter this before. Maybe some recent changes
> in other part of EDKII caused this situation. So ASSERT is not enough.

Can you describe the base address and size of this MMIO entry?

I don't see how it is possible to add such an entry in the first place
-- if you try to add it in PEI, via a HOB, then IIRC HobLib will
ASSERT(). If you try to add it with gDS->AddMemorySpace() in DXE, then
the call should fail.

I believe it might be useful to investigate this entry more closely.
Knowing the base address could help us.

Thanks!
Laszlo