[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