* Re: [patch 2/2] MdeModulePkg/BmBoot: Report status when fail to load/start boot option
2019-02-20 1:19 ` Laszlo Ersek
@ 2019-02-20 1:36 ` Laszlo Ersek
2019-02-20 2:21 ` Ni, Ray
2019-02-20 2:35 ` Bi, Dandan
2 siblings, 0 replies; 12+ messages in thread
From: Laszlo Ersek @ 2019-02-20 1:36 UTC (permalink / raw)
To: Dandan Bi; +Cc: edk2-devel, Hao Wu, Ruiyu Ni
On 02/20/19 02:19, Laszlo Ersek wrote:
> Hi Dandan,
>
> On 02/15/19 09:51, Dandan Bi wrote:
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1398
>>
>> According to PI1.7 Spec, report extended data describing an
>> EFI_STATUS return value along with
>> EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR and
>> EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED status code
>> when fail to load or start boot option image.
> Unfortunately, this patch is not good; we made a mistake here.
> I'll report the same in a TianoCore BZ, and will try to submit a patch
> as well.
https://bugzilla.tianocore.org/show_bug.cgi?id=1539
Thanks
Laszlo
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch 2/2] MdeModulePkg/BmBoot: Report status when fail to load/start boot option
2019-02-20 1:19 ` Laszlo Ersek
2019-02-20 1:36 ` Laszlo Ersek
@ 2019-02-20 2:21 ` Ni, Ray
2019-02-20 9:24 ` Laszlo Ersek
2019-02-20 2:35 ` Bi, Dandan
2 siblings, 1 reply; 12+ messages in thread
From: Ni, Ray @ 2019-02-20 2:21 UTC (permalink / raw)
To: 'Laszlo Ersek', Bi, Dandan; +Cc: edk2-devel@lists.01.org, Wu, Hao A
Laszlo,
Thanks for catching this.
GenPerformMemoryTest() in
MdeModulePkg\Universal\MemoryTest\GenericMemoryTestDxe\LightMemoryTest.c
uses the same technics as you suggested.
I give up to propose another option: having pack(1) for the new status structure.
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, February 20, 2019 9:19 AM
> To: Bi, Dandan <dandan.bi@intel.com>
> Cc: edk2-devel@lists.01.org; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
> <ray.ni@intel.com>
> Subject: Re: [edk2] [patch 2/2] MdeModulePkg/BmBoot: Report status when
> fail to load/start boot option
>
> Hi Dandan,
>
> On 02/15/19 09:51, Dandan Bi wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1398
> >
> > According to PI1.7 Spec, report extended data describing an EFI_STATUS
> > return value along with EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR
> and
> > EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED status code when fail to load
> or
> > start boot option image.
> >
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Hao Wu <hao.a.wu@intel.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Dandan Bi <dandan.bi@intel.com>
> > ---
> > .../Library/UefiBootManagerLib/BmBoot.c | 22 ++++++++++++++-----
> > 1 file changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > index 6444fb43eb..9be1633b74 100644
> > --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > @@ -1818,15 +1818,20 @@ EfiBootManagerBoot (
> > FreePool (FilePath);
> > }
> >
> > if (EFI_ERROR (Status)) {
> > //
> > - // Report Status Code to indicate that the failure to load boot option
> > + // Report Status Code with the failure status to indicate that
> > + the failure to load boot option
> > //
> > - REPORT_STATUS_CODE (
> > + REPORT_STATUS_CODE_EX (
> > EFI_ERROR_CODE | EFI_ERROR_MINOR,
> > - (EFI_SOFTWARE_DXE_BS_DRIVER |
> EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR)
> > + (EFI_SOFTWARE_DXE_BS_DRIVER |
> EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR),
> > + 0,
> > + NULL,
> > + NULL,
> > + &Status,
> > + sizeof (EFI_STATUS)
> > );
> > BootOption->Status = Status;
> > //
> > // Destroy the RAM disk
> > //
> > @@ -1902,15 +1907,20 @@ EfiBootManagerBoot (
> > Status = gBS->StartImage (ImageHandle, &BootOption->ExitDataSize,
> &BootOption->ExitData);
> > DEBUG ((DEBUG_INFO | DEBUG_LOAD, "Image Return Status = %r\n",
> Status));
> > BootOption->Status = Status;
> > if (EFI_ERROR (Status)) {
> > //
> > - // Report Status Code to indicate that boot failure
> > + // Report Status Code with the failure status to indicate that
> > + boot failure
> > //
> > - REPORT_STATUS_CODE (
> > + REPORT_STATUS_CODE_EX (
> > EFI_ERROR_CODE | EFI_ERROR_MINOR,
> > - (EFI_SOFTWARE_DXE_BS_DRIVER |
> EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED)
> > + (EFI_SOFTWARE_DXE_BS_DRIVER |
> EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED),
> > + 0,
> > + NULL,
> > + NULL,
> > + &Status,
> > + sizeof (EFI_STATUS)
> > );
> > }
> > PERF_END_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32)
> > OptionNumber);
> >
> > //
> >
>
> Unfortunately, this patch is not good; we made a mistake here.
>
> Consider the EFI_RETURN_STATUS_EXTENDED_DATA structure, added in
> patch
> #1:
>
> > typedef struct {
> > ///
> > /// The data header identifying the data:
> > /// DataHeader.HeaderSize should be sizeof(EFI_STATUS_CODE_DATA),
> > /// DataHeader.Size should be
> sizeof(EFI_RETURN_STATUS_EXTENDED_DATA) - HeaderSize,
> > /// DataHeader.Type should be EFI_STATUS_CODE_SPECIFIC_DATA_GUID.
> > ///
> > EFI_STATUS_CODE_DATA DataHeader;
> > ///
> > /// The EFI_STATUS return value of the service or function whose failure
> triggered the
> > /// reporting of the status code (generally an error code or a debug code).
> > ///
> > EFI_STATUS ReturnStatus;
> > } EFI_RETURN_STATUS_EXTENDED_DATA;
>
> According to the UEFI spec, unless specified otherwise, structure members
> are aligned naturally.
>
> And, the PI spec references the UEFI spec with regard to data types.
>
> Accordingly, when this structure is built for X64, the size of this structure is 32
> bytes, and the offset of ReturnStatus is 24. There is a 4-byte padding
> between DataHeader (which is 20 bytes in size) and the ReturnStatus field.
> DataHeader has type
>
> > typedef struct {
> > ///
> > /// The size of the structure. This is specified to enable future expansion.
> > ///
> > UINT16 HeaderSize;
> > ///
> > /// The size of the data in bytes. This does not include the size of the
> header structure.
> > ///
> > UINT16 Size;
> > ///
> > /// The GUID defining the type of the data.
> > ///
> > EFI_GUID Type;
> > } EFI_STATUS_CODE_DATA;
>
> which extends to 20 bytes.
>
> I'm working on patches that capture / process
> EFI_RETURN_STATUS_EXTENDED_DATA. The fields I'm seeing in DataHeader
> are (on X64):
> - HeaderSize = 0x14 (20 decimal)
> - Size = 0x8,
> - Type = {
> Data1 = 0x335984bd,
> Data2 = 0xe805,
> Data3 = 0x409a,
> Data4 = {0xb8, 0xf8, 0xd2, 0x7e, 0xce, 0x5f, 0xf7, 0xa6}
> }
>
> The "DataHeader.Size" field is incorrect. It should be 12 (that is, 32-20),
> according to the documentation:
>
> > /// DataHeader.Size should be
> > sizeof(EFI_RETURN_STATUS_EXTENDED_DATA) - HeaderSize,
>
> I think in the code above, we should use a temporary
> EFI_RETURN_STATUS_EXTENDED_DATA structure, zero it out, then set the
> ReturnStatus field in it. Finally, call the REPORT_STATUS_CODE_EX () macro
> with the trailing portion of this temporary object.
>
> I'll report the same in a TianoCore BZ, and will try to submit a patch as well.
>
> I'm sorry that I didn't catch this in review.
>
> Thanks
> Laszlo
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [patch 2/2] MdeModulePkg/BmBoot: Report status when fail to load/start boot option
2019-02-20 2:21 ` Ni, Ray
@ 2019-02-20 9:24 ` Laszlo Ersek
2019-02-20 17:19 ` Doran, Mark
0 siblings, 1 reply; 12+ messages in thread
From: Laszlo Ersek @ 2019-02-20 9:24 UTC (permalink / raw)
To: Ni, Ray, Bi, Dandan; +Cc: edk2-devel@lists.01.org, Wu, Hao A, Doran, Mark
+Mark, for my comments on the process
On 02/20/19 03:21, Ni, Ray wrote:
> Laszlo,
> Thanks for catching this.
>
> GenPerformMemoryTest() in
> MdeModulePkg\Universal\MemoryTest\GenericMemoryTestDxe\LightMemoryTest.c
> uses the same technics as you suggested.
>
> I give up to propose another option: having pack(1) for the new status structure.
I think that byte-packing EFI_RETURN_STATUS_EXTENDED_DATA in the PI-1.7
spec would have been viable, but we should have thought about that in
advance. The PI and UEFI specs require natural alignment for structure
members, unless stated otherwise. There *are* a number of examples of
"stated otherwise" (the term that the specs use is frequently
"byte-packed structure"). Again, we should have thought of that in
advance, before PI-1.7 was released, so now we can only fix the way the
object is populated.
More generally, I think this demonstrates a flaw in the standardization
process. PIWG and USWG should only standardize existing practice; that
is, features that have at least one functional implementation. (Not
necessarily open source, but the interfaces should be battle-hardened
*first*.) The restriction where we can't even propose patches for edk2
before the standards are updated is very counter-productive. There's
practically zero chance for getting an actual implementation
peer-reviewed and peer-tested before proposing the API for the standard.
Personally I have suggested in the past to implement some new features
as edk2 extensions first, and once they work, to upstream them to the
standards. This idea is usually rejected by maintainers, because
maintainers worry about becoming stuck with more and more edk2
extensions to core code (i.e., they worry about the feature proponents
not making good on their promise to standardize). Unfortunately, the
alternative (= the current practice) is that we standardize speculative
interfaces, which then prove suboptimal once we implement them.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch 2/2] MdeModulePkg/BmBoot: Report status when fail to load/start boot option
2019-02-20 9:24 ` Laszlo Ersek
@ 2019-02-20 17:19 ` Doran, Mark
2019-02-21 8:55 ` Laszlo Ersek
0 siblings, 1 reply; 12+ messages in thread
From: Doran, Mark @ 2019-02-20 17:19 UTC (permalink / raw)
To: Laszlo Ersek, Ni, Ray, Bi, Dandan; +Cc: edk2-devel@lists.01.org, Wu, Hao A
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, February 20, 2019 1:25 AM
> To: Ni, Ray <ray.ni@intel.com>; Bi, Dandan <dandan.bi@intel.com>
> Cc: edk2-devel@lists.01.org; Wu, Hao A <hao.a.wu@intel.com>; Doran, Mark
> <mark.doran@intel.com>
> Subject: Re: [edk2] [patch 2/2] MdeModulePkg/BmBoot: Report status when
> fail to load/start boot option
>
> +Mark, for my comments on the process
Thank you :)
> On 02/20/19 03:21, Ni, Ray wrote:
> > Laszlo,
> > Thanks for catching this.
> >
> > GenPerformMemoryTest() in
> > MdeModulePkg\Universal\MemoryTest\GenericMemoryTestDxe\LightMemoryTest
> > .c uses the same technics as you suggested.
> >
> > I give up to propose another option: having pack(1) for the new status
> structure.
>
> I think that byte-packing EFI_RETURN_STATUS_EXTENDED_DATA in the PI-1.7
> spec would have been viable, but we should have thought about that in
> advance. The PI and UEFI specs require natural alignment for structure
> members, unless stated otherwise. There *are* a number of examples of
> "stated otherwise" (the term that the specs use is frequently "byte-packed
> structure"). Again, we should have thought of that in advance, before PI-
> 1.7 was released, so now we can only fix the way the object is populated.
Agreed.
> More generally, I think this demonstrates a flaw in the standardization
> process. PIWG and USWG should only standardize existing practice; that is,
> features that have at least one functional implementation. (Not necessarily
> open source, but the interfaces should be battle-hardened
> *first*.) The restriction where we can't even propose patches for edk2
> before the standards are updated is very counter-productive. There's
> practically zero chance for getting an actual implementation peer-reviewed
> and peer-tested before proposing the API for the standard.
>
> Personally I have suggested in the past to implement some new features as
> edk2 extensions first, and once they work, to upstream them to the
> standards. This idea is usually rejected by maintainers, because
> maintainers worry about becoming stuck with more and more edk2 extensions
> to core code (i.e., they worry about the feature proponents not making good
> on their promise to standardize). Unfortunately, the alternative (= the
> current practice) is that we standardize speculative interfaces, which then
> prove suboptimal once we implement them.
So personally I'm a big believer in having working code for things we write down in the standards. When EFI was first drafted that's how we did it: what was then known as "the sample implementation" and the actual spec content were more or less developed in parallel and the spec wasn't done until the code was working satisfactorily. We got away from that largely because when the UEFI Forum was formed, there was nominal interest in promoting more than one implementation and keeping spec and code requirements separate was a part of that.
Recent changes, particularly inside Intel, present an opportunity for us to revisit this issue. I'm advocating for the approach of spec-follows-code for new Contributions that Intel will make to the open source project going forward. I'll have more to say about this at upcoming industry events.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch 2/2] MdeModulePkg/BmBoot: Report status when fail to load/start boot option
2019-02-20 17:19 ` Doran, Mark
@ 2019-02-21 8:55 ` Laszlo Ersek
0 siblings, 0 replies; 12+ messages in thread
From: Laszlo Ersek @ 2019-02-21 8:55 UTC (permalink / raw)
To: Doran, Mark, Ni, Ray, Bi, Dandan; +Cc: edk2-devel@lists.01.org, Wu, Hao A
On 02/20/19 18:19, Doran, Mark wrote:
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Wednesday, February 20, 2019 1:25 AM
>> To: Ni, Ray <ray.ni@intel.com>; Bi, Dandan <dandan.bi@intel.com>
>> Cc: edk2-devel@lists.01.org; Wu, Hao A <hao.a.wu@intel.com>; Doran, Mark
>> <mark.doran@intel.com>
>> Subject: Re: [edk2] [patch 2/2] MdeModulePkg/BmBoot: Report status when
>> fail to load/start boot option
>>
>> +Mark, for my comments on the process
>
> Thank you :)
>
>> On 02/20/19 03:21, Ni, Ray wrote:
>>> Laszlo,
>>> Thanks for catching this.
>>>
>>> GenPerformMemoryTest() in
>>> MdeModulePkg\Universal\MemoryTest\GenericMemoryTestDxe\LightMemoryTest
>>> .c uses the same technics as you suggested.
>>>
>>> I give up to propose another option: having pack(1) for the new status
>> structure.
>>
>> I think that byte-packing EFI_RETURN_STATUS_EXTENDED_DATA in the PI-1.7
>> spec would have been viable, but we should have thought about that in
>> advance. The PI and UEFI specs require natural alignment for structure
>> members, unless stated otherwise. There *are* a number of examples of
>> "stated otherwise" (the term that the specs use is frequently "byte-packed
>> structure"). Again, we should have thought of that in advance, before PI-
>> 1.7 was released, so now we can only fix the way the object is populated.
>
> Agreed.
>
>> More generally, I think this demonstrates a flaw in the standardization
>> process. PIWG and USWG should only standardize existing practice; that is,
>> features that have at least one functional implementation. (Not necessarily
>> open source, but the interfaces should be battle-hardened
>> *first*.) The restriction where we can't even propose patches for edk2
>> before the standards are updated is very counter-productive. There's
>> practically zero chance for getting an actual implementation peer-reviewed
>> and peer-tested before proposing the API for the standard.
>>
>> Personally I have suggested in the past to implement some new features as
>> edk2 extensions first, and once they work, to upstream them to the
>> standards. This idea is usually rejected by maintainers, because
>> maintainers worry about becoming stuck with more and more edk2 extensions
>> to core code (i.e., they worry about the feature proponents not making good
>> on their promise to standardize). Unfortunately, the alternative (= the
>> current practice) is that we standardize speculative interfaces, which then
>> prove suboptimal once we implement them.
>
> So personally I'm a big believer in having working code for things we write down in the standards. When EFI was first drafted that's how we did it: what was then known as "the sample implementation" and the actual spec content were more or less developed in parallel and the spec wasn't done until the code was working satisfactorily. We got away from that largely because when the UEFI Forum was formed, there was nominal interest in promoting more than one implementation and keeping spec and code requirements separate was a part of that.
>
> Recent changes, particularly inside Intel, present an opportunity for us to revisit this issue. I'm advocating for the approach of spec-follows-code for new Contributions that Intel will make to the open source project going forward. I'll have more to say about this at upcoming industry events.
>
Sounds fantastic! Thank you!
Laszlo
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch 2/2] MdeModulePkg/BmBoot: Report status when fail to load/start boot option
2019-02-20 1:19 ` Laszlo Ersek
2019-02-20 1:36 ` Laszlo Ersek
2019-02-20 2:21 ` Ni, Ray
@ 2019-02-20 2:35 ` Bi, Dandan
2 siblings, 0 replies; 12+ messages in thread
From: Bi, Dandan @ 2019-02-20 2:35 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Wu, Hao A, Ni, Ray, edk2-devel@lists.01.org
Hi Laszlo,
Thanks for catching this issue.
I am sorry that I didn't consider the alignment issue when working on this patch.
Thanks,
Dandan
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Wednesday, February 20, 2019 9:19 AM
> To: Bi, Dandan <dandan.bi@intel.com>
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; edk2-
> devel@lists.01.org
> Subject: Re: [edk2] [patch 2/2] MdeModulePkg/BmBoot: Report status when
> fail to load/start boot option
>
> Hi Dandan,
>
> On 02/15/19 09:51, Dandan Bi wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1398
> >
> > According to PI1.7 Spec, report extended data describing an EFI_STATUS
> > return value along with EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR
> and
> > EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED status code when fail to load
> or
> > start boot option image.
> >
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Hao Wu <hao.a.wu@intel.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Dandan Bi <dandan.bi@intel.com>
> > ---
> > .../Library/UefiBootManagerLib/BmBoot.c | 22 ++++++++++++++-----
> > 1 file changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > index 6444fb43eb..9be1633b74 100644
> > --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > @@ -1818,15 +1818,20 @@ EfiBootManagerBoot (
> > FreePool (FilePath);
> > }
> >
> > if (EFI_ERROR (Status)) {
> > //
> > - // Report Status Code to indicate that the failure to load boot option
> > + // Report Status Code with the failure status to indicate that
> > + the failure to load boot option
> > //
> > - REPORT_STATUS_CODE (
> > + REPORT_STATUS_CODE_EX (
> > EFI_ERROR_CODE | EFI_ERROR_MINOR,
> > - (EFI_SOFTWARE_DXE_BS_DRIVER |
> EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR)
> > + (EFI_SOFTWARE_DXE_BS_DRIVER |
> EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR),
> > + 0,
> > + NULL,
> > + NULL,
> > + &Status,
> > + sizeof (EFI_STATUS)
> > );
> > BootOption->Status = Status;
> > //
> > // Destroy the RAM disk
> > //
> > @@ -1902,15 +1907,20 @@ EfiBootManagerBoot (
> > Status = gBS->StartImage (ImageHandle, &BootOption->ExitDataSize,
> &BootOption->ExitData);
> > DEBUG ((DEBUG_INFO | DEBUG_LOAD, "Image Return Status = %r\n",
> Status));
> > BootOption->Status = Status;
> > if (EFI_ERROR (Status)) {
> > //
> > - // Report Status Code to indicate that boot failure
> > + // Report Status Code with the failure status to indicate that
> > + boot failure
> > //
> > - REPORT_STATUS_CODE (
> > + REPORT_STATUS_CODE_EX (
> > EFI_ERROR_CODE | EFI_ERROR_MINOR,
> > - (EFI_SOFTWARE_DXE_BS_DRIVER |
> EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED)
> > + (EFI_SOFTWARE_DXE_BS_DRIVER |
> EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED),
> > + 0,
> > + NULL,
> > + NULL,
> > + &Status,
> > + sizeof (EFI_STATUS)
> > );
> > }
> > PERF_END_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32)
> > OptionNumber);
> >
> > //
> >
>
> Unfortunately, this patch is not good; we made a mistake here.
>
> Consider the EFI_RETURN_STATUS_EXTENDED_DATA structure, added in
> patch
> #1:
>
> > typedef struct {
> > ///
> > /// The data header identifying the data:
> > /// DataHeader.HeaderSize should be sizeof(EFI_STATUS_CODE_DATA),
> > /// DataHeader.Size should be
> sizeof(EFI_RETURN_STATUS_EXTENDED_DATA) - HeaderSize,
> > /// DataHeader.Type should be EFI_STATUS_CODE_SPECIFIC_DATA_GUID.
> > ///
> > EFI_STATUS_CODE_DATA DataHeader;
> > ///
> > /// The EFI_STATUS return value of the service or function whose failure
> triggered the
> > /// reporting of the status code (generally an error code or a debug code).
> > ///
> > EFI_STATUS ReturnStatus;
> > } EFI_RETURN_STATUS_EXTENDED_DATA;
>
> According to the UEFI spec, unless specified otherwise, structure members
> are aligned naturally.
>
> And, the PI spec references the UEFI spec with regard to data types.
>
> Accordingly, when this structure is built for X64, the size of this structure is 32
> bytes, and the offset of ReturnStatus is 24. There is a 4-byte padding
> between DataHeader (which is 20 bytes in size) and the ReturnStatus field.
> DataHeader has type
>
> > typedef struct {
> > ///
> > /// The size of the structure. This is specified to enable future expansion.
> > ///
> > UINT16 HeaderSize;
> > ///
> > /// The size of the data in bytes. This does not include the size of the
> header structure.
> > ///
> > UINT16 Size;
> > ///
> > /// The GUID defining the type of the data.
> > ///
> > EFI_GUID Type;
> > } EFI_STATUS_CODE_DATA;
>
> which extends to 20 bytes.
>
> I'm working on patches that capture / process
> EFI_RETURN_STATUS_EXTENDED_DATA. The fields I'm seeing in DataHeader
> are (on X64):
> - HeaderSize = 0x14 (20 decimal)
> - Size = 0x8,
> - Type = {
> Data1 = 0x335984bd,
> Data2 = 0xe805,
> Data3 = 0x409a,
> Data4 = {0xb8, 0xf8, 0xd2, 0x7e, 0xce, 0x5f, 0xf7, 0xa6}
> }
>
> The "DataHeader.Size" field is incorrect. It should be 12 (that is, 32-20),
> according to the documentation:
>
> > /// DataHeader.Size should be
> > sizeof(EFI_RETURN_STATUS_EXTENDED_DATA) - HeaderSize,
>
> I think in the code above, we should use a temporary
> EFI_RETURN_STATUS_EXTENDED_DATA structure, zero it out, then set the
> ReturnStatus field in it. Finally, call the REPORT_STATUS_CODE_EX () macro
> with the trailing portion of this temporary object.
>
> I'll report the same in a TianoCore BZ, and will try to submit a patch as well.
>
> I'm sorry that I didn't catch this in review.
>
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 12+ messages in thread