Re: [edk2-devel] [PATCH 35/35] UefiPayloadPkg/BlSupportDxe: fix ReserveResourceInGcd() calls

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 9/23/19 6:02 PM, Dong, Guo wrote:
> 
> This is not dead code. 
> This actual bug didn't cause issues since BlSupportDxe just allocate resources reported from bootloaders. 

Ah OK, thanks Guo.

> Anyway, this is a great enhancement from spec to capture such bugs.
> 
> Thanks,
> Guo
> 
>> -----Original Message-----
>> From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
>> Sent: Monday, September 23, 2019 8:08 AM
>> To: devel@edk2.groups.io; lersek@redhat.com
>> Cc: You, Benjamin <benjamin.you@intel.com>; Dong, Guo
>> <guo.dong@intel.com>; Ma, Maurice <maurice.ma@intel.com>
>> Subject: Re: [edk2-devel] [PATCH 35/35] UefiPayloadPkg/BlSupportDxe: fix
>> ReserveResourceInGcd() calls
>>
>> On 9/17/19 9:49 PM, Laszlo Ersek wrote:
>>> The last parameter of ReserveResourceInGcd() is "ImageHandle",
>>> forwarded in turn to gDS->AllocateMemorySpace() or gDS-
>>> AllocateIoSpace() as "owner"
>>> image handle.
>>>
>>> But BlDxeEntryPoint() passes "SystemTable" as "ImageHandle".
>>>
>>> Compilers have not flagged it because EFI_HANDLE (the type of
>>> "ImageHandle") is unfortunately specified as (VOID*), and
>>> (EFI_SYSTEM_TABLE*) converts to (VOID*) silently.
>>>
>>> Hand the entry point function's "ImageHandle" parameter to
>>> ReserveResourceInGcd(). This fixes an actual bug.
>>
>> Wow very buggy, so I assume this is mostly dead code, right?
>>
>>> Cc: Benjamin You <benjamin.you@intel.com>
>>> Cc: Guo Dong <guo.dong@intel.com>
>>> Cc: Maurice Ma <maurice.ma@intel.com>
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>
>>> Notes:
>>>     build-tested only
>>>
>>>  UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
>>> b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
>>> index bcee4cd9bc41..28dfc8fc5545 100644
>>> --- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
>>> +++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
>>> @@ -106,10 +106,10 @@ BlDxeEntryPoint (
>>>    //
>>>    // Report MMIO/IO Resources
>>>    //
>>> -  Status = ReserveResourceInGcd (TRUE,
>>> EfiGcdMemoryTypeMemoryMappedIo, 0xFEC00000, SIZE_4KB, 0,
>> SystemTable);
>>> // IOAPIC
>>> +  Status = ReserveResourceInGcd (TRUE,
>>> + EfiGcdMemoryTypeMemoryMappedIo, 0xFEC00000, SIZE_4KB, 0,
>>> + ImageHandle); // IOAPIC
>>>    ASSERT_EFI_ERROR (Status);
>>>
>>> -  Status = ReserveResourceInGcd (TRUE,
>>> EfiGcdMemoryTypeMemoryMappedIo, 0xFED00000, SIZE_1KB, 0,
>> SystemTable);
>>> // HPET
>>> +  Status = ReserveResourceInGcd (TRUE,
>>> + EfiGcdMemoryTypeMemoryMappedIo, 0xFED00000, SIZE_1KB, 0,
>>> + ImageHandle); // HPET
>>>    ASSERT_EFI_ERROR (Status);
>>>
>>>    //
>>>
>>
>> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>