[PATCH] Add the AML code generator for runtime code generation

[Rudolf Marek]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello,

Following patch adds dynamic ACPI AML code generator which can be used to
generate run-time ACPI ASL code like this:

Name (XXX, 0xXX)

Or:

    Scope (\_SB.PCI0)
    {
 Name (BUSN, Package (0x04)
{
    0x11111111,
    0x22222222,
    0x33333333,
    0x44444444
})

Moreover it demonstrates its use on Asus M2V-MX SE where the SSDT table is
generated by new function k8acpi_write_vars (technically similar to
update_ssdt). But lot of nicer ;)

The ACPI binary code generator will be also useful for P-states generation,
without ugly run-time patching of DSDT.

The old SSDT of K8 will be removed and rest of boards converted in next patch.

Tested on that board. It generates same table as the static SSDT patched by
amdk8_acpi. Windows still boots too ;)

Signed-off-by: Rudolf Marek <[e-mail]>

Rudolf
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkmFx6QACgkQ3J9wPJqZRNVoRACg1/x4GUW640LFMu3XHMwBtqV6
ODkAn2iWNSMp+pa/xZEbcziG0Il39KLO
=CNJT
-----END PGP SIGNATURE-----

[PATCH] Add the AML code generator for runtime code generation

[Peter Stuge]

Rudolf Marek wrote:
> Following patch adds dynamic ACPI AML code generator which can be
> used to generate run-time ACPI ASL code like this:
>
> Name (XXX, 0xXX)
>
> Or:
>
> Scope (\_SB.PCI0)
>     {
>  Name (BUSN, Package (0x04)
> {
>     0x11111111,
>     0x22222222,
>     0x33333333,
>     0x44444444
> })
>
> Moreover it demonstrates its use on Asus M2V-MX SE where the SSDT table is
> generated by new function k8acpi_write_vars (technically similar to
> update_ssdt). But lot of nicer ;)
>
> The ACPI binary code generator will be also useful for P-states generation,
> without ugly run-time patching of DSDT.
>
> The old SSDT of K8 will be removed and rest of boards converted in next patch.
>
> Tested on that board. It generates same table as the static SSDT patched by
> amdk8_acpi. Windows still boots too ;)
>
> Signed-off-by: Rudolf Marek <[e-mail]>

Acked-by: Peter Stuge <[e-mail]>

[PATCH] Add the AML code generator for runtime code generation

[Rudolf Marek]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Commited as Committed revision 3925.

Thanks,
Rudolf
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkmF68EACgkQ3J9wPJqZRNVH2gCeNNYVACeFaZrNnNna9RFdNbIw
hfQAnRm1n/aB8mbVmAkRB8+sNY+V+PCj
=rFna
-----END PGP SIGNATURE-----

[PATCH] Add the AML code generator for runtime code generation

[Carl-Daniel Hailfinger]

Hi Rudolf,

that's an impressive piece of code!

On 01.02.2009 17:02, Rudolf Marek wrote:
> Hello,
>
> Following patch adds dynamic ACPI AML code generator which can be used to
> generate run-time ACPI ASL code like this:
>
> Name (XXX, 0xXX)
>
> Or:
>
> Scope (\_SB.PCI0)
>     {
>  Name (BUSN, Package (0x04)
> {
>     0x11111111,
>     0x22222222,
>     0x33333333,
>     0x44444444
> })
>
> Moreover it demonstrates its use on Asus M2V-MX SE where the SSDT table is
> generated by new function k8acpi_write_vars (technically similar to
> update_ssdt). But lot of nicer ;)
>
> The ACPI binary code generator will be also useful for P-states
> generation,
> without ugly run-time patching of DSDT.
>
> The old SSDT of K8 will be removed and rest of boards converted in
> next patch.
>
> Tested on that board. It generates same table as the static SSDT
> patched by
> amdk8_acpi. Windows still boots too ;)
>
> Signed-off-by: Rudolf Marek <[e-mail]>

The following review is rather short and to-the-point. No offense intended.

A few comments about the general design:
The global variable gencurrent is a bit irritating. How do you handle
running on multiple cores?
Same for len_stack. Besides that, both variables are in the global
namespace and could use some acpi_ prefix or suffix.
> Index: coreboot-v2/src/arch/i386/boot/acpi.c
> ===================================================================
> --- coreboot-v2.orig/src/arch/i386/boot/acpi.c	2008-12-23 17:31:37.000000000 +0100
> +++ coreboot-v2/src/arch/i386/boot/acpi.c	2009-02-01 11:05:03.171807310 +0100
> @@ -24,6 +24,7 @@
>  #include <console/console.h>
>  #include <string.h>
>  #include <arch/acpi.h>
> +#include <arch/acpigen.h>
>  #include <device/pci.h>
>
> u8 acpi_checksum(u8 *table, u32 length)
> @@ -180,6 +181,34 @@
>   header->checksum	= acpi_checksum((void *)mcfg, header->length);
>  }
>
> +/* this can be overriden by platform ACPI setup code,
> +   if it calls acpi_create_ssdt_generator */
> +unsigned long __attribute__((weak)) acpi_fill_ssdt_generator(unsigned long current,

I think weak symbols are something that makes code harder to follow.
> +						    char *oem_table_id) {

The { belongs on another line.
> +	return current;
> +}
> +
> +void acpi_create_ssdt_generator(acpi_header_t *ssdt, char *oem_table_id)

Strange function name.
> +{
> +	unsigned long current=(unsigned long)ssdt+sizeof(acpi_header_t);
> +	memset((void *)ssdt, 0, sizeof(acpi_header_t));
> +	memcpy(&ssdt->signature, SSDT_NAME, 4);

Maybe strncpy instead?
> +	ssdt->revision = 2;
> +	memcpy(&ssdt->oem_id, OEM_ID, 6);
> +	memcpy(&ssdt->oem_table_id, oem_table_id, 8);
> +	ssdt->oem_revision = 42;
> +	memcpy(&ssdt->asl_compiler_id, "GENAML", 4);
> +	ssdt->asl_compiler_revision = 42;

Heh.
> +	ssdt->length = sizeof(acpi_header_t);
> +
> +	acpigen_set_current((unsigned char *) current);
> +	current = acpi_fill_ssdt_generator(current, oem_table_id);
> +
> +	/* recalculate length */
> +	ssdt->length = current - (unsigned long)ssdt;
> +	ssdt->checksum = acpi_checksum((void *)ssdt, ssdt->length);
> +}
> +
>  int acpi_create_srat_lapic(acpi_srat_lapic_t *lapic, u8 node, u8 apic)
>  {
>          lapic->type=0;
> Index: coreboot-v2/src/arch/i386/boot/acpigen.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ coreboot-v2/src/arch/i386/boot/acpigen.c	2009-02-01 11:04:09.927808334 +0100
> @@ -0,0 +1,138 @@
> +/*
> + * This file is part of the coreboot project.
> + *
> + * Copyright (C) 2009 Rudolf Marek <[e-mail]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License v2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> + */
> +
> +/* how many nesting we support */

"how many nesting levels do we support"
> +#define ACPIGEN_LENSTACK_SIZE 10
> +
> +/* if you need to change this, change the acpigen_write_f and
> +   acpigen_patch_len */
> +

Can you remove the empty line above?
> +#define ACPIGEN_MAXLEN 0xfff
> +
> +#include <string.h>
> +#include <arch/acpigen.h>
> +#include <console/console.h>
> +
> +static char *gencurrent;
> +
> +char *len_stack[ACPIGEN_LENSTACK_SIZE];
> +int ltop = 0;

These global variables need better names and some comments explaining
what they do.
> +
> +static int acpigen_write_len_f()

Needs a doxygen comment.
> +{
> +	ASSERT(ltop < (ACPIGEN_LENSTACK_SIZE - 1))
> +	len_stack[ltop++] = gencurrent;
> +	acpigen_emit_byte(0);
> +	acpigen_emit_byte(0);
> +	return 2;
> +}
> +
> +void acpigen_patch_len(int len)

Needs a doxygen comment.
> +{
> +	ASSERT(len <= ACPIGEN_MAXLEN)
> +	ASSERT(ltop > 0)
> +	char *p = len_stack[--ltop];
> +	/* generate store length for 0xfff max */
> +	p[0] = (0x40 | (len & 0xf));
> +	p[1] = (len >> 4 & 0xff);
> +
> +}
> +
> +void acpigen_set_current(char *curr) {

Needs a doxygen comment.
> +    gencurrent = curr;
> +}
> +
> +char *acpigen_get_current(void) {

Needs a doxygen comment.
> +    return gencurrent;
> +}
> +
> +int acpigen_emit_byte(unsigned char b)
> +{
> +	(*gencurrent++) = b;
> +	return 1;
> +}
> +
> +int acpigen_write_package(int nr_el)
> +{
> +	int len;
> +	/* package op */
> +	acpigen_emit_byte(0x12);
> +	len = acpigen_write_len_f();
> +	acpigen_emit_byte(nr_el);
> +	return len + 2;
> +}
> +
> +int acpigen_write_byte(unsigned int data)
> +{
> +	/* byte op */
> +	acpigen_emit_byte(0xa);
> +	acpigen_emit_byte(data & 0xff);
> +	return 2;
> +}
> +
> +int acpigen_write_dword(unsigned int data)
> +{
> +	/* dword op */
> +	acpigen_emit_byte(0xc);
> +	acpigen_emit_byte(data & 0xff);
> +	acpigen_emit_byte((data >> 8) & 0xff);
> +	acpigen_emit_byte((data >> 16) & 0xff);
> +	acpigen_emit_byte((data >> 24) & 0xff);
> +	return 5;
> +}
> +
> +int acpigen_write_name_byte(char *name, uint8_t val) {
> +	int len;
> +	len = acpigen_write_name(name);
> +	len += acpigen_write_byte(val);
> +	return len;
> +}
> +
> +int acpigen_write_name_dword(char *name, uint32_t val) {
> +	int len;
> +	len = acpigen_write_name(name);
> +	len += acpigen_write_dword(val);
> +	return len;
> +}
> +
> +int acpigen_emit_stream(char *data, int size) {
> +	int i;
> +	for (i = 0; i < size; i++) {
> +		acpigen_emit_byte(data[i]);
> +	}
> +	return size;
> +}
> +
> +int acpigen_write_name(char *name)
> +{
> +	int len = strlen(name);
> +	/* name op */
> +	acpigen_emit_byte(0x8);
> +	acpigen_emit_stream(name, len);
> +	return len + 1;
> +}
> +
> +int acpigen_write_scope(char *name)
> +{
> +	int len;
> +	/* scope op */
> +	acpigen_emit_byte(0x10);
> +	len = acpigen_write_len_f();
> +	return len + acpigen_emit_stream(name, strlen(name)) + 1;
> +}
> Index: coreboot-v2/src/arch/i386/boot/Config.lb
> ===================================================================
> --- coreboot-v2.orig/src/arch/i386/boot/Config.lb	2009-02-01 10:25:41.491806925 +0100
> +++ coreboot-v2/src/arch/i386/boot/Config.lb	2009-02-01 10:30:56.756876765 +0100
> @@ -13,5 +13,5 @@
>  end
>  if HAVE_ACPI_TABLES
>  object acpi.o
> +object acpigen.o
>  end
> -
> Index: coreboot-v2/src/arch/i386/include/arch/acpigen.h
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ coreboot-v2/src/arch/i386/include/arch/acpigen.h	2009-02-01 10:30:56.758807179 +0100
> @@ -0,0 +1,37 @@
> +/*
> + * This file is part of the coreboot project.
> + *
> + * Copyright (C) 2009 Rudolf Marek <[e-mail]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License v2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> + */
> +#ifndef LIBACPI_H
> +#define LIBACPI_H
> +#include <assert.h>
> +#include <stdlib.h>
> +#include <stdint.h>
> +
> +void acpigen_patch_len(int len);
> +void acpigen_set_current(char *curr);
> +char *acpigen_get_current(void);
> +int acpigen_write_package(int nr_el);
> +int acpigen_write_byte(unsigned int data);
> +int acpigen_emit_byte(unsigned char data);
> +int acpigen_emit_stream(char *data, int size);
> +int acpigen_write_dword(unsigned int data);
> +int acpigen_write_name(char *name);
> +int acpigen_write_name_dword(char *name, uint32_t val);
> +int acpigen_write_name_byte(char *name, uint8_t val);
> +int acpigen_write_scope(char *name);
> +#endif
> Index: coreboot-v2/src/arch/i386/include/arch/acpi.h
> ===================================================================
> --- coreboot-v2.orig/src/arch/i386/include/arch/acpi.h	2008-12-23 17:38:41.000000000 +0100
> +++ coreboot-v2/src/arch/i386/include/arch/acpi.h	2009-02-01 10:52:25.478808600 +0100
> @@ -29,6 +29,7 @@
>  #define MCFG_NAME             "MCFG"
>  #define SRAT_NAME             "SRAT"
>  #define SLIT_NAME	      "SLIT"
> +#define SSDT_NAME	      "SSDT"
>
> #define RSDT_TABLE            "RSDT    "
>  #define HPET_TABLE            "AMD64   "
> @@ -293,6 +294,8 @@
>  unsigned long acpi_fill_madt(unsigned long current);
>  unsigned long acpi_fill_mcfg(unsigned long current);
>  unsigned long acpi_fill_srat(unsigned long current); 
> +unsigned long acpi_fill_ssdt_generator(unsigned long current, char *oem_table_id);
> +void acpi_create_ssdt_generator(acpi_header_t *ssdt, char *oem_table_id);

The two functions above have rather similar names and it's not
immedately ofvious how they differ.
> void acpi_create_fadt(acpi_fadt_t *fadt,acpi_facs_t *facs,void *dsdt);
>
> /* These can be used by the target port */
> Index: coreboot-v2/src/mainboard/asus/m2v-mx_se/acpi_tables.c
> ===================================================================
> --- coreboot-v2.orig/src/mainboard/asus/m2v-mx_se/acpi_tables.c	2008-12-23 17:46:56.000000000 +0100
> +++ coreboot-v2/src/mainboard/asus/m2v-mx_se/acpi_tables.c	2009-02-01 11:23:05.142807423 +0100
> @@ -30,9 +30,9 @@
>  #include <device/pci_ids.h>
>  #include <../../../southbridge/via/vt8237r/vt8237r.h>
>  #include <../../../southbridge/via/k8t890/k8t890.h>
> +#include <../../../northbridge/amd/amdk8/amdk8_acpi.h>
>
> extern unsigned char AmlCode[];
> -extern unsigned char AmlCode_ssdt[];
>
> unsigned long acpi_fill_mcfg(unsigned long current)
>  {
> @@ -81,6 +81,12 @@
>   return current;
>  }
>
> +unsigned long acpi_fill_ssdt_generator(unsigned long current, char *oem_table_id) {
> +	k8acpi_write_vars();
> +	/* put PSTATES generator call here */
> +	return (unsigned long) (acpigen_get_current());

acpigen_get_current is a really confusingly named function.
> +}
> +
>  unsigned long write_acpi_tables(unsigned long start)
>  {
>   unsigned long current;
> @@ -175,13 +181,10 @@
>   /* SSDT */
>   printk_debug("ACPI:    * SSDT\n");
>   ssdt = (acpi_header_t *)current;
> -	current += ((acpi_header_t *)AmlCode_ssdt)->length;
> -	memcpy((void *)ssdt, (void *)AmlCode_ssdt, ((acpi_header_t *)AmlCode_ssdt)->length);
> -	update_ssdt((void*)ssdt);
> -        /* recalculate checksum */
> -        ssdt->checksum = 0;
> -        ssdt->checksum = acpi_checksum((unsigned char *)ssdt,ssdt->length);
> -	acpi_add_table(rsdt,ssdt);
> +
> +	acpi_create_ssdt_generator(ssdt, "DYNADATA");

"COREBOOT" please. That way, people can actually find out where the
table came from. The fact that the data are generated dynamically is
already visible from the compiler ID.
> +	current += ssdt->length;
> +	acpi_add_table(rsdt, ssdt);
>
> printk_info("ACPI: done.\n");
>   return current;
> Index: coreboot-v2/src/northbridge/amd/amdk8/amdk8_acpi.c
> ===================================================================
> --- coreboot-v2.orig/src/northbridge/amd/amdk8/amdk8_acpi.c	2008-12-23 17:31:36.000000000 +0100
> +++ coreboot-v2/src/northbridge/amd/amdk8/amdk8_acpi.c	2009-02-01 11:26:00.102959065 +0100
> @@ -259,6 +259,80 @@
>   }
>  }
>
> +static int k8acpi_write_HT(void) {
> +	device_t dev;
> +	uint32_t dword;
> +	int len, lenp, i;
> +
> +	len = acpigen_write_name("HCLK");
> +	lenp = acpigen_write_package(HC_POSSIBLE_NUM);
> +
> +	for(i=0;i<sysconf.hc_possible_num;i++) {
> +		lenp += acpigen_write_dword(sysconf.pci1234[i]);
> +	}
> +	for(i=sysconf.hc_possible_num; i<HC_POSSIBLE_NUM; i++) { // in case we set array size to other than 8

Comment on a separate line, please.
> +		lenp += acpigen_write_dword(0x0);
> +	}
> +
> +	acpigen_patch_len(lenp - 1);

acpigen_patch_len is a name which does not really reveal function purpose.
> +	len += lenp;
> +
> +	len += acpigen_write_name("HCDN");
> +	lenp = acpigen_write_package(HC_POSSIBLE_NUM);
> +
> +	for(i=0;i<sysconf.hc_possible_num;i++) {
> +		lenp += acpigen_write_dword(sysconf.hcdn[i]);
> +	}
> +	for(i=sysconf.hc_possible_num; i<HC_POSSIBLE_NUM; i++) { // in case we set array size to other than 8
> +		lenp += acpigen_write_dword(0x20202020);
> +	}
> +	acpigen_patch_len(lenp - 1);
> +	len += lenp;
> +
> +	return len;
> +}
> +
> +static int k8acpi_write_pci_data(int dlen, char *name, int offset) {
> +	device_t dev;
> +	uint32_t dword;
> +	int len, lenp, i;
> +
> +	dev = dev_find_slot(0, PCI_DEVFN(0x18, 1));
> +
> +	len = acpigen_write_name(name);
> +	lenp = acpigen_write_package(dlen);
> +	for(i=0; i<dlen; i++) {
> +		dword = pci_read_config32(dev, offset+i*4);
> +		lenp += acpigen_write_dword(dword);
> +	}
> +	// minus the opcode
> +	acpigen_patch_len(lenp - 1);
> +	return len + lenp;
> +}
> +
> +
> +int k8acpi_write_vars(void)
> +{
> +	int lens;

Maybe use len instead of lens?
> +	msr_t msr;
> +	char pscope[] = "\\._SB_PCI0";
> +
> +	lens = acpigen_write_scope(pscope);
> +	lens += k8acpi_write_pci_data(4, "BUSN", 0xe0);
> +	lens += k8acpi_write_pci_data(8, "PCIO", 0xc0);
> +	lens += k8acpi_write_pci_data(16, "MMIO", 0x80);
> +	lens += acpigen_write_name_byte("SBLK", sysconf.sblk);
> +	lens += acpigen_write_name_byte("CBST",
> +	    ((sysconf.pci1234[0] >> 12) & 0xff) ? 0xf : 0x0);
> +	lens += acpigen_write_name_dword("SBDN", sysconf.sbdn);
> +	msr = rdmsr(TOP_MEM);
> +	lens += acpigen_write_name_dword("TOM1", msr.lo);

I know the original code did not have it, but can you please add TOM2?
> +
> +	lens += k8acpi_write_HT();
> +	//minus opcode
> +	acpigen_patch_len(lens - 1);
> +	return lens;
> +}
>
> // used by acpi_tables.h
>
> Index: coreboot-v2/src/northbridge/amd/amdk8/amdk8_acpi.h
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ coreboot-v2/src/northbridge/amd/amdk8/amdk8_acpi.h	2009-02-01 11:28:13.236202005 +0100
> @@ -0,0 +1,25 @@
> +/*
> + * This file is part of the coreboot project.
> + *
> + * Copyright (C) 2009 Rudolf Marek <[e-mail]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License v2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> + */
> +#ifndef AMDK8_ACPI_H
> +#define AMDK8_ACPI_H
> +#include <arch/acpigen.h>
> +
> +int k8acpi_write_vars(void);
> +
> +#endif

Regards,
Carl-Daniel
-- 
http://www.hailfinger.org/

[ron minnich]

In general I'd like to avoid weak symbols, what purpose is being
served by them here?

ron

[PATCH] Add the AML code generator for runtime code generation

[Stefan Reinauer]

ron minnich wrote:
> In general I'd like to avoid weak symbols, what purpose is being
> served by them here?
>
> ron

It avoids the need to add an empty function to every single mainboard
not supporting the feature yet, in order to avoid compilation breakage
of those boards.

Carl-Daniel mentioned that weak symbols make the code harder to follow.
In a classic C sense that may not wrong, but since we're modelling
components, what happens is that we have a slightly object oriented
approach and some "constructors" are being overloaded, while others stay
empty.

We do a similar thing with the device functions in many places.. "If
there's a subsystem vendor/device id function for that device, execute
it, otherwise execute the default function" is harder to follow than
just "If there's an ACPI create function for that board, execute it"

To show an example, look at this:

What we have today:

        /* Set the subsystem vendor and device id for mainboard devices */
        ops = ops_pci(dev);
        if (dev->on_mainboard && ops && ops->set_subsystem) {
                ops->set_subsystem(dev,
                        MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID,
                        MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID);
        }

Here's on the other hand, how filo uses weak symbols:
        void __attribute__((weak)) platform_reboot(void);

        if (platform_reboot)
                platform_reboot();
        else
                printf("Rebooting not supported.\n");

which looks very familiar, does it? Oh, and a fix of the else case/error
message would potentially fix 50 boards without requiring 50 small hunks
of patches.

I'm no big fan of empty dummy function for everyone to fix compilation.
Are they considered better than weak symbols as a method of controlling
code flow and readability? They're sure not some overly clever code, so
it hopefully won't become dangerous.

"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are, by
definition, not smart enough to debug it." – Brian W. Kernighan

One thing I wonder though, do we want to call weak symbols
unconditionally, as in Rudolf's code? No if() clause catches the case
that the symbol isn't there. In a test program that call would segfault
a user space program here.

Stefan


-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
      Tel.: [phone] • Fax: [phone]
Email: [e-mail]  • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866

[ron minnich]

On Mon, Feb 2, 2009 at 12:53 AM, Stefan Reinauer <[e-mail]> wrote:
> One thing I wonder though, do we want to call weak symbols
> unconditionally, as in Rudolf's code? No if() clause catches the case
> that the symbol isn't there. In a test program that call would segfault
> a user space program here.

if you're going to have a weak symbol I can't see any reason NOT to
call it unconditionally. The symbol is always defined.

If you're going to call it unconditionally then you might as well put
it in a file by itself, and either compile that file in or compile in
code that has a different definition of the function. That way, if you
know which files are used to build a mainboard, you also know exactly
which functions are used -- the ones in the files.

Weak symbols can be frustrating when you're using code management
tools like kscope or eclipse. You've to two symbols defined in the
source code base -- which do you  use? (yes, I know we can tell people
'just ignore the weak one' but the code analysis tools are not always
that smart).

I'm still not convinced we need them. We can get the same capability
(in v3 anyway) just by carefully selecting which files to build into a
project.

Weak symbols can make sense in a project (like glibc) where one can
never know what files are being compiled in as part of a program (by
definition, it's a library), but that logic does not apply to
coreboot, which is not a library.

ron

[PATCH] Add the AML code generator for runtime code generation

[Stefan Reinauer]

ron minnich wrote:
> On Mon, Feb 2, 2009 at 12:53 AM, Stefan Reinauer <[e-mail]> wrote:
> > One thing I wonder though, do we want to call weak symbols
> > unconditionally, as in Rudolf's code? No if() clause catches the case
> > that the symbol isn't there. In a test program that call would segfault
> > a user space program here.
>
> if you're going to have a weak symbol I can't see any reason NOT to
> call it unconditionally. The symbol is always defined.

A weak function is like a function pointer. The linker does not choke
when it is encountered, but it might still not have a value. So before
calling that function, you need to check if the function pointer is NULL
or your code crashes.

You are correct, if a symbol is weak, you don't have to compile
conditional as we "normally" do:
 
#ifdef CONFIG_ACPI_TABLEDUMPER
...
#endif

uses CONFIG_ACPI_TABLEDUMPER
default CONFIG_ACPI_TABLEDUMPER=1

and some minor bits in Config/Options.lb

But, frankly. This is not an _option_. We're just treating it as one
because that's a cheap way for us to get a #define on a single mainboard.

With a weak symbol, execution still has to be conditional:

if (function_to_call)
       function_to_call(params);
else
       printk_debug("No function_to_call for this mainboard");


I think the above is pretty beautiful and creates code that is a lot
better understandable than code that is marbled with preprocessor macros
to achieve the same thing.
> If you're going to call it unconditionally then you might as well put
> it in a file by itself, and either compile that file in or compile in
> code that has a different definition of the function. That way, if you
> know which files are used to build a mainboard, you also know exactly
> which functions are used -- the ones in the files.

This is not about the function itself. That lives in an extra file
already, as you suggested. And that is exactly the problem.
The question is: How does the code that calls the function in that file
find out if it (the file or the function) is there or not.
- Either it is there on all mainboards (dummy functions)
- or the _call_ is guarded by preprocessor crap
- or the function prototype is defined weak
> Weak symbols can be frustrating when you're using code management
> tools like kscope or eclipse. You've to two symbols defined in the
> source code base -- which do you  use? (yes, I know we can tell people
> 'just ignore the weak one' but the code analysis tools are not always
> that smart).

Wait. That problem is completely unrelated to weak symbols, and we will
continue to have it, with weak symbols or without. Even more if we
continue without weak symbols.

The reason for that is, we call functions the same. 10 mainboards have
write_pirq_routing_table() ... all mainboards have "main" or "amd64_main".

Of course kscope, eclipse, or doxygen choke on that. But not using weak
symbols means:
- we have more preprocessor macros
or
- we have more duplicate dummy functions

and both options are even worse for kscope, eclipse or doxygen.
> I'm still not convinced we need them. We can get the same capability
> (in v3 anyway) just by carefully selecting which files to build into a
> project.

No. The problem arises through this careful selection. If we don't solve
it in v2, we will see the problem in v3 even more.

Stefan

-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
      Tel.: [phone] • Fax: [phone]
Email: [e-mail]  • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866

[ron minnich]

On Tue, Feb 3, 2009 at 2:36 AM, Stefan Reinauer <[e-mail]> wrote:
> With a weak symbol, execution still has to be conditional:
>
> if (function_to_call)
>       function_to_call(params);
> else
>       printk_debug("No function_to_call for this mainboard");

You're right, I did not realize this:
[rminnich@192 tmp]$ cat weak2.c
extern unsigned long __attribute__((weak)) strong(void);
main(){strong();}
[rminnich@192 tmp]$ cc weak2.c
[rminnich@192 tmp]$ ./a.out
Segmentation fault
[rminnich@192 tmp]$

I've used weak symbols for a long time but only as a way to easily
allow code to override "default" functions. This is not a usage that
would have crossed my mind. :-)
> > Weak symbols can be frustrating when you're using code management
> > tools like kscope or eclipse. You've to two symbols defined in the
> > source code base -- which do you  use? (yes, I know we can tell people
> > 'just ignore the weak one' but the code analysis tools are not always
> > that smart).
>
> Wait. That problem is completely unrelated to weak symbols, and we will
> continue to have it, with weak symbols or without. Even more if we
> continue without weak symbols.
>
> The reason for that is, we call functions the same. 10 mainboards have
> write_pirq_routing_table() ... all mainboards have "main" or "amd64_main".

Ah, I can not get anyone to try make kscope eh? :-)

because if you make kscope in v3, and you look for
write_pirq_routing_table, you only see the one copy for the mainboard
you are using. I'll be doing the same for eclipse. It's a simple
trick: I use the makefile SOURCES variable to create the kscope
project file. I did this in part because I want to easily fine the
version of a function for the mainboard I am working on.

But now that I understand your point about weak symbols I guess we're
ok. I still would like, long term, to put this kind of thing into the
device operations as a phase 7.

Anyway, thanks for clearing me up on this question.

ron

[ron minnich]

Another question.

Were we to follow the device object model, isn't it more proper to add
a new device_operations struct member to devices to generate ACPI?
Then we add another pass to the device code which walks the tree and
each device can optionally create ACPI as it needs to. The first
object is the mainboard, and this could do all the initial setup for
the AML code generation.

If we had this I think the weak symbol would not be needed. This would
drop very nicely in to v3: I would add a phase7_acpi struct member to
device_operations.

ron

[PATCH] Add the AML code generator for runtime code generation

[Carl-Daniel Hailfinger]

On 02.02.2009 18:47, ron minnich wrote:
> Another question.
>
> Were we to follow the device object model, isn't it more proper to add
> a new device_operations struct member to devices to generate ACPI?
> Then we add another pass to the device code which walks the tree and
> each device can optionally create ACPI as it needs to. The first
> object is the mainboard, and this could do all the initial setup for
> the AML code generation.
>
> If we had this I think the weak symbol would not be needed. This would
> drop very nicely in to v3: I would add a phase7_acpi struct member to
> device_operations.

That idea is really awesome.

However, it is rather inconvenient to generate all the ACPI code at
runtime. Of course, we need ACPI code generation for exchangeable
devices like the CPU. But tables which are static anyway should not need
code generation. What do you think about a solution which simply
references the externally compiled AML for static table parts? That way,
we have one common interface and only the minimum needed complexity.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

[ron minnich]

On Mon, Feb 2, 2009 at 10:11 AM, Carl-Daniel Hailfinger
<[e-mail]> wrote:
> However, it is rather inconvenient to generate all the ACPI code at
> runtime. Of course, we need ACPI code generation for exchangeable
> devices like the CPU. But tables which are static anyway should not need
> code generation. What do you think about a solution which simply
> references the externally compiled AML for static table parts? That way,
> we have one common interface and only the minimum needed complexity.

So what we need in the general case are static tables that can be
extended at runtime as we gather more info. The static tables are
'attached'
ot the mainboard during (in v3 terms) phase 7 so, yes, I like this a lot.

I think it ought to be possible to use our object model to remove the
weak symbol and add this kind of functionality.

that said, Rudolf's code doesn't rule this idea out since his code is
v2; I certainly don't want to block progress, so let's leave it as a
suggestion for v3 and, if you want, we can try to do this in v2.

ron

[PATCH] Add the AML code generator for runtime code generation

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> However, it is rather inconvenient to generate all the ACPI code at
> runtime. 
> We simply can't do it. I am tempted to add a "yet".
> Of course, we need ACPI code generation for exchangeable
> devices like the CPU. But tables which are static anyway should not need
> code generation. What do you think about a solution which simply
> references the externally compiled AML for static table parts? 
> That's what the SSDT is for. It's loaded in addition to the DSDT to
> enhance it.

Stefan

-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
      Tel.: [phone] • Fax: [phone]
Email: [e-mail]  • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866

[PATCH] Add the AML code generator for runtime code generation

[Stefan Reinauer]

ron minnich wrote:
> Another question.
>
> Were we to follow the device object model, isn't it more proper to add
> a new device_operations struct member to devices to generate ACPI?
> Then we add another pass to the device code which walks the tree and
> each device can optionally create ACPI as it needs to. The first
> object is the mainboard, and this could do all the initial setup for
> the AML code generation.

The idea is definitely sound. But we're many steps before that, still.
With our device model, and with our ACPI support.

There are large portions of a DSDT that are "not just the device tree",
but a lot more. We can start feeding that stuff into our device tree.
That's a thing I was yelling about already, too. But then we have a
complicated device tree and a complicated generator. Seeing the
complexity of ACPI in all its shades, I am slightly, temporarily scared
> If we had this I think the weak symbol would not be needed.

Absolutely. But it means creating a new framework that is much more
enhanced than what we have today.
> This would
> drop very nicely in to v3: I would add a phase7_acpi struct member to
> device_operations.

It does sound a bit like v4. This is why it is so good.


-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
      Tel.: [phone] • Fax: [phone]
Email: [e-mail]  • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866

[PATCH] Add the AML code generator for runtime code generation

[Rudolf Marek]

ron minnich wrote:
> In general I'd like to avoid weak symbols, what purpose is being
> served by them here?

To support boards which dont need this callback. And avoids the #ifdefs 
or dummy functions.

Rudolf

[PATCH] Add the AML code generator for runtime code generation

[Rudolf Marek]

Hi,
> that's an impressive piece of code!

Heh nice.
> The following review is rather short and to-the-point. No offense intended.

Well yes as always ;)
> A few comments about the general design:
> The global variable gencurrent is a bit irritating. How do you handle
> running on multiple cores?

The idea is that the create_acpi_tables always run on BSP. FOr the 
powernow I admin we will need to run get/set MSR on each CPU.
I have in plan to move the powernow from the AMD board to more general 
place. For multi cores, they have same MSR so we dont need to solve that.
> Same for len_stack. Besides that, both variables are in the global
> namespace and could use some acpi_ prefix or suffix.

Well I was quite hesitating if to use a global variable or introduce 
some pointer parameter to every function. Then I choose to have the 
global (and static) var
for this.  The reason is that the pointer propagation would be needed 
through all functions. Also I was not considering the multi thread 
support, because ACPI
tables are always build by BSP. All pointers and stack is encapsulated, 
therefore not visible from outside. It should work fine.
> I think weak symbols are something that makes code harder to follow.

Yes, and I have read all mails until now. I decided to put here empty 
dummy routine with weak symbol. This routine will be hidden if someone 
creates
"strong" version of same function in some other object file. The reason 
for this is to avoid having #ifdefs and dummy empty functions for all boards
which don't need this.

The second possibility is to have just exactly one weak function and 
test its presence with if (name_fn) { name_fn)() } The linux kernel uses 
first case
and I decided that this nicer, because if someone accidentally manages 
to create another weak function, it depends on luck which one will be 
called.
The used scheme above assures that either the weak fn will be called or 
the strong one.
> The { belongs on another line.

Hmm I already commit that, maybe I can fix it as trivial commit.
> > +	return current;
> > +}
> > +
> > +void acpi_create_ssdt_generator(acpi_header_t *ssdt, char *oem_table_id)
>
> Strange function name.

I have in plan to introduce acpi_create_ssdt which will link up the 
ssdt.aml code to this. This is the reason for this strange name.
> > +{
> > +	unsigned long current=(unsigned long)ssdt+sizeof(acpi_header_t);
> > +	memset((void *)ssdt, 0, sizeof(acpi_header_t));
> > +	memcpy(&ssdt->signature, SSDT_NAME, 4);
>
> Maybe strncpy instead?

Here it is still OK. because it is static. I think it would be better 
for oem_table_name.
> "how many nesting levels do we support"

Hmm true maybe trival fix too.
> > +#define ACPIGEN_LENSTACK_SIZE 10
> > +
> > +/* if you need to change this, change the acpigen_write_f and
> > +   acpigen_patch_len */
> > +
>
> Can you remove the empty line above?

I like them, you dont?
> > +#define ACPIGEN_MAXLEN 0xfff
> > +
> > +#include <string.h>
> > +#include <arch/acpigen.h>
> > +#include <console/console.h>
> > +
> > +static char *gencurrent;
> > +
> > +char *len_stack[ACPIGEN_LENSTACK_SIZE];
> > +int ltop = 0;
>
> These global variables need better names and some comments explaining
> what they do.

Whoops they should be static. Sorry about that. For this reason they 
have just names like this, because they should be static and not visible.
I will fix them.
> > +
> > +static int acpigen_write_len_f()
>
> Needs a doxygen comment.

Yes in Alps I did not have doxygen installed. I will try to write some 
patch addressing the issues and also document this a bit.
Also some words to ACPI wiki would be good...
> > +{
> > +	ASSERT(ltop < (ACPIGEN_LENSTACK_SIZE - 1))
> > +	len_stack[ltop++] = gencurrent;
> > +	acpigen_emit_byte(0);
> > +	acpigen_emit_byte(0);
> > +	return 2;
> > +}
> > +
> > +void acpigen_patch_len(int len)
>
> Needs a doxygen comment.
> > +{
> > +	ASSERT(len <= ACPIGEN_MAXLEN)
> > +	ASSERT(ltop > 0)
> > +	char *p = len_stack[--ltop];
> > +	/* generate store length for 0xfff max */
> > +	p[0] = (0x40 | (len & 0xf));
> > +	p[1] = (len >> 4 & 0xff);
> > +
> > +}
> > +
> > +void acpigen_set_current(char *curr) {
>
> Needs a doxygen comment.
> > +    gencurrent = curr;
> > +}
> > +
> > +char *acpigen_get_current(void) {
>
> Needs a doxygen comment.

Yes its true.
> > unsigned long acpi_fill_srat(unsigned long current); 
> > +unsigned long acpi_fill_ssdt_generator(unsigned long current, char *oem_table_id);
> > +void acpi_create_ssdt_generator(acpi_header_t *ssdt, char *oem_table_id);
>
> The two functions above have rather similar names and it's not
> immedately ofvious how they differ.

It follows the scheme we have right now. Create creates, fill is callback.
> acpigen_get_current is a really confusingly named function.

Hmm any idea of better name? it gets "current" which is used in all acpi 
code as current data pointer.
> "COREBOOT" please. That way, people can actually find out where the
> table came from. The fact that the data are generated dynamically is
> already visible from the compiler ID.

Here is small catch. The ACPI specs says that the OEM_TABLE_ID should 
have unique name, please note that the OEM_ID is set to coreboot.
> > +	for(i=sysconf.hc_possible_num; i<HC_POSSIBLE_NUM; i++) { // in case we set array size to other than 8
>
> Comment on a separate line, please.

Aha this is cut and paste from old code.
> > +		lenp += acpigen_write_dword(0x0);
> > +	}
> > +
> > +	acpigen_patch_len(lenp - 1);
>
> acpigen_patch_len is a name which does not really reveal function purpose.

Give me better one please, what it does its a patching of size when the 
size of the object is known.
> > +int k8acpi_write_vars(void)
> > +{
> > +	int lens;
>
> Maybe use len instead of lens?

Lens because its a Scope. lenp is for Package.
> I know the original code did not have it, but can you please add TOM2?

Yes I was also thinking about this one. What is cool now, that we can 
easily add new stuff without worring to break the binary
offset dependent patching ;)

I will try to address most of the issues also depending on your answers,

Thanks,
Rudolf

[PATCH] Add the AML code generator for runtime code generation

[Carl-Daniel Hailfinger]

On 02.02.2009 19:02, Rudolf Marek wrote:
> Hi,
> > that's an impressive piece of code!
>
> Heh nice.
> > The following review is rather short and to-the-point. No offense
> > intended.
>
> Well yes as always ;)
> > A few comments about the general design:
> > The global variable gencurrent is a bit irritating. How do you handle
> > running on multiple cores?
>
> The idea is that the create_acpi_tables always run on BSP. FOr the
> powernow I admin we will need to run get/set MSR on each CPU.
> I have in plan to move the powernow from the AMD board to more general
> place. For multi cores, they have same MSR so we dont need to solve that.

OK.
> > Same for len_stack. Besides that, both variables are in the global
> > namespace and could use some acpi_ prefix or suffix.
>
> Well I was quite hesitating if to use a global variable or introduce
> some pointer parameter to every function. Then I choose to have the
> global (and static) var
> for this.  The reason is that the pointer propagation would be needed
> through all functions. Also I was not considering the multi thread
> support, because ACPI
> tables are always build by BSP. All pointers and stack is
> encapsulated, therefore not visible from outside. It should work fine.

Maybe add a comment that all these functions should not run on APs?
> > I think weak symbols are something that makes code harder to follow.
>
> Yes, and I have read all mails until now. I decided to put here empty
> dummy routine with weak symbol. This routine will be hidden if someone
> creates
> "strong" version of same function in some other object file. The
> reason for this is to avoid having #ifdefs and dummy empty functions
> for all boards
> which don't need this.
>
> The second possibility is to have just exactly one weak function and
> test its presence with if (name_fn) { name_fn)() } The linux kernel
> uses first case
> and I decided that this nicer, because if someone accidentally manages
> to create another weak function, it depends on luck which one will be
> called.
> The used scheme above assures that either the weak fn will be called
> or the strong one.

Hm. What about having boards #define HAVE_ACPI_SOME_FUNCTION
> > The { belongs on another line.
>
> Hmm I already commit that, maybe I can fix it as trivial commit.

Yes please.
> > > +    return current;
> > > +}
> > > +
> > > +void acpi_create_ssdt_generator(acpi_header_t *ssdt, char
> > > *oem_table_id)
> >
> > Strange function name.
>
> I have in plan to introduce acpi_create_ssdt which will link up the
> ssdt.aml code to this. This is the reason for this strange name.

OK.
> > > +{
> > > +    unsigned long current=(unsigned long)ssdt+sizeof(acpi_header_t);
> > > +    memset((void *)ssdt, 0, sizeof(acpi_header_t));
> > > +    memcpy(&ssdt->signature, SSDT_NAME, 4);
> >
> > Maybe strncpy instead?
>
> Here it is still OK. because it is static. I think it would be better
> for oem_table_name.
> > "how many nesting levels do we support"
>
> Hmm true maybe trival fix too.

Yes.
> > > +#define ACPIGEN_LENSTACK_SIZE 10
> > > +
> > > +/* if you need to change this, change the acpigen_write_f and
> > > +   acpigen_patch_len */
> > > +
> >
> > Can you remove the empty line above?
>
> I like them, you dont?

It is not clear if the comment belongs to the #define if there is an
empty line in between.
> > > +#define ACPIGEN_MAXLEN 0xfff
> > > +
> > > +#include <string.h>
> > > +#include <arch/acpigen.h>
> > > +#include <console/console.h>
> > > +
> > > +static char *gencurrent;
> > > +
> > > +char *len_stack[ACPIGEN_LENSTACK_SIZE];
> > > +int ltop = 0;
> >
> > These global variables need better names and some comments explaining
> > what they do.
>
> Whoops they should be static. Sorry about that. For this reason they
> have just names like this, because they should be static and not visible.
> I will fix them.

Grepping (and cscope) is easier if they have that prefix. It also avoids
collisions if we ever change to unit-at-a-time compilation (like in v3).
> > > +
> > > +static int acpigen_write_len_f()
> >
> > Needs a doxygen comment.
>
> Yes in Alps I did not have doxygen installed. I will try to write some
> patch addressing the issues and also document this a bit.
> Also some words to ACPI wiki would be good...

Yes please. You know a lot about ACPI.
> > > +{
> > > +    ASSERT(ltop < (ACPIGEN_LENSTACK_SIZE - 1))
> > > +    len_stack[ltop++] = gencurrent;
> > > +    acpigen_emit_byte(0);
> > > +    acpigen_emit_byte(0);
> > > +    return 2;
> > > +}
> > > +
> > > +void acpigen_patch_len(int len)
> >
> > Needs a doxygen comment.
> > > +{
> > > +    ASSERT(len <= ACPIGEN_MAXLEN)
> > > +    ASSERT(ltop > 0)
> > > +    char *p = len_stack[--ltop];
> > > +    /* generate store length for 0xfff max */
> > > +    p[0] = (0x40 | (len & 0xf));
> > > +    p[1] = (len >> 4 & 0xff);
> > > +
> > > +}
> > > +
> > > +void acpigen_set_current(char *curr) {
> >
> > Needs a doxygen comment.
> > > +    gencurrent = curr;
> > > +}
> > > +
> > > +char *acpigen_get_current(void) {
> >
> > Needs a doxygen comment.
>
> Yes its true.
> > > unsigned long acpi_fill_srat(unsigned long current); +unsigned long
> > > acpi_fill_ssdt_generator(unsigned long current, char *oem_table_id);
> > > +void acpi_create_ssdt_generator(acpi_header_t *ssdt, char
> > > *oem_table_id);
> >
> > The two functions above have rather similar names and it's not
> > immedately ofvious how they differ.
>
> It follows the scheme we have right now. Create creates, fill is
> callback.

I didn't know that. OK.
> > acpigen_get_current is a really confusingly named function.
>
> Hmm any idea of better name? it gets "current" which is used in all
> acpi code as current data pointer.

Is it really the current data pointer? The code seems to treat it as
end-of-data pointer, not current data pointer.
> > "COREBOOT" please. That way, people can actually find out where the
> > table came from. The fact that the data are generated dynamically is
> > already visible from the compiler ID.
>
> Here is small catch. The ACPI specs says that the OEM_TABLE_ID should
> have unique name, please note that the OEM_ID is set to coreboot.

Hm yes. Which OEM table ID is used by factory BIOS?
> > > +    for(i=sysconf.hc_possible_num; i<HC_POSSIBLE_NUM; i++) { // in
> > > case we set array size to other than 8
> >
> > Comment on a separate line, please.
>
> Aha this is cut and paste from old code.

-> maybe fix it in the trivial patch with the other whitespace etc?
> > > +        lenp += acpigen_write_dword(0x0);
> > > +    }
> > > +
> > > +    acpigen_patch_len(lenp - 1);
> >
> > acpigen_patch_len is a name which does not really reveal function
> > purpose.
>
> Give me better one please, what it does its a patching of size when
> the size of the object is known.

Ah. I thought is is the length of the patch. Maybe acpigen_modify_len or
acpigen_adjust_len (preferred)?
> > > +int k8acpi_write_vars(void)
> > > +{
> > > +    int lens;
> >
> > Maybe use len instead of lens?
>
> Lens because its a Scope. lenp is for Package.

Ah. I thought it was the plural of len. Maybe use scopelen and packlen?
> > I know the original code did not have it, but can you please add TOM2?
>
> Yes I was also thinking about this one. What is cool now, that we can
> easily add new stuff without worring to break the binary
> offset dependent patching ;)

Yes absolutely. That's what I thought.
> I will try to address most of the issues also depending on your answers,

Thanks!

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/