Re: [coreboot] [SeaBIOS] Compiling SeaBIOS for coreboot has problems with its ACPI code

[Michael S. Tsirkin]

On Thu, Aug 09, 2012 at 02:59:45AM +0000, Moore, Robert wrote:
> > -----Original Message-----
> > From: Kevin O'Connor [mailto:[e-mail]]
> > Sent: Wednesday, August 08, 2012 7:23 PM
> > To: Moore, Robert
> > Cc: Michael S. Tsirkin; Idwer Vollering; [e-mail]; Tang, Feng;
> > [e-mail]
> > Subject: Re: [SeaBIOS] Compiling SeaBIOS for coreboot has problems with its
> > ACPI code
> >
> > On Tue, Aug 07, 2012 at 07:34:37PM +0000, Moore, Robert wrote:
> > > This is very interesting. If I understand correctly, you are using a
> > > utility plus various directives to generate tables of AML offsets --
> > > presumably in order to dynamically change AML values, correct?
> >
> > Yes - exactly.  We started off completely generating the SSDT
> > dynamically, but that got awkward when we needed to generate Processor
> > objects dynamically.  So, we compiled an SSDT as template and then
> > patched it.  It was fragile to hard-code the offsets, so a tool was
> > written to parse the IASL output and generate the offsets
> > automatically.
> >
> > I'm CC'ing the coreboot list - they have also been doing SSDT runtime
> > generation.
> > > I have to say that I have not seen anything like this, from any BIOS
> >
> > vendor.
> > > > By the way, is there interest in adding some of the functionality that
> > > > we get by parsing the listing to iasl directly?
> > >
> > > We are always interested in adding features to make the compiler
> > > more useful. What would you suggest?
> >
> > The tool currently generates offsets and can rename the Amlcode[]
> > variable to something more unique.  I'm sure additional features could
> > be thought up.
> >
> > -Kevin
>
> Here are additional thoughts and questions that I sent out earlier today:
> > From what we have seen, standard BIOS code often modifies the AML on the fly (before the OS gets the ACPI tables) to handle the various setup options and other things. Some vendors do this better than others (we've seen checksum errors because of this, as well as package length issues because, for example, a byte constant is replaced by a dword constant but there is no room for the 32-bit value.)
>
> Obviously, the iASL compiler knows the offset of not only the start of every ASL statement, but also the start of every argument of every ASL statement.
>
> What we would be willing to provide is a new type of output file that provides arrays of offsets for you to use when modifying the AML. Something like this should be useful for your project, as well as standard BIOS code that needs this information as well.
>
> The -sc option for iASL already provides a limited version of this, but only at the statement level with offsets within a trailing comment field:
> /*
>  *      10....{
>  *      11....    Method (MAIN, 0, NotSerialized)
>  */
> unsigned char    DSDT_Template_MAIN [] =
> {
>     0x14,0x08,0x4D,0x41,0x49,0x4E,0x00,         /* 00000023    "..MAIN." */
>
> /*
>  *      12....    {
>  *      13....        Return (Zero)
>  */
>     0xA4,0x00,                                  /* 00000025    ".." */
>
> A new type of output file could instead create arrays of offsets, one offset per ASL term (statement or argument). The format could be something like one array per ASL statement or one array per method. You could then pick and choose which of these arrays you want to use/include into your BIOS code.
>
> Does this sound like it would solve your issues?
>
> Thanks,
> Bob

Hmm. How would C code know which of the statements it needs?
For example, we could have many Return(Zero) statements but only want
to patch some of them.

Would it be possible to allow some tags in the AML code
that specifies which offsets to extract, and how to name the
arrays?

It would also be useful to specify the array names
for the AML code, to avoid dependencies
between C code and makefiles.

For example, code could look like this:

/* Pull in required macros, also allows iasl
   to figure out it is not in compatibility mode.  */
#include <acpi-extract.h>

/* Put all code in this definition block in AmlCode array */
ACPI_EXTRACT_CODE(AmlCode)
DefinitionBlock ("ssdt-pcihp.aml", "SSDT", 0x01, "BXPC", "BXSSDTPCIHP", 0x1)

Name (_S4,
    	/* Put offset of next argument in acpi_s4_pkg array */
    	ACPI_EXTRACT_OFFSET(acpi_s4_pkg)
    	Package (0x04)
    	{
    	0x2,  /* PM1a_CNT.SLP_TYP */
    	0x2,  /* PM1b_CNT.SLP_TYP */
    	Zero,  /* reserved */
    	Zero   /* reserved */
    	})

One other thing to note about our scripts is that
offsets are often small, our scripts make the
array the correct type (char,short,int,long)
to make it fit, to avoid wasting memory.
You might want to do the same.