Re: [edk2-devel] [PATCH v1-resend 1/1] ShellPkg: Add revision check for DSDT Header on Arm

Re: [edk2-devel] [PATCH v1-resend 1/1] ShellPkg: Add revision check for DSDT Header on Arm

[Edward Pickup]

Hi Zhichao,

Would it be possible to get some feedback on this patch?

Regards,

Edward

On 20/07/2022 09:59, Edward Pickup via groups.io wrote:
> Bugzilla: 3995 (https://bugzilla.tianocore.org/show_bug.cgi?id=3995)
>
> ACPI 6.4 spec states that if the revision field in the DSDT header is less
> than 2, then all integers are restricted in width to 32 bits, including in
> SSDTs.
>
> Arm Base boot requirements state that platforms must conform to ACPI 6.3
> or later, and that legacy tables are not supported.
>
> Adds a check for this field and raise warning if revision is less
> than 2 on arm.
>
> Signed-off-by: Edward Pickup <edward.pickup@arm.com>
> ---
>
> The changes can be seen at https://github.com/edpickup/edk2/tree/2073_add_DSDT_revision_check_v1_resend
>
> Notes:
>      v1:
>        - Resending with rebase and bugzilla id
>        - No code changes since previous submit.
>          Ref: https://edk2.groups.io/g/devel/message/89378
>
>   ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dsdt/DsdtParser.c | 18 +++++++++++++++++-
>   1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dsdt/DsdtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dsdt/DsdtParser.c
> index e75e4622239ed03b171adc92d32b5eeb75a8edef..7f30a8409bb711d4edf77ed04319c628ef8aea73 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dsdt/DsdtParser.c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dsdt/DsdtParser.c
> @@ -1,7 +1,7 @@
>   /** @file
>
>     DSDT table parser
>
>   
>
> -  Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.
>
> +  Copyright (c) 2016 - 2022, ARM Limited. All rights reserved.
>
>     SPDX-License-Identifier: BSD-2-Clause-Patent
>
>   
>
>     @par Reference(s):
>
> @@ -39,4 +39,20 @@ ParseAcpiDsdt (
>     }
>
>   
>
>     DumpAcpiHeader (Ptr);
>
> +
>
> +  // As per 19.6.29 in the version 6.4 of the ACPI spec, a revision less than 2
>
> +  // restricts integers to 32 bit width. This may not be intended, raise a
>
> +  // warning
>
> + #if defined (MDE_CPU_AARCH64) || defined (MDE_CPU_ARM)
>
> +  if (AcpiTableRevision < 2) {
>
> +    IncrementWarningCount ();
>
> +    Print (
>
> +      L"WARNING: DSDT Table Revision less than 2. Integer width restricted to "
>
> +      L"32 bits. Table Revision = %d.\n",
>
> +      AcpiTableRevision
>
> +      );
>
> +    return;
>
> +  }
>
> +
>
> + #endif
>
>   }
>

Re: [edk2-devel] [PATCH v1-resend 1/1] ShellPkg: Add revision check for DSDT Header on Arm

[Gao, Zhichao]

Hi Edward,

Sorry, I missed this patch. Please feel free to inform the maintainers/reviewers if the patch didn't get response for a long time.
The patch looks good to me.
Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>

Thanks,
Zhichao

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Edward
> Pickup
> Sent: Tuesday, August 23, 2022 5:44 PM
> To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>
> Cc: Ni, Ray <ray.ni@intel.com>; sami.mujawar@arm.com; nd@arm.com
> Subject: Re: [edk2-devel] [PATCH v1-resend 1/1] ShellPkg: Add revision
> check for DSDT Header on Arm
> 
> Hi Zhichao,
> 
> Would it be possible to get some feedback on this patch?
> 
> Regards,
> 
> Edward
> 
> On 20/07/2022 09:59, Edward Pickup via groups.io wrote:
> > Bugzilla: 3995 (https://bugzilla.tianocore.org/show_bug.cgi?id=3995)
> >
> > ACPI 6.4 spec states that if the revision field in the DSDT header is
> > less than 2, then all integers are restricted in width to 32 bits,
> > including in SSDTs.
> >
> > Arm Base boot requirements state that platforms must conform to ACPI
> > 6.3 or later, and that legacy tables are not supported.
> >
> > Adds a check for this field and raise warning if revision is less than
> > 2 on arm.
> >
> > Signed-off-by: Edward Pickup <edward.pickup@arm.com>
> > ---
> >
> > The changes can be seen at
> >
> https://github.com/edpickup/edk2/tree/2073_add_DSDT_revision_check_v
> 1_
> > resend
> >
> > Notes:
> >      v1:
> >        - Resending with rebase and bugzilla id
> >        - No code changes since previous submit.
> >          Ref: https://edk2.groups.io/g/devel/message/89378
> >
> >
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dsdt/DsdtParser.c
> | 18 +++++++++++++++++-
> >   1 file changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git
> >
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dsdt/DsdtParser
> > .c
> >
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dsdt/DsdtParser
> > .c index
> >
> e75e4622239ed03b171adc92d32b5eeb75a8edef..7f30a8409bb711d4edf77ed0
> 4319
> > c628ef8aea73 100644
> > ---
> >
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dsdt/DsdtParser
> > .c
> > +++
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dsdt/DsdtPa
> > +++ rser.c
> > @@ -1,7 +1,7 @@
> >   /** @file
> >
> >     DSDT table parser
> >
> >
> >
> > -  Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.
> >
> > +  Copyright (c) 2016 - 2022, ARM Limited. All rights reserved.
> >
> >     SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >
> >
> >     @par Reference(s):
> >
> > @@ -39,4 +39,20 @@ ParseAcpiDsdt (
> >     }
> >
> >
> >
> >     DumpAcpiHeader (Ptr);
> >
> > +
> >
> > +  // As per 19.6.29 in the version 6.4 of the ACPI spec, a revision
> > + less than 2
> >
> > +  // restricts integers to 32 bit width. This may not be intended,
> > + raise a
> >
> > +  // warning
> >
> > + #if defined (MDE_CPU_AARCH64) || defined (MDE_CPU_ARM)
> >
> > +  if (AcpiTableRevision < 2) {
> >
> > +    IncrementWarningCount ();
> >
> > +    Print (
> >
> > +      L"WARNING: DSDT Table Revision less than 2. Integer width restricted
> to "
> >
> > +      L"32 bits. Table Revision = %d.\n",
> >
> > +      AcpiTableRevision
> >
> > +      );
> >
> > +    return;
> >
> > +  }
> >
> > +
> >
> > + #endif
> >
> >   }
> >
> 
> 
> 
>