Re: [edk2-devel] [PATCH v5 5/6] ShellPkg: acpiview: Add routines to print reserved fields

["Sami Mujawar" <sami.mujawar@arm.com>]

Hi Rohit,

I have a minor suggestion marked inline as [SAMI].

Otherwise this patch looks good to me.

With that addressed,

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

On 02/10/2023 06:17 pm, Rohit Mathew wrote:
> Most of the ACPI tables have fields that are marked reserved. Implement
> functions "DumpReserved" and "DumpReservedBits" aligning with the
> print-formatter prototype to print out reserved fields.
>
> Signed-off-by: Rohit Mathew<Rohit.Mathew@arm.com>
> Cc: James Morse<james.Morse@arm.com>
> Cc: Sami Mujawar<sami.mujawar@arm.com>
> Cc: Thomas Abraham<thomas.abraham@arm.com>
> Cc: Zhichao Gao<zhichao.gao@intel.com>
> ---
>   ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c | 126 ++++++++++++++++++++
>   ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h |  38 ++++++
>   2 files changed, 164 insertions(+)
>
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> index 20d817e357..cc520d9b27 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> @@ -498,6 +498,132 @@ Dump16Chars (
>       );
>   }
>   
> +/**
> +  This function traces reserved fields up to 8 bytes in length.
> +
> +  Format string is ignored by this function as the reserved field is printed
> +  byte by byte with intermittent spacing <eg: 0 0 0 0>. Use DumpxChars for any
> +  other use case.
> +  @param [in] Format  Optional format string for tracing the data.
> +  @param [in] Ptr     Pointer to the start of the buffer.
> +  @param [in] Length  Length of the field.
> +**/
> +VOID
> +EFIAPI
> +DumpReserved (
> +  IN CONST CHAR16  *Format OPTIONAL,
> +  IN UINT8         *Ptr,
> +  IN UINT32        Length
> +  )
> +{
> +  switch (Length) {
> +    case 8:
> +      Print (
> +        L"%u %u %u %u %u %u %u %u",
> +        Ptr[0],
> +        Ptr[1],
> +        Ptr[2],
> +        Ptr[3],
> +        Ptr[4],
> +        Ptr[5],
> +        Ptr[6],
> +        Ptr[7]
> +        );
> +      break;
> +    case 7:
> +      Print (
> +        L"%u %u %u %u %u %u %u",
> +        Ptr[0],
> +        Ptr[1],
> +        Ptr[2],
> +        Ptr[3],
> +        Ptr[4],
> +        Ptr[5],
> +        Ptr[6]
> +        );
> +      break;
> +    case 6:
> +      Print (
> +        L"%u %u %u %u %u %u",
> +        Ptr[0],
> +        Ptr[1],
> +        Ptr[2],
> +        Ptr[3],
> +        Ptr[4],
> +        Ptr[5]
> +        );
> +      break;
> +    case 5:
> +      Print (
> +        L"%u %u %u %u %u",
> +        Ptr[0],
> +        Ptr[1],
> +        Ptr[2],
> +        Ptr[3],
> +        Ptr[4]
> +        );
> +      break;
> +    case 4:
> +      Print (
> +        L"%u %u %u %u",
> +        Ptr[0],
> +        Ptr[1],
> +        Ptr[2],
> +        Ptr[3]
> +        );
> +      break;
> +    case 3:
> +      Print (
> +        L"%u %u %u",
> +        Ptr[0],
> +        Ptr[1],
> +        Ptr[2]
> +        );
> +      break;
> +    case 2:
> +      Print (
> +        L"%u %u",
> +        Ptr[0],
> +        Ptr[1]
> +        );
> +      break;
> +    case 1:
> +      Print (
> +        L"%u",
> +        Ptr[0]
> +        );
> +      break;
> +    default:
> +      return;
> +  }
> +}
> +
> +/**
> +  This function traces reserved fields up to 64 bits in length.
> +
> +  Format string is ignored by this function as the reserved field is printed
> +  byte by byte with intermittent spacing. eg: <0 0 0 0>. When the field length
> +  isn't a multiple of 8, the number of bytes are "ceil"-ed by one. eg for 27
> +  bits <0 0 0 0>
> +
> +  @param [in] Format  Optional format string for tracing the data.
> +  @param [in] Ptr     Pointer to the start of the buffer.
> +  @param [in] Length  Length of the field.
[SAMI] I think the documentation for the Length filed should say 
something like 'Length of the field as number of bits.'
> +**/
> +VOID
> +EFIAPI
> +DumpReservedBits (
> +  IN CONST CHAR16  *Format OPTIONAL,
> +  IN UINT8         *Ptr,
> +  IN UINT32        Length
> +  )
> +{
> +  UINT32  ByteLength;
> +
> +  ByteLength = (Length + 7) >> 3;
> +  DumpReserved (Format, Ptr, ByteLength);
> +}
> +
>   /**
>     This function indents and prints the ACPI table Field Name.
>   
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> index 80bac6a584..89930fdc6d 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> @@ -230,6 +230,44 @@ Dump16Chars (
>     IN UINT32        Length
>     );
>   
> +/**
> +  This function traces reserved fields up to 8 bytes in length.
> +
> +  Format string is ignored by this function as the reserved field is printed
> +  byte by byte with intermittent spacing <eg: 0 0 0 0>. Use DumpxChars for any
> +  other use case.
> +  @param [in] Format  Optional format string for tracing the data.
> +  @param [in] Ptr     Pointer to the start of the buffer.
> +  @param [in] Length  Length of the field.
> +**/
> +VOID
> +EFIAPI
> +DumpReserved (
> +  IN CONST CHAR16  *Format OPTIONAL,
> +  IN UINT8         *Ptr,
> +  IN UINT32        Length
> +  );
> +
> +/**
> +  This function traces reserved fields up to 64 bits in length.
> +
> +  Format string is ignored by this function as the reserved field is printed
> +  byte by byte with intermittent spacing. eg: <0 0 0 0>. When the field length
> +  isn't a multiple of 8, the number of bytes are "ceil"-ed by one. eg for 27
> +  bits <0 0 0 0>
> +
> +  @param [in] Format  Optional format string for tracing the data.
> +  @param [in] Ptr     Pointer to the start of the buffer.
> +  @param [in] Length  Length of the field.
[SAMI] Please see suggestion above.
> +**/
> +VOID
> +EFIAPI
> +DumpReservedBits (
> +  IN CONST CHAR16  *Format OPTIONAL,
> +  IN UINT8         *Ptr,
> +  IN UINT32        Length
> +  );
> +
>   /**
>     This function indents and prints the ACPI table Field Name.
>   


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112077): https://edk2.groups.io/g/devel/message/112077
Mute This Topic: https://groups.io/mt/101716966/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-