-----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 r.marek@assembler.cz
Rudolf
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 r.marek@assembler.cz
Acked-by: Peter Stuge peter@stuge.se
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Commited as Committed revision 3925.
Thanks, Rudolf
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 r.marek@assembler.cz
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 r.marek@assembler.cz
- 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 r.marek@assembler.cz
- 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 r.marek@assembler.cz
- 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
In general I'd like to avoid weak symbols, what purpose is being served by them here?
ron
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
On Mon, Feb 2, 2009 at 12:53 AM, Stefan Reinauer stepan@coresystems.de 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
ron minnich wrote:
On Mon, Feb 2, 2009 at 12:53 AM, Stefan Reinauer stepan@coresystems.de 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
On Tue, Feb 3, 2009 at 2:36 AM, Stefan Reinauer stepan@coresystems.de 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
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
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
On Mon, Feb 2, 2009 at 10:11 AM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net 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
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
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.
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
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
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
-
Carl-Daniel Hailfinger -
Peter Stuge -
ron minnich -
Rudolf Marek -
Stefan Reinauer