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

Rudolf Marek r.marek at assembler.cz
Mon Feb 2 19:02:58 CET 2009


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




More information about the coreboot mailing list