Thanks for the review. Reply inline. -----Original Message----- From: Pierre Gondois <pierre.gond...@arm.com> Sent: Tuesday, October 10, 2023 4:14 AM To: Jeshua Smith <jesh...@nvidia.com>; devel@edk2.groups.io Cc: sami.muja...@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.gond...@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 <jesh...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-