Re: [PATCH edk2-platforms v1 5/6] Platform/ARM: Add OEM CPU generator for FVP

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Fri, 21 Dec 2018 at 19:14, Sami Mujawar <Sami.Mujawar@arm.com> wrote:
>
> Hi Ard,
>
> Please see my response inline.
>
> Regards,
>
> Sami Mujawar
>
> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Sent: 21 December 2018 05:08 PM
> To: Sami Mujawar <Sami.Mujawar@arm.com>
> Cc: edk2-devel@lists.01.org; Arvind Chauhan <Arvind.Chauhan@arm.com>; Daniil Egranov <Daniil.Egranov@arm.com>; Thomas Abraham <thomas.abraham@arm.com>; Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Alexei Fedorov <Alexei.Fedorov@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH edk2-platforms v1 5/6] Platform/ARM: Add OEM CPU generator for FVP
>
> Hi Sami,
>
> On Fri, 21 Dec 2018 at 18:01, Sami Mujawar <sami.mujawar@arm.com> wrote:
> >
> > Add support for dynamic generation of ACPI CPU device information.
> > This generator uses the compiled data from a template asl file and
> > patches it at runtime to generate the CPU information based on the
> > number of CPUs and their ACPI UID. This patched data is then installed
> > as a SSDT table.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> > ---
> >  Platform/ARM/VExpressPkg/ConfigurationManager/AcpiOemCpuGeneratorLib/AcpiOemCpuASLLib.inf       |  27 ++
> >  Platform/ARM/VExpressPkg/ConfigurationManager/AcpiOemCpuGeneratorLib/AcpiOemCpuGeneratorLib.inf |  42 ++
> >  Platform/ARM/VExpressPkg/ConfigurationManager/AcpiOemCpuGeneratorLib/OemCpuGenerator.c          | 403 ++++++++++++++++++++
> >  Platform/ARM/VExpressPkg/ConfigurationManager/AcpiOemCpuGeneratorLib/OemCpuGenerator.h          |  23 ++
> >  Platform/ARM/VExpressPkg/ConfigurationManager/AcpiOemCpuGeneratorLib/SsdtCpuTemplate.asl        |  25 ++
> >  5 files changed, 520 insertions(+)
> >
> ...
> > diff --git
> > a/Platform/ARM/VExpressPkg/ConfigurationManager/AcpiOemCpuGeneratorLib
> > /OemCpuGenerator.c
> > b/Platform/ARM/VExpressPkg/ConfigurationManager/AcpiOemCpuGeneratorLib
> > /OemCpuGenerator.c
> > new file mode 100644
> > index
> > 0000000000000000000000000000000000000000..d544244bfbd566c128b57d80b0c8
> > c2bdc0cca374
> > --- /dev/null
> > +++ b/Platform/ARM/VExpressPkg/ConfigurationManager/AcpiOemCpuGenerato
> > +++ rLib/OemCpuGenerator.c
> > @@ -0,0 +1,403 @@
> > +/** @file
> > +  OEM CPU Table Generator
> > +
> > +  Copyright (c) 2018, ARM Limited. All rights reserved.
> > +  This program and the accompanying materials  are licensed and made
> > + available under the terms and conditions of the BSD License  which
> > + accompanies this distribution.  The full text of the license may be
> > + found at  http://opensource.org/licenses/bsd-license.php
> > +
> > +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> > +BASIS,
> > +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> > +**/
> > +
> > +#include <Library/AcpiLib.h>
> > +#include <Library/BaseLib.h>
> > +#include <Library/BaseMemoryLib.h>
> > +#include <Library/DebugLib.h>
> > +#include <Library/MemoryAllocationLib.h> #include
> > +<Library/UefiBootServicesTableLib.h>
> > +#include <Protocol/AcpiTable.h>
> > +
> > +// Module specific include files.
> > +#include <AcpiTableGenerator.h>
> > +#include <ConfigurationManagerObject.h> #include
> > +<ConfigurationManagerHelper.h> #include <Library/TableHelperLib.h>
> > +#include <Protocol/ConfigurationManagerProtocol.h>
> > +#include <Protocol/DynamicTableFactoryProtocol.h>
> > +
> > +#include <OemCpuGenerator.h>
> > +
> > +// AML Code Include files generated by iASL Compiler #include
> > +<SsdtCpuTemplate.hex>
> > +
> > +// AML Code offsets file generated by iASL Compiler #include
> > +<SsdtCpuTemplate.offset.h>
> > +
>
> Apologies if I should have spotted this before, but this is a no-go.
> We are relying on intermediate output of some version of the IASL compiler here, which [AFAIK] is not formally specified or documented.
> We cannot base an elaborate framework like DynamicTables on this.
>
> I guess this only affects DSDT/SSDT generation, right?
> [SAMI] Yes. The last 2 patches in this series add this feature. We probably need more discussion on this topic.
> Until then can we proceed with review of the remaining patches, or should I submit a new patch series that drops the last 2 patches?
>

Could you look into whether EFI's ACPI SDT protocol has what you need
to manipulate AML at runtime?