* [edk2-devel] [PATCH 0/2] DynamicTablesPkg/TableHelperLib updates @ 2023-10-06 16:28 Jeshua Smith via groups.io 2023-10-06 16:28 ` [edk2-devel] [PATCH 1/2] DynamicTablesPkg/TableHelperLib: Fix and improve text handling Jeshua Smith via groups.io ` (4 more replies) 0 siblings, 5 replies; 11+ messages in thread From: Jeshua Smith via groups.io @ 2023-10-06 16:28 UTC (permalink / raw) To: devel; +Cc: Sami.Mujawar, pierre.gondois, Jeshua Smith While using the ConfigurationManagerObjectParser to dump objects and debug adding new objects, I noticed some bugs and deficiencies. This series is intended to address those. Jeshua Smith (2): DynamicTablesPkg/TableHelperLib: Fix and improve text handling DynamicTablesPkg/TableHelperLib: Enhance error handling .../ConfigurationManagerObjectParser.c | 223 ++++++++++++++---- 1 file changed, 176 insertions(+), 47 deletions(-) -- 2.25.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109363): https://edk2.groups.io/g/devel/message/109363 Mute This Topic: https://groups.io/mt/101801382/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 11+ messages in thread
* [edk2-devel] [PATCH 1/2] DynamicTablesPkg/TableHelperLib: Fix and improve text handling 2023-10-06 16:28 [edk2-devel] [PATCH 0/2] DynamicTablesPkg/TableHelperLib updates Jeshua Smith via groups.io @ 2023-10-06 16:28 ` Jeshua Smith via groups.io 2023-10-10 10:13 ` PierreGondois 2023-10-06 16:28 ` [edk2-devel] [PATCH 2/2] DynamicTablesPkg/TableHelperLib: Enhance error handling Jeshua Smith via groups.io ` (3 subsequent siblings) 4 siblings, 1 reply; 11+ messages in thread From: Jeshua Smith via groups.io @ 2023-10-06 16:28 UTC (permalink / raw) To: devel; +Cc: Sami.Mujawar, pierre.gondois, Jeshua Smith This fixes two bugs and adds some enhancements to the handling of characters and strings in objects being printed by the CM ObjectParser. Bug fixes: 1. PrintOemID() currently attempts to print characters with "%C", but the correct syntax is (lowercase) "%c". This bug results in "CCCCCC" being printed instead of the actual ASCII characters. 2. PrintString() is being passed a pointer to data in objects, but in some cases this data is the actual string to print and other cases it is a pointer to the string to print. This adds a PrintStringPtr function and uses the correct functions depending on the situation. Enhancements: 1. Some objects contain ASCII characters, which are currently printed as their hex values. This adds functions to print out ASCII character fields as text rather than hex, and uses those functions in several cases where the object data is defined to be ASCII. 2. The PrintOemID() function is replaced with the new identical but more generecically-named PrintChar6() function. Signed-off-by: Jeshua Smith <jeshuas@nvidia.com> --- .../ConfigurationManagerObjectParser.c | 176 ++++++++++++++---- 1 file changed, 143 insertions(+), 33 deletions(-) diff --git a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c index 99d6032510..92df1efee8 100644 --- a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c +++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c @@ -14,7 +14,7 @@ STATIC VOID EFIAPI -PrintOemId ( +PrintString ( CONST CHAR8 *Format, UINT8 *Ptr ); @@ -22,7 +22,31 @@ PrintOemId ( STATIC VOID EFIAPI -PrintString ( +PrintStringPtr ( + CONST CHAR8 *Format, + UINT8 *Ptr + ); + +STATIC +VOID +EFIAPI +PrintChar4 ( + CONST CHAR8 *Format, + UINT8 *Ptr + ); + +STATIC +VOID +EFIAPI +PrintChar6 ( + CONST CHAR8 *Format, + UINT8 *Ptr + ); + +STATIC +VOID +EFIAPI +PrintChar8 ( CONST CHAR8 *Format, UINT8 *Ptr ); @@ -190,16 +214,16 @@ STATIC CONST CM_OBJ_PARSER CmArmItsGroupNodeParser[] = { /** A parser for EArmObjNamedComponent. */ STATIC CONST CM_OBJ_PARSER CmArmNamedComponentNodeParser[] = { - { "Token", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL }, - { "IdMappingCount", 4, "0x%x", NULL }, - { "IdMappingToken", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL }, - { "Flags", 4, "0x%x", NULL }, - { "CacheCoherent", 4, "0x%x", NULL }, - { "AllocationHints", 1, "0x%x", NULL }, - { "MemoryAccessFlags", 1, "0x%x", NULL }, - { "AddressSizeLimit", 1, "0x%x", NULL }, - { "ObjectName", sizeof (CHAR8 *), NULL, PrintString }, - { "Identifier", 4, "0x%x", NULL }, + { "Token", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL }, + { "IdMappingCount", 4, "0x%x", NULL }, + { "IdMappingToken", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL }, + { "Flags", 4, "0x%x", NULL }, + { "CacheCoherent", 4, "0x%x", NULL }, + { "AllocationHints", 1, "0x%x", NULL }, + { "MemoryAccessFlags", 1, "0x%x", NULL }, + { "AddressSizeLimit", 1, "0x%x", NULL }, + { "ObjectName", sizeof (CHAR8 *), NULL, PrintStringPtr }, + { "Identifier", 4, "0x%x", NULL }, }; /** A parser for EArmObjRootComplex. @@ -740,19 +764,19 @@ STATIC CONST CM_OBJ_PARSER_ARRAY ArmNamespaceObjectParser[] = { */ STATIC CONST CM_OBJ_PARSER StdObjCfgMgrInfoParser[] = { { "Revision", 4, "0x%x", NULL }, - { "OemId[6]", 6, "%C%C%C%C%C%C", PrintOemId } + { "OemId[6]", 6, "%c%c%c%c%c%c", PrintChar6 } }; /** A parser for EStdObjAcpiTableList. */ STATIC CONST CM_OBJ_PARSER StdObjAcpiTableInfoParser[] = { - { "AcpiTableSignature", 4, "0x%x", NULL }, - { "AcpiTableRevision", 1, "%d", NULL }, - { "TableGeneratorId", sizeof (ACPI_TABLE_GENERATOR_ID), "0x%x", NULL }, - { "AcpiTableData", sizeof (EFI_ACPI_DESCRIPTION_HEADER *), "0x%p", NULL }, - { "OemTableId", 8, "0x%LLX", NULL }, - { "OemRevision", 4, "0x%x", NULL }, - { "MinorRevision", 1, "0x%x", NULL }, + { "AcpiTableSignature", 4, "%c%c%c%c", PrintChar4 }, + { "AcpiTableRevision", 1, "%d", NULL }, + { "TableGeneratorId", sizeof (ACPI_TABLE_GENERATOR_ID), "0x%x", NULL }, + { "AcpiTableData", sizeof (EFI_ACPI_DESCRIPTION_HEADER *), "0x%p", NULL }, + { "OemTableId", 8, "%c%c%c%c%c%c%c%c", PrintChar8 }, + { "OemRevision", 4, "0x%x", NULL }, + { "MinorRevision", 1, "0x%x", NULL }, }; /** A parser for EStdObjSmbiosTableList. @@ -773,22 +797,99 @@ STATIC CONST CM_OBJ_PARSER_ARRAY StdNamespaceObjectParser[] = { ARRAY_SIZE (StdObjSmbiosTableInfoParser) }, }; -/** Print OEM Id. +/** Print string data. + + The string must be NULL terminated. + + @param [in] Format Format to print the Ptr. + @param [in] Ptr Pointer to the string. +**/ +STATIC +VOID +EFIAPI +PrintString ( + IN CONST CHAR8 *Format, + IN UINT8 *Ptr + ) +{ + if (Ptr == NULL) { + ASSERT (0); + return; + } + + DEBUG ((DEBUG_ERROR, "%a", Ptr)); +} + +/** Print string from pointer. + + The string must be NULL terminated. + + @param [in] Format Format to print the string. + @param [in] Ptr Pointer to the string pointer. +**/ +STATIC +VOID +EFIAPI +PrintStringPtr ( + IN CONST CHAR8 *Format, + IN UINT8 *Ptr + ) +{ + UINT8 *String; + + if (Ptr == NULL) { + ASSERT (0); + return; + } + + String = *(UINT8 **)Ptr; + + if (String == NULL) { + String = (UINT8 *)"(NULLPTR)"; + } + + PrintString (Format, String); +} + +/** Print 4 characters. @param [in] Format Format to print the Ptr. - @param [in] Ptr Pointer to the OEM Id. + @param [in] Ptr Pointer to the characters. **/ STATIC VOID EFIAPI -PrintOemId ( +PrintChar4 ( IN CONST CHAR8 *Format, IN UINT8 *Ptr ) { DEBUG (( - DEBUG_INFO, - (Format != NULL) ? Format : "%C%C%C%C%C%C", + DEBUG_ERROR, + (Format != NULL) ? Format : "%c%c%c%c", + Ptr[0], + Ptr[1], + Ptr[2], + Ptr[3] + )); +} + +/** Print 6 characters. + + @param [in] Format Format to print the Ptr. + @param [in] Ptr Pointer to the characters. +**/ +STATIC +VOID +EFIAPI +PrintChar6 ( + IN CONST CHAR8 *Format, + IN UINT8 *Ptr + ) +{ + DEBUG (( + DEBUG_ERROR, + (Format != NULL) ? Format : "%c%c%c%c%c%c", Ptr[0], Ptr[1], Ptr[2], @@ -798,22 +899,31 @@ PrintOemId ( )); } -/** Print string. - - The string must be NULL terminated. +/** Print 8 characters. @param [in] Format Format to print the Ptr. - @param [in] Ptr Pointer to the string. + @param [in] Ptr Pointer to the characters. **/ STATIC VOID EFIAPI -PrintString ( - CONST CHAR8 *Format, - UINT8 *Ptr +PrintChar8 ( + IN CONST CHAR8 *Format, + IN UINT8 *Ptr ) { - DEBUG ((DEBUG_ERROR, "%a", Ptr)); + DEBUG (( + DEBUG_ERROR, + (Format != NULL) ? Format : "%c%c%c%c%c%c%c%c", + Ptr[0], + Ptr[1], + Ptr[2], + Ptr[3], + Ptr[4], + Ptr[5], + Ptr[6], + Ptr[7] + )); } /** Print fields of the objects. -- 2.25.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109365): https://edk2.groups.io/g/devel/message/109365 Mute This Topic: https://groups.io/mt/101801390/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] DynamicTablesPkg/TableHelperLib: Fix and improve text handling 2023-10-06 16:28 ` [edk2-devel] [PATCH 1/2] DynamicTablesPkg/TableHelperLib: Fix and improve text handling Jeshua Smith via groups.io @ 2023-10-10 10:13 ` PierreGondois 2023-10-10 14:57 ` Jeshua Smith via groups.io 0 siblings, 1 reply; 11+ messages in thread From: PierreGondois @ 2023-10-10 10:13 UTC (permalink / raw) To: Jeshua Smith, devel; +Cc: Sami.Mujawar Hello Jeshua, Just a small remark, but this should be equivalent, so: Reviewed-by: Pierre Gondois <pierre.gondois@arm.com> On 10/6/23 18:28, Jeshua Smith wrote: > This fixes two bugs and adds some enhancements to the handling of > characters and strings in objects being printed by the CM ObjectParser. > > Bug fixes: > 1. PrintOemID() currently attempts to print characters with "%C", > but the correct syntax is (lowercase) "%c". This bug results in > "CCCCCC" being printed instead of the actual ASCII characters. > 2. PrintString() is being passed a pointer to data in objects, but in > some cases this data is the actual string to print and other cases > it is a pointer to the string to print. This adds a PrintStringPtr > function and uses the correct functions depending on the situation. > > Enhancements: > 1. Some objects contain ASCII characters, which are currently printed > as their hex values. This adds functions to print out ASCII > character fields as text rather than hex, and uses those functions in > several cases where the object data is defined to be ASCII. > 2. The PrintOemID() function is replaced with the new identical but more > generecically-named PrintChar6() function. > > Signed-off-by: Jeshua Smith <jeshuas@nvidia.com> > --- > .../ConfigurationManagerObjectParser.c | 176 ++++++++++++++---- > 1 file changed, 143 insertions(+), 33 deletions(-) > > diff --git a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c > index 99d6032510..92df1efee8 100644 > --- a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c > +++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c > @@ -14,7 +14,7 @@ > STATIC > VOID > EFIAPI > -PrintOemId ( > +PrintString ( > CONST CHAR8 *Format, > UINT8 *Ptr > ); > @@ -22,7 +22,31 @@ PrintOemId ( > STATIC > VOID > EFIAPI > -PrintString ( > +PrintStringPtr ( > + CONST CHAR8 *Format, > + UINT8 *Ptr > + ); > + > +STATIC > +VOID > +EFIAPI > +PrintChar4 ( > + CONST CHAR8 *Format, > + UINT8 *Ptr > + ); > + > +STATIC > +VOID > +EFIAPI > +PrintChar6 ( > + CONST CHAR8 *Format, > + UINT8 *Ptr > + ); > + > +STATIC > +VOID > +EFIAPI > +PrintChar8 ( > CONST CHAR8 *Format, > UINT8 *Ptr > ); > @@ -190,16 +214,16 @@ STATIC CONST CM_OBJ_PARSER CmArmItsGroupNodeParser[] = { > /** A parser for EArmObjNamedComponent. > */ > STATIC CONST CM_OBJ_PARSER CmArmNamedComponentNodeParser[] = { > - { "Token", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL }, > - { "IdMappingCount", 4, "0x%x", NULL }, > - { "IdMappingToken", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL }, > - { "Flags", 4, "0x%x", NULL }, > - { "CacheCoherent", 4, "0x%x", NULL }, > - { "AllocationHints", 1, "0x%x", NULL }, > - { "MemoryAccessFlags", 1, "0x%x", NULL }, > - { "AddressSizeLimit", 1, "0x%x", NULL }, > - { "ObjectName", sizeof (CHAR8 *), NULL, PrintString }, > - { "Identifier", 4, "0x%x", NULL }, > + { "Token", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL }, > + { "IdMappingCount", 4, "0x%x", NULL }, > + { "IdMappingToken", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL }, > + { "Flags", 4, "0x%x", NULL }, > + { "CacheCoherent", 4, "0x%x", NULL }, > + { "AllocationHints", 1, "0x%x", NULL }, > + { "MemoryAccessFlags", 1, "0x%x", NULL }, > + { "AddressSizeLimit", 1, "0x%x", NULL }, > + { "ObjectName", sizeof (CHAR8 *), NULL, PrintStringPtr }, > + { "Identifier", 4, "0x%x", NULL }, > }; > > /** A parser for EArmObjRootComplex. > @@ -740,19 +764,19 @@ STATIC CONST CM_OBJ_PARSER_ARRAY ArmNamespaceObjectParser[] = { > */ > STATIC CONST CM_OBJ_PARSER StdObjCfgMgrInfoParser[] = { > { "Revision", 4, "0x%x", NULL }, > - { "OemId[6]", 6, "%C%C%C%C%C%C", PrintOemId } > + { "OemId[6]", 6, "%c%c%c%c%c%c", PrintChar6 } NIT: Is it necessary, since "%c%c%c%c%c%c" is the default format of PrintChar6() ? > }; > > /** A parser for EStdObjAcpiTableList. > */ > STATIC CONST CM_OBJ_PARSER StdObjAcpiTableInfoParser[] = { > - { "AcpiTableSignature", 4, "0x%x", NULL }, > - { "AcpiTableRevision", 1, "%d", NULL }, > - { "TableGeneratorId", sizeof (ACPI_TABLE_GENERATOR_ID), "0x%x", NULL }, > - { "AcpiTableData", sizeof (EFI_ACPI_DESCRIPTION_HEADER *), "0x%p", NULL }, > - { "OemTableId", 8, "0x%LLX", NULL }, > - { "OemRevision", 4, "0x%x", NULL }, > - { "MinorRevision", 1, "0x%x", NULL }, > + { "AcpiTableSignature", 4, "%c%c%c%c", PrintChar4 }, > + { "AcpiTableRevision", 1, "%d", NULL }, > + { "TableGeneratorId", sizeof (ACPI_TABLE_GENERATOR_ID), "0x%x", NULL }, > + { "AcpiTableData", sizeof (EFI_ACPI_DESCRIPTION_HEADER *), "0x%p", NULL }, > + { "OemTableId", 8, "%c%c%c%c%c%c%c%c", PrintChar8 }, > + { "OemRevision", 4, "0x%x", NULL }, > + { "MinorRevision", 1, "0x%x", NULL }, > }; > > /** A parser for EStdObjSmbiosTableList. > @@ -773,22 +797,99 @@ STATIC CONST CM_OBJ_PARSER_ARRAY StdNamespaceObjectParser[] = { > ARRAY_SIZE (StdObjSmbiosTableInfoParser) }, > }; > > -/** Print OEM Id. > +/** Print string data. > + > + The string must be NULL terminated. > + > + @param [in] Format Format to print the Ptr. > + @param [in] Ptr Pointer to the string. > +**/ > +STATIC > +VOID > +EFIAPI > +PrintString ( > + IN CONST CHAR8 *Format, > + IN UINT8 *Ptr > + ) > +{ > + if (Ptr == NULL) { > + ASSERT (0); > + return; > + } > + > + DEBUG ((DEBUG_ERROR, "%a", Ptr)); > +} > + > +/** Print string from pointer. > + > + The string must be NULL terminated. > + > + @param [in] Format Format to print the string. > + @param [in] Ptr Pointer to the string pointer. > +**/ > +STATIC > +VOID > +EFIAPI > +PrintStringPtr ( > + IN CONST CHAR8 *Format, > + IN UINT8 *Ptr > + ) > +{ > + UINT8 *String; > + > + if (Ptr == NULL) { > + ASSERT (0); > + return; > + } > + > + String = *(UINT8 **)Ptr; > + > + if (String == NULL) { > + String = (UINT8 *)"(NULLPTR)"; > + } > + > + PrintString (Format, String); > +} > + > +/** Print 4 characters. > > @param [in] Format Format to print the Ptr. > - @param [in] Ptr Pointer to the OEM Id. > + @param [in] Ptr Pointer to the characters. > **/ > STATIC > VOID > EFIAPI > -PrintOemId ( > +PrintChar4 ( > IN CONST CHAR8 *Format, > IN UINT8 *Ptr > ) > { > DEBUG (( > - DEBUG_INFO, > - (Format != NULL) ? Format : "%C%C%C%C%C%C", > + DEBUG_ERROR, > + (Format != NULL) ? Format : "%c%c%c%c", > + Ptr[0], > + Ptr[1], > + Ptr[2], > + Ptr[3] > + )); > +} > + > +/** Print 6 characters. > + > + @param [in] Format Format to print the Ptr. > + @param [in] Ptr Pointer to the characters. > +**/ > +STATIC > +VOID > +EFIAPI > +PrintChar6 ( > + IN CONST CHAR8 *Format, > + IN UINT8 *Ptr > + ) > +{ > + DEBUG (( > + DEBUG_ERROR, > + (Format != NULL) ? Format : "%c%c%c%c%c%c", > Ptr[0], > Ptr[1], > Ptr[2], > @@ -798,22 +899,31 @@ PrintOemId ( > )); > } > > -/** Print string. > - > - The string must be NULL terminated. > +/** Print 8 characters. > > @param [in] Format Format to print the Ptr. > - @param [in] Ptr Pointer to the string. > + @param [in] Ptr Pointer to the characters. > **/ > STATIC > VOID > EFIAPI > -PrintString ( > - CONST CHAR8 *Format, > - UINT8 *Ptr > +PrintChar8 ( > + IN CONST CHAR8 *Format, > + IN UINT8 *Ptr > ) > { > - DEBUG ((DEBUG_ERROR, "%a", Ptr)); > + DEBUG (( > + DEBUG_ERROR, > + (Format != NULL) ? Format : "%c%c%c%c%c%c%c%c", > + Ptr[0], > + Ptr[1], > + Ptr[2], > + Ptr[3], > + Ptr[4], > + Ptr[5], > + Ptr[6], > + Ptr[7] > + )); > } > > /** Print fields of the objects. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109487): https://edk2.groups.io/g/devel/message/109487 Mute This Topic: https://groups.io/mt/101801390/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] DynamicTablesPkg/TableHelperLib: Fix and improve text handling 2023-10-10 10:13 ` PierreGondois @ 2023-10-10 14:57 ` Jeshua Smith via groups.io 0 siblings, 0 replies; 11+ messages in thread From: Jeshua Smith via groups.io @ 2023-10-10 14:57 UTC (permalink / raw) To: Pierre Gondois, devel@edk2.groups.io; +Cc: Sami.Mujawar@arm.com Thanks for the review. Reply inline. -----Original Message----- From: Pierre Gondois <pierre.gondois@arm.com> Sent: Tuesday, October 10, 2023 4:14 AM To: Jeshua Smith <jeshuas@nvidia.com>; devel@edk2.groups.io Cc: Sami.Mujawar@arm.com Subject: Re: [PATCH 1/2] DynamicTablesPkg/TableHelperLib: Fix and improve text handling External email: Use caution opening links or attachments Hello Jeshua, Just a small remark, but this should be equivalent, so: Reviewed-by: Pierre Gondois <pierre.gondois@arm.com> On 10/6/23 18:28, Jeshua Smith wrote: > This fixes two bugs and adds some enhancements to the handling of > characters and strings in objects being printed by the CM ObjectParser. > > Bug fixes: > 1. PrintOemID() currently attempts to print characters with "%C", > but the correct syntax is (lowercase) "%c". This bug results in > "CCCCCC" being printed instead of the actual ASCII characters. > 2. PrintString() is being passed a pointer to data in objects, but in > some cases this data is the actual string to print and other cases > it is a pointer to the string to print. This adds a PrintStringPtr > function and uses the correct functions depending on the situation. > > Enhancements: > 1. Some objects contain ASCII characters, which are currently printed > as their hex values. This adds functions to print out ASCII > character fields as text rather than hex, and uses those functions in > several cases where the object data is defined to be ASCII. > 2. The PrintOemID() function is replaced with the new identical but more > generecically-named PrintChar6() function. > > Signed-off-by: Jeshua Smith <jeshuas@nvidia.com> > --- > .../ConfigurationManagerObjectParser.c | 176 ++++++++++++++---- > 1 file changed, 143 insertions(+), 33 deletions(-) > > diff --git > a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO > bjectParser.c > b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO > bjectParser.c > index 99d6032510..92df1efee8 100644 > --- > a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO > bjectParser.c > +++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationMana > +++ gerObjectParser.c > @@ -14,7 +14,7 @@ > STATIC > VOID > EFIAPI > -PrintOemId ( > +PrintString ( > CONST CHAR8 *Format, > UINT8 *Ptr > ); > @@ -22,7 +22,31 @@ PrintOemId ( > STATIC > VOID > EFIAPI > -PrintString ( > +PrintStringPtr ( > + CONST CHAR8 *Format, > + UINT8 *Ptr > + ); > + > +STATIC > +VOID > +EFIAPI > +PrintChar4 ( > + CONST CHAR8 *Format, > + UINT8 *Ptr > + ); > + > +STATIC > +VOID > +EFIAPI > +PrintChar6 ( > + CONST CHAR8 *Format, > + UINT8 *Ptr > + ); > + > +STATIC > +VOID > +EFIAPI > +PrintChar8 ( > CONST CHAR8 *Format, > UINT8 *Ptr > ); > @@ -190,16 +214,16 @@ STATIC CONST CM_OBJ_PARSER CmArmItsGroupNodeParser[] = { > /** A parser for EArmObjNamedComponent. > */ > STATIC CONST CM_OBJ_PARSER CmArmNamedComponentNodeParser[] = { > - { "Token", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL }, > - { "IdMappingCount", 4, "0x%x", NULL }, > - { "IdMappingToken", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL }, > - { "Flags", 4, "0x%x", NULL }, > - { "CacheCoherent", 4, "0x%x", NULL }, > - { "AllocationHints", 1, "0x%x", NULL }, > - { "MemoryAccessFlags", 1, "0x%x", NULL }, > - { "AddressSizeLimit", 1, "0x%x", NULL }, > - { "ObjectName", sizeof (CHAR8 *), NULL, PrintString }, > - { "Identifier", 4, "0x%x", NULL }, > + { "Token", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL }, > + { "IdMappingCount", 4, "0x%x", NULL }, > + { "IdMappingToken", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL }, > + { "Flags", 4, "0x%x", NULL }, > + { "CacheCoherent", 4, "0x%x", NULL }, > + { "AllocationHints", 1, "0x%x", NULL }, > + { "MemoryAccessFlags", 1, "0x%x", NULL }, > + { "AddressSizeLimit", 1, "0x%x", NULL }, > + { "ObjectName", sizeof (CHAR8 *), NULL, PrintStringPtr }, > + { "Identifier", 4, "0x%x", NULL }, > }; > > /** A parser for EArmObjRootComplex. > @@ -740,19 +764,19 @@ STATIC CONST CM_OBJ_PARSER_ARRAY ArmNamespaceObjectParser[] = { > */ > STATIC CONST CM_OBJ_PARSER StdObjCfgMgrInfoParser[] = { > { "Revision", 4, "0x%x", NULL }, > - { "OemId[6]", 6, "%C%C%C%C%C%C", PrintOemId } > + { "OemId[6]", 6, "%c%c%c%c%c%c", PrintChar6 } NIT: Is it necessary, since "%c%c%c%c%c%c" is the default format of PrintChar6() ? [JDS]: I considered changing it as unnecessary, but in the end left it as is since the original code used the same pattern (i.e. passing in a format that was the same as the default format). > }; > > /** A parser for EStdObjAcpiTableList. > */ > STATIC CONST CM_OBJ_PARSER StdObjAcpiTableInfoParser[] = { > - { "AcpiTableSignature", 4, "0x%x", NULL }, > - { "AcpiTableRevision", 1, "%d", NULL }, > - { "TableGeneratorId", sizeof (ACPI_TABLE_GENERATOR_ID), "0x%x", NULL }, > - { "AcpiTableData", sizeof (EFI_ACPI_DESCRIPTION_HEADER *), "0x%p", NULL }, > - { "OemTableId", 8, "0x%LLX", NULL }, > - { "OemRevision", 4, "0x%x", NULL }, > - { "MinorRevision", 1, "0x%x", NULL }, > + { "AcpiTableSignature", 4, "%c%c%c%c", PrintChar4 }, > + { "AcpiTableRevision", 1, "%d", NULL }, > + { "TableGeneratorId", sizeof (ACPI_TABLE_GENERATOR_ID), "0x%x", NULL }, > + { "AcpiTableData", sizeof (EFI_ACPI_DESCRIPTION_HEADER *), "0x%p", NULL }, > + { "OemTableId", 8, "%c%c%c%c%c%c%c%c", PrintChar8 }, > + { "OemRevision", 4, "0x%x", NULL }, > + { "MinorRevision", 1, "0x%x", NULL }, > }; > > /** A parser for EStdObjSmbiosTableList. > @@ -773,22 +797,99 @@ STATIC CONST CM_OBJ_PARSER_ARRAY StdNamespaceObjectParser[] = { > ARRAY_SIZE (StdObjSmbiosTableInfoParser) }, > }; > > -/** Print OEM Id. > +/** Print string data. > + > + The string must be NULL terminated. > + > + @param [in] Format Format to print the Ptr. > + @param [in] Ptr Pointer to the string. > +**/ > +STATIC > +VOID > +EFIAPI > +PrintString ( > + IN CONST CHAR8 *Format, > + IN UINT8 *Ptr > + ) > +{ > + if (Ptr == NULL) { > + ASSERT (0); > + return; > + } > + > + DEBUG ((DEBUG_ERROR, "%a", Ptr)); > +} > + > +/** Print string from pointer. > + > + The string must be NULL terminated. > + > + @param [in] Format Format to print the string. > + @param [in] Ptr Pointer to the string pointer. > +**/ > +STATIC > +VOID > +EFIAPI > +PrintStringPtr ( > + IN CONST CHAR8 *Format, > + IN UINT8 *Ptr > + ) > +{ > + UINT8 *String; > + > + if (Ptr == NULL) { > + ASSERT (0); > + return; > + } > + > + String = *(UINT8 **)Ptr; > + > + if (String == NULL) { > + String = (UINT8 *)"(NULLPTR)"; > + } > + > + PrintString (Format, String); > +} > + > +/** Print 4 characters. > > @param [in] Format Format to print the Ptr. > - @param [in] Ptr Pointer to the OEM Id. > + @param [in] Ptr Pointer to the characters. > **/ > STATIC > VOID > EFIAPI > -PrintOemId ( > +PrintChar4 ( > IN CONST CHAR8 *Format, > IN UINT8 *Ptr > ) > { > DEBUG (( > - DEBUG_INFO, > - (Format != NULL) ? Format : "%C%C%C%C%C%C", > + DEBUG_ERROR, > + (Format != NULL) ? Format : "%c%c%c%c", > + Ptr[0], > + Ptr[1], > + Ptr[2], > + Ptr[3] > + )); > +} > + > +/** Print 6 characters. > + > + @param [in] Format Format to print the Ptr. > + @param [in] Ptr Pointer to the characters. > +**/ > +STATIC > +VOID > +EFIAPI > +PrintChar6 ( > + IN CONST CHAR8 *Format, > + IN UINT8 *Ptr > + ) > +{ > + DEBUG (( > + DEBUG_ERROR, > + (Format != NULL) ? Format : "%c%c%c%c%c%c", > Ptr[0], > Ptr[1], > Ptr[2], > @@ -798,22 +899,31 @@ PrintOemId ( > )); > } > > -/** Print string. > - > - The string must be NULL terminated. > +/** Print 8 characters. > > @param [in] Format Format to print the Ptr. > - @param [in] Ptr Pointer to the string. > + @param [in] Ptr Pointer to the characters. > **/ > STATIC > VOID > EFIAPI > -PrintString ( > - CONST CHAR8 *Format, > - UINT8 *Ptr > +PrintChar8 ( > + IN CONST CHAR8 *Format, > + IN UINT8 *Ptr > ) > { > - DEBUG ((DEBUG_ERROR, "%a", Ptr)); > + DEBUG (( > + DEBUG_ERROR, > + (Format != NULL) ? Format : "%c%c%c%c%c%c%c%c", > + Ptr[0], > + Ptr[1], > + Ptr[2], > + Ptr[3], > + Ptr[4], > + Ptr[5], > + Ptr[6], > + Ptr[7] > + )); > } > > /** Print fields of the objects. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109495): https://edk2.groups.io/g/devel/message/109495 Mute This Topic: https://groups.io/mt/101801390/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 11+ messages in thread
* [edk2-devel] [PATCH 2/2] DynamicTablesPkg/TableHelperLib: Enhance error handling 2023-10-06 16:28 [edk2-devel] [PATCH 0/2] DynamicTablesPkg/TableHelperLib updates Jeshua Smith via groups.io 2023-10-06 16:28 ` [edk2-devel] [PATCH 1/2] DynamicTablesPkg/TableHelperLib: Fix and improve text handling Jeshua Smith via groups.io @ 2023-10-06 16:28 ` Jeshua Smith via groups.io 2023-10-10 10:13 ` PierreGondois 2023-10-23 15:30 ` [edk2-devel] [PATCH 0/2] DynamicTablesPkg/TableHelperLib updates Jeshua Smith via groups.io ` (2 subsequent siblings) 4 siblings, 1 reply; 11+ messages in thread From: Jeshua Smith via groups.io @ 2023-10-06 16:28 UTC (permalink / raw) To: devel; +Cc: Sami.Mujawar, pierre.gondois, Jeshua Smith This patch enhances error handling and reporting in the CM ObjectParser. Specifically: 1. ObjectIDs used as array indexes are checked for being out of bounds, and if so an error message is printed before the assert. 2. An error message is printed for unsupported NameSpaceIDs. 3. Adds support for unimplemented parsers by allowing IDs to list a NULL parser, resulting in an unimplemented message being printed. Signed-off-by: Jeshua Smith <jeshuas@nvidia.com> --- .../ConfigurationManagerObjectParser.c | 47 +++++++++++++------ 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c index 92df1efee8..22b8fdb906 100644 --- a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c +++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c @@ -795,6 +795,7 @@ STATIC CONST CM_OBJ_PARSER_ARRAY StdNamespaceObjectParser[] = { ARRAY_SIZE (StdObjAcpiTableInfoParser) }, { "EStdObjSmbiosTableList", StdObjSmbiosTableInfoParser, ARRAY_SIZE (StdObjSmbiosTableInfoParser) }, + { "EStdObjMax", NULL, 0} }; /** Print string data. @@ -1066,6 +1067,12 @@ ParseCmObjDesc ( return; } + if (ObjId >= ARRAY_SIZE (StdNamespaceObjectParser)) { + DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the StdNamespaceObjectParser array\n", ObjId)); + ASSERT (0); + return; + } + ParserArray = &StdNamespaceObjectParser[ObjId]; break; case EObjNameSpaceArm: @@ -1074,10 +1081,17 @@ ParseCmObjDesc ( return; } + if (ObjId >= ARRAY_SIZE (ArmNamespaceObjectParser)) { + DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the ArmNamespaceObjectParser array\n", ObjId)); + ASSERT (0); + return; + } + ParserArray = &ArmNamespaceObjectParser[ObjId]; break; default: // Not supported + DEBUG ((DEBUG_ERROR, "NameSpaceId 0x%x, ObjId 0x%x is not supported by the parser\n", NameSpaceId, ObjId)); ASSERT (0); return; } // switch @@ -1095,21 +1109,26 @@ ParseCmObjDesc ( ObjIndex + 1, ObjectCount )); - PrintCmObjDesc ( - (VOID *)((UINTN)CmObjDesc->Data + Offset), - ParserArray->Parser, - ParserArray->ItemCount, - &RemainingSize, - 1 - ); - if ((RemainingSize > CmObjDesc->Size) || - (RemainingSize < 0)) - { - ASSERT (0); - return; - } + if (ParserArray->Parser == NULL) { + DEBUG ((DEBUG_ERROR, "Parser not implemented\n")); + RemainingSize = 0; + } else { + PrintCmObjDesc ( + (VOID *)((UINTN)CmObjDesc->Data + Offset), + ParserArray->Parser, + ParserArray->ItemCount, + &RemainingSize, + 1 + ); + if ((RemainingSize > CmObjDesc->Size) || + (RemainingSize < 0)) + { + ASSERT (0); + return; + } - Offset = CmObjDesc->Size - RemainingSize; + Offset = CmObjDesc->Size - RemainingSize; + } } // for ASSERT (RemainingSize == 0); -- 2.25.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109364): https://edk2.groups.io/g/devel/message/109364 Mute This Topic: https://groups.io/mt/101801385/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH 2/2] DynamicTablesPkg/TableHelperLib: Enhance error handling 2023-10-06 16:28 ` [edk2-devel] [PATCH 2/2] DynamicTablesPkg/TableHelperLib: Enhance error handling Jeshua Smith via groups.io @ 2023-10-10 10:13 ` PierreGondois 2023-10-10 15:41 ` Jeshua Smith via groups.io 0 siblings, 1 reply; 11+ messages in thread From: PierreGondois @ 2023-10-10 10:13 UTC (permalink / raw) To: Jeshua Smith, devel; +Cc: Sami.Mujawar Hello Jeshua, On 10/6/23 18:28, Jeshua Smith wrote: > This patch enhances error handling and reporting in the CM ObjectParser. > Specifically: > 1. ObjectIDs used as array indexes are checked for being out of bounds, > and if so an error message is printed before the assert. > 2. An error message is printed for unsupported NameSpaceIDs. > 3. Adds support for unimplemented parsers by allowing IDs to list a > NULL parser, resulting in an unimplemented message being printed. I am not sure I see in which context 3. would be used/necessary. Is it possible to detail ? (Code-wise everything looks good to me) Regards, Pierre > > Signed-off-by: Jeshua Smith <jeshuas@nvidia.com> > --- > .../ConfigurationManagerObjectParser.c | 47 +++++++++++++------ > 1 file changed, 33 insertions(+), 14 deletions(-) > > diff --git a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c > index 92df1efee8..22b8fdb906 100644 > --- a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c > +++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c > @@ -795,6 +795,7 @@ STATIC CONST CM_OBJ_PARSER_ARRAY StdNamespaceObjectParser[] = { > ARRAY_SIZE (StdObjAcpiTableInfoParser) }, > { "EStdObjSmbiosTableList", StdObjSmbiosTableInfoParser, > ARRAY_SIZE (StdObjSmbiosTableInfoParser) }, > + { "EStdObjMax", NULL, 0} > }; > > /** Print string data. > @@ -1066,6 +1067,12 @@ ParseCmObjDesc ( > return; > } > > + if (ObjId >= ARRAY_SIZE (StdNamespaceObjectParser)) { > + DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the StdNamespaceObjectParser array\n", ObjId)); > + ASSERT (0); > + return; > + } > + > ParserArray = &StdNamespaceObjectParser[ObjId]; > break; > case EObjNameSpaceArm: > @@ -1074,10 +1081,17 @@ ParseCmObjDesc ( > return; > } > > + if (ObjId >= ARRAY_SIZE (ArmNamespaceObjectParser)) { > + DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the ArmNamespaceObjectParser array\n", ObjId)); > + ASSERT (0); > + return; > + } > + > ParserArray = &ArmNamespaceObjectParser[ObjId]; > break; > default: > // Not supported > + DEBUG ((DEBUG_ERROR, "NameSpaceId 0x%x, ObjId 0x%x is not supported by the parser\n", NameSpaceId, ObjId)); > ASSERT (0); > return; > } // switch > @@ -1095,21 +1109,26 @@ ParseCmObjDesc ( > ObjIndex + 1, > ObjectCount > )); > - PrintCmObjDesc ( > - (VOID *)((UINTN)CmObjDesc->Data + Offset), > - ParserArray->Parser, > - ParserArray->ItemCount, > - &RemainingSize, > - 1 > - ); > - if ((RemainingSize > CmObjDesc->Size) || > - (RemainingSize < 0)) > - { > - ASSERT (0); > - return; > - } > + if (ParserArray->Parser == NULL) { > + DEBUG ((DEBUG_ERROR, "Parser not implemented\n")); > + RemainingSize = 0; > + } else { > + PrintCmObjDesc ( > + (VOID *)((UINTN)CmObjDesc->Data + Offset), > + ParserArray->Parser, > + ParserArray->ItemCount, > + &RemainingSize, > + 1 > + ); > + if ((RemainingSize > CmObjDesc->Size) || > + (RemainingSize < 0)) > + { > + ASSERT (0); > + return; > + } > > - Offset = CmObjDesc->Size - RemainingSize; > + Offset = CmObjDesc->Size - RemainingSize; > + } > } // for > > ASSERT (RemainingSize == 0); -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109486): https://edk2.groups.io/g/devel/message/109486 Mute This Topic: https://groups.io/mt/101801385/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH 2/2] DynamicTablesPkg/TableHelperLib: Enhance error handling 2023-10-10 10:13 ` PierreGondois @ 2023-10-10 15:41 ` Jeshua Smith via groups.io 2023-10-11 12:51 ` PierreGondois 0 siblings, 1 reply; 11+ messages in thread From: Jeshua Smith via groups.io @ 2023-10-10 15:41 UTC (permalink / raw) To: Pierre Gondois, devel@edk2.groups.io; +Cc: Sami.Mujawar@arm.com #3 is not currently used by any published code. It is a development aid, which the ObjectParser itself seems to be. Here's why I added it. Several people on our team have (not yet upstreamed) changes that resulted in additional ObjectIDs being added to the ObjectID enums, but without corresponding parsers being added to the ObjectParser. Dumping of the objects with the ObjectParser wasn't enabled by default, so this wasn't detected by them. Without #1 the ObjectParser code does out of bounds array accesses, sometimes leading to crashes. #1 will now detect and report that problem. When I enabled dumping of objects to debug my new code, I hit this bug leading me to write #1. For me to work around the issue of missing parsers in order to be able to continue debug with my work, the "easy" solution was for me to temporarily add the new ObjectIDs to the parser list with NULL parsers and then inform the responsible parties that they need to go and add parsers for their new ObjectIDs. Doing this required #3 (support for NULL parsers) to be added. Ideally any code that is upstreamed back to EDKII will have non-NULL parsers at the point it is sent upstream, but allowing in-development changes to temporarily use NULL parsers is helpful. Hopefully that clarifies things. -----Original Message----- From: Pierre Gondois <pierre.gondois@arm.com> Sent: Tuesday, October 10, 2023 4:14 AM To: Jeshua Smith <jeshuas@nvidia.com>; devel@edk2.groups.io Cc: Sami.Mujawar@arm.com Subject: Re: [PATCH 2/2] DynamicTablesPkg/TableHelperLib: Enhance error handling External email: Use caution opening links or attachments Hello Jeshua, On 10/6/23 18:28, Jeshua Smith wrote: > This patch enhances error handling and reporting in the CM ObjectParser. > Specifically: > 1. ObjectIDs used as array indexes are checked for being out of bounds, > and if so an error message is printed before the assert. > 2. An error message is printed for unsupported NameSpaceIDs. > 3. Adds support for unimplemented parsers by allowing IDs to list a > NULL parser, resulting in an unimplemented message being printed. I am not sure I see in which context 3. would be used/necessary. Is it possible to detail ? (Code-wise everything looks good to me) Regards, Pierre > > Signed-off-by: Jeshua Smith <jeshuas@nvidia.com> > --- > .../ConfigurationManagerObjectParser.c | 47 +++++++++++++------ > 1 file changed, 33 insertions(+), 14 deletions(-) > > diff --git > a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO > bjectParser.c > b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO > bjectParser.c > index 92df1efee8..22b8fdb906 100644 > --- > a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO > bjectParser.c > +++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationMana > +++ gerObjectParser.c > @@ -795,6 +795,7 @@ STATIC CONST CM_OBJ_PARSER_ARRAY StdNamespaceObjectParser[] = { > ARRAY_SIZE (StdObjAcpiTableInfoParser) }, > { "EStdObjSmbiosTableList", StdObjSmbiosTableInfoParser, > ARRAY_SIZE (StdObjSmbiosTableInfoParser) }, > + { "EStdObjMax", NULL, 0} > }; > > /** Print string data. > @@ -1066,6 +1067,12 @@ ParseCmObjDesc ( > return; > } > > + if (ObjId >= ARRAY_SIZE (StdNamespaceObjectParser)) { > + DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the StdNamespaceObjectParser array\n", ObjId)); > + ASSERT (0); > + return; > + } > + > ParserArray = &StdNamespaceObjectParser[ObjId]; > break; > case EObjNameSpaceArm: > @@ -1074,10 +1081,17 @@ ParseCmObjDesc ( > return; > } > > + if (ObjId >= ARRAY_SIZE (ArmNamespaceObjectParser)) { > + DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the ArmNamespaceObjectParser array\n", ObjId)); > + ASSERT (0); > + return; > + } > + > ParserArray = &ArmNamespaceObjectParser[ObjId]; > break; > default: > // Not supported > + DEBUG ((DEBUG_ERROR, "NameSpaceId 0x%x, ObjId 0x%x is not > + supported by the parser\n", NameSpaceId, ObjId)); > ASSERT (0); > return; > } // switch > @@ -1095,21 +1109,26 @@ ParseCmObjDesc ( > ObjIndex + 1, > ObjectCount > )); > - PrintCmObjDesc ( > - (VOID *)((UINTN)CmObjDesc->Data + Offset), > - ParserArray->Parser, > - ParserArray->ItemCount, > - &RemainingSize, > - 1 > - ); > - if ((RemainingSize > CmObjDesc->Size) || > - (RemainingSize < 0)) > - { > - ASSERT (0); > - return; > - } > + if (ParserArray->Parser == NULL) { > + DEBUG ((DEBUG_ERROR, "Parser not implemented\n")); > + RemainingSize = 0; > + } else { > + PrintCmObjDesc ( > + (VOID *)((UINTN)CmObjDesc->Data + Offset), > + ParserArray->Parser, > + ParserArray->ItemCount, > + &RemainingSize, > + 1 > + ); > + if ((RemainingSize > CmObjDesc->Size) || > + (RemainingSize < 0)) > + { > + ASSERT (0); > + return; > + } > > - Offset = CmObjDesc->Size - RemainingSize; > + Offset = CmObjDesc->Size - RemainingSize; > + } > } // for > > ASSERT (RemainingSize == 0); -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109499): https://edk2.groups.io/g/devel/message/109499 Mute This Topic: https://groups.io/mt/101801385/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH 2/2] DynamicTablesPkg/TableHelperLib: Enhance error handling 2023-10-10 15:41 ` Jeshua Smith via groups.io @ 2023-10-11 12:51 ` PierreGondois 0 siblings, 0 replies; 11+ messages in thread From: PierreGondois @ 2023-10-11 12:51 UTC (permalink / raw) To: Jeshua Smith, devel@edk2.groups.io; +Cc: Sami.Mujawar@arm.com Hi Jeshua, On 10/10/23 17:41, Jeshua Smith wrote: > #3 is not currently used by any published code. It is a development aid, which the ObjectParser itself seems to be. > > Here's why I added it. Several people on our team have (not yet upstreamed) changes that resulted in additional ObjectIDs being added to the ObjectID enums, but without corresponding parsers being added to the ObjectParser. Dumping of the objects with the ObjectParser wasn't enabled by default, so this wasn't detected by them. Without #1 the ObjectParser code does out of bounds array accesses, sometimes leading to crashes. #1 will now detect and report that problem. When I enabled dumping of objects to debug my new code, I hit this bug leading me to write #1. For me to work around the issue of missing parsers in order to be able to continue debug with my work, the "easy" solution was for me to temporarily add the new ObjectIDs to the parser list with NULL parsers and then inform the responsible parties that they need to go and add parsers for their new ObjectIDs. Doing this required #3 (support for NULL parsers) to be added. Ideally any code that is upstreamed back to EDKII will have non-NULL parsers at the point it is sent upstream, but allowing in-development changes to temporarily use NULL parsers is helpful. > > Hopefully that clarifies things. Thanks for the detailed explanation, Reviewed-by: Pierre Gondois <pierre.gondois@arm.com> > > -----Original Message----- > From: Pierre Gondois <pierre.gondois@arm.com> > Sent: Tuesday, October 10, 2023 4:14 AM > To: Jeshua Smith <jeshuas@nvidia.com>; devel@edk2.groups.io > Cc: Sami.Mujawar@arm.com > Subject: Re: [PATCH 2/2] DynamicTablesPkg/TableHelperLib: Enhance error handling > > External email: Use caution opening links or attachments > > > Hello Jeshua, > > On 10/6/23 18:28, Jeshua Smith wrote: >> This patch enhances error handling and reporting in the CM ObjectParser. >> Specifically: >> 1. ObjectIDs used as array indexes are checked for being out of bounds, >> and if so an error message is printed before the assert. >> 2. An error message is printed for unsupported NameSpaceIDs. >> 3. Adds support for unimplemented parsers by allowing IDs to list a >> NULL parser, resulting in an unimplemented message being printed. > > I am not sure I see in which context 3. would be used/necessary. Is it possible to detail ? > > (Code-wise everything looks good to me) > > Regards, > Pierre > > >> >> Signed-off-by: Jeshua Smith <jeshuas@nvidia.com> >> --- >> .../ConfigurationManagerObjectParser.c | 47 +++++++++++++------ >> 1 file changed, 33 insertions(+), 14 deletions(-) >> >> diff --git >> a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO >> bjectParser.c >> b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO >> bjectParser.c >> index 92df1efee8..22b8fdb906 100644 >> --- >> a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO >> bjectParser.c >> +++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationMana >> +++ gerObjectParser.c >> @@ -795,6 +795,7 @@ STATIC CONST CM_OBJ_PARSER_ARRAY StdNamespaceObjectParser[] = { >> ARRAY_SIZE (StdObjAcpiTableInfoParser) }, >> { "EStdObjSmbiosTableList", StdObjSmbiosTableInfoParser, >> ARRAY_SIZE (StdObjSmbiosTableInfoParser) }, >> + { "EStdObjMax", NULL, 0} >> }; >> >> /** Print string data. >> @@ -1066,6 +1067,12 @@ ParseCmObjDesc ( >> return; >> } >> >> + if (ObjId >= ARRAY_SIZE (StdNamespaceObjectParser)) { >> + DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the StdNamespaceObjectParser array\n", ObjId)); >> + ASSERT (0); >> + return; >> + } >> + >> ParserArray = &StdNamespaceObjectParser[ObjId]; >> break; >> case EObjNameSpaceArm: >> @@ -1074,10 +1081,17 @@ ParseCmObjDesc ( >> return; >> } >> >> + if (ObjId >= ARRAY_SIZE (ArmNamespaceObjectParser)) { >> + DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the ArmNamespaceObjectParser array\n", ObjId)); >> + ASSERT (0); >> + return; >> + } >> + >> ParserArray = &ArmNamespaceObjectParser[ObjId]; >> break; >> default: >> // Not supported >> + DEBUG ((DEBUG_ERROR, "NameSpaceId 0x%x, ObjId 0x%x is not >> + supported by the parser\n", NameSpaceId, ObjId)); >> ASSERT (0); >> return; >> } // switch >> @@ -1095,21 +1109,26 @@ ParseCmObjDesc ( >> ObjIndex + 1, >> ObjectCount >> )); >> - PrintCmObjDesc ( >> - (VOID *)((UINTN)CmObjDesc->Data + Offset), >> - ParserArray->Parser, >> - ParserArray->ItemCount, >> - &RemainingSize, >> - 1 >> - ); >> - if ((RemainingSize > CmObjDesc->Size) || >> - (RemainingSize < 0)) >> - { >> - ASSERT (0); >> - return; >> - } >> + if (ParserArray->Parser == NULL) { >> + DEBUG ((DEBUG_ERROR, "Parser not implemented\n")); >> + RemainingSize = 0; >> + } else { >> + PrintCmObjDesc ( >> + (VOID *)((UINTN)CmObjDesc->Data + Offset), >> + ParserArray->Parser, >> + ParserArray->ItemCount, >> + &RemainingSize, >> + 1 >> + ); >> + if ((RemainingSize > CmObjDesc->Size) || >> + (RemainingSize < 0)) >> + { >> + ASSERT (0); >> + return; >> + } >> >> - Offset = CmObjDesc->Size - RemainingSize; >> + Offset = CmObjDesc->Size - RemainingSize; >> + } >> } // for >> >> ASSERT (RemainingSize == 0); -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109524): https://edk2.groups.io/g/devel/message/109524 Mute This Topic: https://groups.io/mt/101801385/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH 0/2] DynamicTablesPkg/TableHelperLib updates 2023-10-06 16:28 [edk2-devel] [PATCH 0/2] DynamicTablesPkg/TableHelperLib updates Jeshua Smith via groups.io 2023-10-06 16:28 ` [edk2-devel] [PATCH 1/2] DynamicTablesPkg/TableHelperLib: Fix and improve text handling Jeshua Smith via groups.io 2023-10-06 16:28 ` [edk2-devel] [PATCH 2/2] DynamicTablesPkg/TableHelperLib: Enhance error handling Jeshua Smith via groups.io @ 2023-10-23 15:30 ` Jeshua Smith via groups.io 2023-10-23 16:55 ` Sami Mujawar [not found] ` <1790CAE852EF102A.20272@groups.io> 4 siblings, 0 replies; 11+ messages in thread From: Jeshua Smith via groups.io @ 2023-10-23 15:30 UTC (permalink / raw) To: devel@edk2.groups.io; +Cc: Sami.Mujawar@arm.com, pierre.gondois@arm.com The patches in this series have both individually received: Reviewed-by: Pierre Gondois <pierre.gondois@arm.com> Can this be merged? -----Original Message----- From: Jeshua Smith <jeshuas@nvidia.com> Sent: Friday, October 6, 2023 10:28 AM To: devel@edk2.groups.io Cc: Sami.Mujawar@arm.com; pierre.gondois@arm.com; Jeshua Smith <jeshuas@nvidia.com> Subject: [PATCH 0/2] DynamicTablesPkg/TableHelperLib updates While using the ConfigurationManagerObjectParser to dump objects and debug adding new objects, I noticed some bugs and deficiencies. This series is intended to address those. Jeshua Smith (2): DynamicTablesPkg/TableHelperLib: Fix and improve text handling DynamicTablesPkg/TableHelperLib: Enhance error handling .../ConfigurationManagerObjectParser.c | 223 ++++++++++++++---- 1 file changed, 176 insertions(+), 47 deletions(-) -- 2.25.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109930): https://edk2.groups.io/g/devel/message/109930 Mute This Topic: https://groups.io/mt/101801382/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH 0/2] DynamicTablesPkg/TableHelperLib updates 2023-10-06 16:28 [edk2-devel] [PATCH 0/2] DynamicTablesPkg/TableHelperLib updates Jeshua Smith via groups.io ` (2 preceding siblings ...) 2023-10-23 15:30 ` [edk2-devel] [PATCH 0/2] DynamicTablesPkg/TableHelperLib updates Jeshua Smith via groups.io @ 2023-10-23 16:55 ` Sami Mujawar [not found] ` <1790CAE852EF102A.20272@groups.io> 4 siblings, 0 replies; 11+ messages in thread From: Sami Mujawar @ 2023-10-23 16:55 UTC (permalink / raw) To: Jeshua Smith, devel; +Cc: pierre.gondois, nd@arm.com Hi Jeshua, Thank you for these fixes. For this series, Reviewed-by: Sami Mujawar <sami.mujawar@arm.com> I have now queued this up for merging. Regards, Sami Mujawar On 06/10/2023 05:28 pm, Jeshua Smith wrote: > While using the ConfigurationManagerObjectParser to dump objects and debug > adding new objects, I noticed some bugs and deficiencies. This series is > intended to address those. > > Jeshua Smith (2): > DynamicTablesPkg/TableHelperLib: Fix and improve text handling > DynamicTablesPkg/TableHelperLib: Enhance error handling > > .../ConfigurationManagerObjectParser.c | 223 ++++++++++++++---- > 1 file changed, 176 insertions(+), 47 deletions(-) > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109932): https://edk2.groups.io/g/devel/message/109932 Mute This Topic: https://groups.io/mt/101801382/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <1790CAE852EF102A.20272@groups.io>]
* Re: [edk2-devel] [PATCH 0/2] DynamicTablesPkg/TableHelperLib updates [not found] ` <1790CAE852EF102A.20272@groups.io> @ 2023-10-23 17:10 ` Sami Mujawar 0 siblings, 0 replies; 11+ messages in thread From: Sami Mujawar @ 2023-10-23 17:10 UTC (permalink / raw) To: devel, Jeshua Smith; +Cc: pierre.gondois, nd@arm.com Merged as c591395f4ab5..ec7f73436646 Thanks. Regards, Sami Mujawar On 23/10/2023 05:55 pm, Sami Mujawar via groups.io wrote: > Hi Jeshua, > > Thank you for these fixes. > > For this series, > > Reviewed-by: Sami Mujawar <sami.mujawar@arm.com> > > I have now queued this up for merging. > > Regards, > > Sami Mujawar > > On 06/10/2023 05:28 pm, Jeshua Smith wrote: >> While using the ConfigurationManagerObjectParser to dump objects and >> debug >> adding new objects, I noticed some bugs and deficiencies. This series is >> intended to address those. >> >> Jeshua Smith (2): >> DynamicTablesPkg/TableHelperLib: Fix and improve text handling >> DynamicTablesPkg/TableHelperLib: Enhance error handling >> >> .../ConfigurationManagerObjectParser.c | 223 ++++++++++++++---- >> 1 file changed, 176 insertions(+), 47 deletions(-) >> > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109933): https://edk2.groups.io/g/devel/message/109933 Mute This Topic: https://groups.io/mt/101801382/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-10-23 17:10 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-10-06 16:28 [edk2-devel] [PATCH 0/2] DynamicTablesPkg/TableHelperLib updates Jeshua Smith via groups.io 2023-10-06 16:28 ` [edk2-devel] [PATCH 1/2] DynamicTablesPkg/TableHelperLib: Fix and improve text handling Jeshua Smith via groups.io 2023-10-10 10:13 ` PierreGondois 2023-10-10 14:57 ` Jeshua Smith via groups.io 2023-10-06 16:28 ` [edk2-devel] [PATCH 2/2] DynamicTablesPkg/TableHelperLib: Enhance error handling Jeshua Smith via groups.io 2023-10-10 10:13 ` PierreGondois 2023-10-10 15:41 ` Jeshua Smith via groups.io 2023-10-11 12:51 ` PierreGondois 2023-10-23 15:30 ` [edk2-devel] [PATCH 0/2] DynamicTablesPkg/TableHelperLib updates Jeshua Smith via groups.io 2023-10-23 16:55 ` Sami Mujawar [not found] ` <1790CAE852EF102A.20272@groups.io> 2023-10-23 17:10 ` Sami Mujawar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox