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

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Tue Feb 3 00:26:03 CET 2009


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/





More information about the coreboot mailing list