Re: [edk2-devel] [PATCH 35/35] UefiPayloadPkg/BlSupportDxe: fix ReserveResourceInGcd() calls
On 09/23/19 17:08, Philippe Mathieu-Daudé wrote: > 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? Not necessarily; as long as noone tries to use the "owner" image handle in practice, the issue may remain dormant. Thanks Laszlo >> Cc: Benjamin You >> Cc: Guo Dong >> Cc: Maurice Ma >> Signed-off-by: Laszlo Ersek >> --- >> >> 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, >> 0xFEC0, SIZE_4KB, 0, SystemTable); // IOAPIC >> + Status = ReserveResourceInGcd (TRUE, EfiGcdMemoryTypeMemoryMappedIo, >> 0xFEC0, SIZE_4KB, 0, ImageHandle); // IOAPIC >>ASSERT_EFI_ERROR (Status); >> >> - Status = ReserveResourceInGcd (TRUE, EfiGcdMemoryTypeMemoryMappedIo, >> 0xFED0, SIZE_1KB, 0, SystemTable); // HPET >> + Status = ReserveResourceInGcd (TRUE, EfiGcdMemoryTypeMemoryMappedIo, >> 0xFED0, SIZE_1KB, 0, ImageHandle); // HPET >>ASSERT_EFI_ERROR (Status); >> >>// >> > > Reviewed-by: Philippe Mathieu-Daude > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#47973): https://edk2.groups.io/g/devel/message/47973 Mute This Topic: https://groups.io/mt/34180240/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 35/35] UefiPayloadPkg/BlSupportDxe: fix ReserveResourceInGcd() calls
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:phi...@redhat.com] >> Sent: Monday, September 23, 2019 8:08 AM >> To: devel@edk2.groups.io; ler...@redhat.com >> Cc: You, Benjamin ; Dong, Guo >> ; Ma, Maurice >> 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 >>> Cc: Guo Dong >>> Cc: Maurice Ma >>> Signed-off-by: Laszlo Ersek >>> --- >>> >>> 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, 0xFEC0, SIZE_4KB, 0, >> SystemTable); >>> // IOAPIC >>> + Status = ReserveResourceInGcd (TRUE, >>> + EfiGcdMemoryTypeMemoryMappedIo, 0xFEC0, SIZE_4KB, 0, >>> + ImageHandle); // IOAPIC >>>ASSERT_EFI_ERROR (Status); >>> >>> - Status = ReserveResourceInGcd (TRUE, >>> EfiGcdMemoryTypeMemoryMappedIo, 0xFED0, SIZE_1KB, 0, >> SystemTable); >>> // HPET >>> + Status = ReserveResourceInGcd (TRUE, >>> + EfiGcdMemoryTypeMemoryMappedIo, 0xFED0, SIZE_1KB, 0, >>> + ImageHandle); // HPET >>>ASSERT_EFI_ERROR (Status); >>> >>>// >>> >> >> Reviewed-by: Philippe Mathieu-Daude -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#47860): https://edk2.groups.io/g/devel/message/47860 Mute This Topic: https://groups.io/mt/34180240/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 35/35] UefiPayloadPkg/BlSupportDxe: fix ReserveResourceInGcd() calls
This is not dead code. This actual bug didn't cause issues since BlSupportDxe just allocate resources reported from bootloaders. Anyway, this is a great enhancement from spec to capture such bugs. Thanks, Guo > -Original Message- > From: Philippe Mathieu-Daudé [mailto:phi...@redhat.com] > Sent: Monday, September 23, 2019 8:08 AM > To: devel@edk2.groups.io; ler...@redhat.com > Cc: You, Benjamin ; Dong, Guo > ; Ma, Maurice > 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 > > Cc: Guo Dong > > Cc: Maurice Ma > > Signed-off-by: Laszlo Ersek > > --- > > > > 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, 0xFEC0, SIZE_4KB, 0, > SystemTable); > > // IOAPIC > > + Status = ReserveResourceInGcd (TRUE, > > + EfiGcdMemoryTypeMemoryMappedIo, 0xFEC0, SIZE_4KB, 0, > > + ImageHandle); // IOAPIC > >ASSERT_EFI_ERROR (Status); > > > > - Status = ReserveResourceInGcd (TRUE, > > EfiGcdMemoryTypeMemoryMappedIo, 0xFED0, SIZE_1KB, 0, > SystemTable); > > // HPET > > + Status = ReserveResourceInGcd (TRUE, > > + EfiGcdMemoryTypeMemoryMappedIo, 0xFED0, SIZE_1KB, 0, > > + ImageHandle); // HPET > >ASSERT_EFI_ERROR (Status); > > > >// > > > > Reviewed-by: Philippe Mathieu-Daude -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#47858): https://edk2.groups.io/g/devel/message/47858 Mute This Topic: https://groups.io/mt/34180240/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
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 > Cc: Guo Dong > Cc: Maurice Ma > Signed-off-by: Laszlo Ersek > --- > > 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, > 0xFEC0, SIZE_4KB, 0, SystemTable); // IOAPIC > + Status = ReserveResourceInGcd (TRUE, EfiGcdMemoryTypeMemoryMappedIo, > 0xFEC0, SIZE_4KB, 0, ImageHandle); // IOAPIC >ASSERT_EFI_ERROR (Status); > > - Status = ReserveResourceInGcd (TRUE, EfiGcdMemoryTypeMemoryMappedIo, > 0xFED0, SIZE_1KB, 0, SystemTable); // HPET > + Status = ReserveResourceInGcd (TRUE, EfiGcdMemoryTypeMemoryMappedIo, > 0xFED0, SIZE_1KB, 0, ImageHandle); // HPET >ASSERT_EFI_ERROR (Status); > >// > Reviewed-by: Philippe Mathieu-Daude -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#47855): https://edk2.groups.io/g/devel/message/47855 Mute This Topic: https://groups.io/mt/34180240/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 35/35] UefiPayloadPkg/BlSupportDxe: fix ReserveResourceInGcd() calls
Thanks for the fix. Reviewed-by: Guo Dong > -Original Message- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Laszlo Ersek > Sent: Tuesday, September 17, 2019 12:50 PM > To: edk2-devel-groups-io > Cc: You, Benjamin ; Dong, Guo > ; Ma, Maurice > Subject: [edk2-devel] [PATCH 35/35] UefiPayloadPkg/BlSupportDxe: fix > ReserveResourceInGcd() calls > > 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. > > Cc: Benjamin You > Cc: Guo Dong > Cc: Maurice Ma > Signed-off-by: Laszlo Ersek > --- > > 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, 0xFEC0, SIZE_4KB, 0, > SystemTable); // IOAPIC > + Status = ReserveResourceInGcd (TRUE, > EfiGcdMemoryTypeMemoryMappedIo, > + 0xFEC0, SIZE_4KB, 0, ImageHandle); // IOAPIC >ASSERT_EFI_ERROR (Status); > > - Status = ReserveResourceInGcd (TRUE, > EfiGcdMemoryTypeMemoryMappedIo, 0xFED0, SIZE_1KB, 0, > SystemTable); // HPET > + Status = ReserveResourceInGcd (TRUE, > EfiGcdMemoryTypeMemoryMappedIo, > + 0xFED0, SIZE_1KB, 0, ImageHandle); // HPET >ASSERT_EFI_ERROR (Status); > >// > -- > 2.19.1.3.g30247aa5d201 > > > -=-=-=-=-=-= > Groups.io Links: You receive all messages sent to this group. > > View/Reply Online (#47422): https://edk2.groups.io/g/devel/message/47422 > Mute This Topic: https://groups.io/mt/34180240/1781375 > Group Owner: devel+ow...@edk2.groups.io > Unsubscribe: https://edk2.groups.io/g/devel/unsub [guo.d...@intel.com] > -=-=-=-=-=-= -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#47784): https://edk2.groups.io/g/devel/message/47784 Mute This Topic: https://groups.io/mt/34180240/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH 35/35] UefiPayloadPkg/BlSupportDxe: fix ReserveResourceInGcd() calls
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. Cc: Benjamin You Cc: Guo Dong Cc: Maurice Ma Signed-off-by: Laszlo Ersek --- 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, 0xFEC0, SIZE_4KB, 0, SystemTable); // IOAPIC + Status = ReserveResourceInGcd (TRUE, EfiGcdMemoryTypeMemoryMappedIo, 0xFEC0, SIZE_4KB, 0, ImageHandle); // IOAPIC ASSERT_EFI_ERROR (Status); - Status = ReserveResourceInGcd (TRUE, EfiGcdMemoryTypeMemoryMappedIo, 0xFED0, SIZE_1KB, 0, SystemTable); // HPET + Status = ReserveResourceInGcd (TRUE, EfiGcdMemoryTypeMemoryMappedIo, 0xFED0, SIZE_1KB, 0, ImageHandle); // HPET ASSERT_EFI_ERROR (Status); // -- 2.19.1.3.g30247aa5d201 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#47422): https://edk2.groups.io/g/devel/message/47422 Mute This Topic: https://groups.io/mt/34180240/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
