Re: [coreboot] [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

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
> 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
> 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
> 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