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 suggestion. I am good with the new name and will send out 
the updated branch shortly.

Regards,
Kun

On 01/07/2022 12:45, Kinney, Michael D wrote:
> Perhaps the term fragmented would be better than inconsistent for this condition?
> 
> EFI_SW_EC_FRAGMENTED_MEMORY_MAP
> 
> The term inconsistent could be used to describe other issues with the memory map.
> If we think there may be other memory map conditions that we want to cover with
> the same status code, then I am fine with INCONSISTENT.
> 
> Mike
> 
>> -----Original Message-----
>> From: Kun Qin <kuqin12@gmail.com>
>> Sent: Friday, January 7, 2022 12:10 PM
>> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
>> Cc: Andrew Fish <afish@apple.com>; Leif Lindholm <leif@nuviainc.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Liu, Zhiguang
>> <zhiguang.liu@intel.com>
>> Subject: Re: [edk2-devel] [PATCH v1 1/2] EDK2 Code First: PI Specification: New error codes of Host Software class
>>
>> 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
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>> 
>>>
>>>