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

[Laszlo Ersek <lersek@redhat.com>]

On 11/09/17 02:48, Yao, Jiewen wrote:
> FED00000 is HPET region. It is MMIO.

Right, we add it in OvmfPkg/PlatformPei/Platform.c:

    //
    // address       purpose   size
    // ------------  --------  -------------------------
    ...
    // 0xFED00000    HPET                           1 KB
    ...
    AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB);

Because this is MMIO and not system memory, I think the 1KB size is
valid, as far as a resource descriptor HOB is concerned.

This goes on to support my earlier suggestion to check the GCD memory
space entry more strictly, for its type.

Anyway, the latest v3 (v4?) approach can handle the entry just as well,
so I don't insist on modifying the entry type check. I was mainly
curious if we did something wrong in OVMF. I believe the above HOB is
valid though.

Thanks
Laszlo


>> -----Original Message-----
>> From: Wang, Jian J
>> Sent: Thursday, November 9, 2017 8:42 AM
>> To: Laszlo Ersek <lersek@redhat.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
>>
>> Hi Laszlo,
>>
>> The memory range is 00000000FED00000 - 00000000FED003FF. Actually I don't
>> know if it's for MMIO or not. I might be mess it with other things.
>>
>> Thanks,
>> Jian
>>> -----Original Message-----
>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>> Sent: Wednesday, November 08, 2017 10:17 PM
>>> 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
>>>
>>> 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