Re: [edk2-devel] [PATCH v1 1/2] EDK2 Code First: PI Specification: New error codes of Host Software class

["Kun Qin" <kuqin12@gmail.com>]

Hi Mike,

Thanks for the input.

Is it better to rename "EFI_SW_EC_MEMORY_TYPE_INFORMATION_CHANGE" as 
"EFI_SW_EC_INCONSISTENT_MEM_MAP"? This name should be more generic yet 
still describing what OSes may be expecting.

As per RELEASE_ASSERT, I agree that this definition is implementation 
specific and such unrecoverable errors are already covered by 
"EFI_SW_EC_ILLEGAL_SOFTWARE_STATE". So I will drop the addition of 
"RELEASE_ASSERT" in the next update.

Please let me know if you have other comments or concerns, otherwise I 
will send out v2 update shortly.

Thanks,
Kun

On 01/06/2022 18:09, Michael D Kinney wrote:
> Hi Kun,
> 
> Memory Type Information is the name of an EDK II feature.  Also, not all memory
> type information changes required a reset/reboot.  That is configurable by the
> platform.
> 
> The system attribute that requires a reboot is if the UEFI Memory Map during a normal
> boot is in a state that would be incompatible with a potential future ACPI S4
> resume boot.  Some OSes require the UEFI Memory ranges for RT and ACPI memory types
> to be in the same location in normal boot and ACPI S4 resume boot.
> 
> I am wondering if we can choose a different name for the new PI Status code that
> reflects this OS ACPI requirement for a consistent memory map instead of referring
> to the EDK II Memory Type Information feature.  That way, the PI Spec name would
> allow implementations that do not necessarily required the EDK II specific
> implementation feature.
> 
> RELEASE_ASSERT also seems to imply an implementation specific way the reset/reboot
> is triggered.  An ASSERT() is typically triggered for a condition for which the
> code that follow the ASSERT() can not continue without unexpected or undefined
> behavior.  So the system is in a bad state that is not recoverable.  This type of
> state could be detected with a normal if/then/else logic in C code when looking
> at system state or encapsulated in an ASSERT() that is enabled in release builds.
> Once again, I think we need a different name that does not require the detection
> logic to be in an EDK II ASSERT() macro.
> 
> Thanks,
> 
> Mike
> 
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Kun Qin
>> Sent: Thursday, January 6, 2022 5:03 PM
>> To: devel@edk2.groups.io
>> Cc: Andrew Fish <afish@apple.com>; Leif Lindholm <leif@nuviainc.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
>> <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>
>> Subject: [edk2-devel] [PATCH v1 1/2] EDK2 Code First: PI Specification: New error codes of Host Software class
>>
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3794
>>
>> This change includes specification update markdown file that describes
>> the proposed PI Specification v1.7 Errata A in detail and potential
>> impact to the existing codebase.
>>
>> Cc: Andrew Fish <afish@apple.com>
>> Cc: Leif Lindholm <leif@nuviainc.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>>
>> Signed-off-by: Kun Qin <kuqin12@gmail.com>
>> ---
>>   CodeFirst/BZ3794-SpecChange.md | 60 ++++++++++++++++++++
>>   1 file changed, 60 insertions(+)
>>
>> diff --git a/CodeFirst/BZ3794-SpecChange.md b/CodeFirst/BZ3794-SpecChange.md
>> new file mode 100644
>> index 000000000000..bbb526896795
>> --- /dev/null
>> +++ b/CodeFirst/BZ3794-SpecChange.md
>> @@ -0,0 +1,60 @@
>> +# Title: Introduction of `EFI_MM_COMMUNICATE_HEADER_V3` and `MM_COMMUNICATE3_*` interface
>> +
>> +## Status: Draft
>> +
>> +## Document: UEFI Platform Initialization Specification Version 1.7 Errata A
>> +
>> +## License
>> +
>> +SPDX-License-Identifier: CC-BY-4.0
>> +
>> +## Submitter: [TianoCore Community](https://www.tianocore.org)
>> +
>> +## Summary of the change
>> +
>> +Introduce `EFI_SW_EC_MEMORY_TYPE_INFORMATION_CHANGE` and `EFI_SW_EC_RELEASE_ASSERT` into Status Codes definition.
>> +
>> +## Benefits of the change
>> +
>> +Current Status Codes covered various [software class error code
>> definitions](https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Pi/PiStatusCode.h).
>> +
>> +However, there are a few critical instances where the software could trigger system reboots while the corresponding case was not
>> covered by the already defined status codes:
>> +
>> +1. Memory type information change triggered system reboot;
>> +2. Assert triggered reboot on systems that did not enable system halts;
>> +
>> +The unexpected system reboots above could indicate decay of system health and reporting of such generic events would provide
>> helpful information to OEMs to investigate/prevent system failures in general.
>> +
>> +The request of this change intends to expand definitions of `EFI_SW_EC_**` under Status Codes to cover more unexpected system
>> reboot events, which could improve Status Code futility and readability.
>> +
>> +## Impact of the change
>> +
>> +Occupy 2 new macro definitions of Error Codes under Software class Status Codes.
>> +
>> +## Detailed description of the change [normative updates]
>> +
>> +### Specification Changes
>> +
>> +1. In PI Specification v1.7 Errata A: Vol. 3, Table 3-61: Error Code Operations: Host Software Class, add 2 new rows below
>> `EFI_SW_EC_FV_CORRUPTED` definition:
>> +
>> +    | Operation | Description | Extended Data |
>> +    | --- | --- | --- |
>> +    | EFI_SW_EC_MEMORY_TYPE_INFORMATION_CHANGE | System will reboot due to memory type information changes | None |
>> +    | EFI_SW_EC_RELEASE_ASSERT | System software asserted  | None |
>> +
>> +1. In PI Specification v1.7 Errata A: Vol. 3, Table 3-61: Error Code Operations: Host Software Class, replace the row of
>> `0x0014–0x00FF` to:
>> +
>> +    | Operation | Description | Extended Data |
>> +    | --- | --- | --- |
>> +    | 0x0016–0x00FF | Reserved for future use by this specification for Host Software class error codes. | None |
>> +
>> +1. In PI Specification v1.7 Errata A: Vol. 3, Section 6.7.4.3 Error Code Definitions: Prototype, add 2 new definitions below
>> `EFI_SW_EC_FV_CORRUPTED` definition:
>> +
>> +    ```c
>> +    #define EFI_SW_EC_MEMORY_TYPE_INFORMATION_CHANGE  0x00000014
>> +    #define EFI_SW_EC_RELEASE_ASSERT                  0x00000015
>> +    ```
>> +
>> +### Code Changes
>> +
>> +1. Add macro definitions in `MdePkg/Include/Pi/PiStatusCode.h` to match new specification.
>> --
>> 2.34.1.windows.1
>>
>>
>>
>>
>>
> 
> 
> 
> 
> 
>