Re: [PATCHv3 4/4] acpi: automatically generated ssdt proc

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Oct 04, 2011 at 10:52:33PM -0400, Kevin O'Connor wrote:
> On Tue, Oct 04, 2011 at 03:26:19PM +0200, Michael S. Tsirkin wrote:
> > Get rid of manually cut and pasted ssdt_proc,
> > use ssdt compiled by iasl and offsets extracted
> > by acpi_extract instead.
> 
> Thanks - I like the idea of auto-generating the offsets.
> 
> [...]
> > +#define AmlCode static ssdp_proc_aml
> > +#include "ssdt-proc.hex"
> > +#undef AmlCode
> 
> Side note - since you're post-processing the acpi data, it would be
> nice to update the name in the hex file too.

That would mean we will output hex as well, ignore the
hex produced by iasl. Sure, I can do that.
Another benefit would be that we can skip the ssdt
preamble generated by iasl in the hex we output.

> > +/* 0x5B 0x83 ProcessorOp PkgLength NameString ProcID */
> > +#define SD_OFFSET_CPUHEX (*ssdt_proc_name - *ssdt_proc_start + 2)
> > +#define SD_OFFSET_CPUID1 (*ssdt_proc_name - *ssdt_proc_start + 4)
> > +#define SD_OFFSET_CPUID2 (*ssdt_proc_id - *ssdt_proc_start)
> > +#define SD_SIZEOF (*ssdt_proc_end - *ssdt_proc_start)
> > +#define SD_PROC (ssdp_proc_aml + *ssdt_proc_start)
> [...]
> >  DefinitionBlock ("ssdt-proc.aml", "SSDT", 0x01, "BXPC", "BXSSDT", 0x1)
> > -/*  v------------------ DO NOT EDIT ------------------v */
> >  {
> > +    ACPI_EXTRACT_PROCESSOR_START ssdt_proc_start
> > +    ACPI_EXTRACT_PROCESSOR_END ssdt_proc_end
> > +    ACPI_EXTRACT_PROCESSOR_STRING ssdt_proc_name
> >      Processor (CPAA, 0xAA, 0x0000b010, 0x06) {
> 
> Since the acpi.c code needs to know the processor object format
> anyway, what about making a generic "ACPI_EXTRACT" indicator that
> exports the location, size, and parameter location in one go.
> Something like:
> 
>     ACPI_EXTRACT ssdt_proc_obj
>     Processor (CPAA, 0xAA, 0x0000b010, 0x06) {
> 

I considered this, sure. We could parse AML to figure out
what is the object type we are trying to look up.

However I decided sanity-checking that we get the right type of object
in AML is better. This way if iasl output format breaks
we will have a better chance to detect that.
Makes sense?

Also this can not be as generic as it seems: each type of
object in AML bytecode is encoded slightly differently.
So we would still have types of objects we support
and types of object we don't.


> which would produce something like:
> 
> static struct aml_object ssdt_proc_obj = {.addr=0x24, .size=0x40, .param=0x28};

What is the param offset here?

I really think multiple arrays are better so we don't waste memory
on fields that we don't need and alignment.
I also dislike hardcoding field names. With a struct,
if you want to know where did 'param' come from you must
read python. My way, all names come from DSL source.


> As for the other parts of this patch series - I'm still leary of
> changing the DSDT dynamically.

Hmm, not sure I understand why. Could you explain pls?

>  I'd be curious to see if we can add
> the following to ssdt-proc.dsl:
> 
>     ACPI_EXTRACT hotplug_obj
>     Device (SL00) {
>         ACPI_EXTRACT_NAME_DWORD_CONST hotplog_id
>         Name (ID, 0xAABBCCDD)
>         Name (_ADR, ID)
>         Method (_EJ0, 1) { Return(PCEJ(ID)) }
>         Name (_SUN, ID)
>     }
> 
> and then just memcpy the "hotplug_obj" N number of times into the ssdt
> for each available slot.  (This would be on top of the DSDT
> simplification patch series that I posted previously.)

This assumes all devices are the same. But unfortunately this will not
work for other devices such as VGA.

> 
> -Kevin