Re: [Qemu-devel] [RFC 6/9] acpi: consume GPE0 IO resources in PNP0C02 device

[Michael S. Tsirkin]

On Fri, Feb 07, 2014 at 01:51:33PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
>  hw/i386/acpi-build.c |   62 
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 62 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index f0bedbd..ce5f715 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -230,8 +230,13 @@ static void acpi_get_pci_info(PcPciInfo *info)
>  
>  #define NameOp             0x08
>  #define ScopeOp            0x10
> +#define BufferOp           0x11
>  #define DeviceOp           0x82
>  
> +#define EndTag             0x79

I would say we should use the values from
Table 6-162 Small Resource Items.
Wrap them in a function to get the full resource.


> +#define Decode16           0x1
> +#define Decode10           0x0
> +

This is the name from ASL, it's really _DEC value.


>  static void
>  build_header(GArray *linker, GArray *table_data,
>               AcpiTableHeader *h, uint32_t sig, int len, uint8_t rev)
> @@ -406,6 +411,25 @@ static void build_append_int(GArray *table, uint32_t 
> value)
>      }
>  }
>  
> +static void build_prepend_int(GArray *array, uint32_t value)
> +{
> +    GArray *data = build_alloc_array();
> +
> +    build_append_int(data, value);
> +    g_array_prepend_vals(array, data->data, data->len);
> +    build_free_array(data);
> +}
> +
> +static void build_buffer(GArray *package, unsigned BufferSize)
> +{
> +    uint32_t len = package->len > BufferSize ? package->len : BufferSize;
> +
> +    /* TODO: buffer padding if BufferSize > actual buffer length */

Not sure what this means.
So assert here?
Or just make it work ...

> +    build_prepend_int(package, len);
> +    build_prepend_package_length(package, 0);
> +    build_prepend_byte(package, BufferOp);

prepend is confusing.
Just do it like we do for methods:
        build_append_and_cleanup_buffer(template, buffer);



> +}
> +
>  static GArray *build_alloc_method(const char *name, uint8_t arg_count)
>  {
>      GArray *method = build_alloc_array();
> @@ -523,6 +547,14 @@ static uint32_t encodeEisaId(const char *str)
>      build_free_array(name); \
>  }
>  
> +#define ACPI_BUFFER(ctx, name, min_size, ...) { \

Why pass in min_size?
the only reason we have it in existing code
was I wanted ACPI to be bit for bit compatible
with what seabios generated.

We can drop minsize everywhere ...


> +    GArray *name = build_alloc_array(); \
> +    __VA_ARGS__; \
> +    build_buffer(name, min_size); \
> +    build_append_array(ctx, name); \
> +    build_free_array(name); \
> +}
> +
>  #define ACPI_NAME(ctx, name) { \
>      build_append_byte(ctx, NameOp); \
>      build_append_nameseg(ctx, name); \
> @@ -541,6 +573,29 @@ static uint32_t encodeEisaId(const char *str)
>      build_free_array(name); \
>  }
>  
> +#define ACPI_ENDTAG(ctx) { \
> +    build_append_byte(ctx, EndTag); \
> +    build_append_byte(ctx, 0); \

Confused.
what's going on with the checksum here?
What fills it in?
why don't we add in the correct byte straight away?

> +}
> +
> +#define ACPI_RESOURCE_TEMPLATE(ctx, name, ...) { \
> +    ACPI_BUFFER(ctx, name, 0, \
> +        __VA_ARGS__; \
> +        ACPI_ENDTAG(name); \

Ugh.
Not worth the ugliness in my opinion.
Just add end tag explicitly.


> +    ) \
> +}
> +
> +#define ACPI_IO(ctx, _DEC, _MIN_BASE, _MAX_BASE, _ALN, _LEN) { \

C spec says
— All identifiers that begin with an underscore and either an uppercase
letter or another
underscore are always reserved for any use.
— All identifiers that begin with an underscore are always reserved for
use as identifiers
so we try to avoid these.


> +    build_append_byte(ctx, 0x47 /* IO port descriptor */); \
> +    build_append_byte(ctx, _DEC); \
> +    build_append_byte(ctx, _MIN_BASE  & 0xff); \
> +    build_append_byte(ctx, (_MIN_BASE >> 8) & 0xff); \
> +    build_append_byte(ctx, _MAX_BASE  & 0xff); \
> +    build_append_byte(ctx, (_MAX_BASE >> 8) & 0xff); \
> +    build_append_byte(ctx, _ALN); \
> +    build_append_byte(ctx, _LEN); \
> +}
> +
>  /* FACS */
>  static void
>  build_facs(GArray *table_data, GArray *linker, PcGuestInfo *guest_info)
> @@ -1084,6 +1139,13 @@ build_ssdt(GArray *table_data, GArray *linker,
>          ACPI_SCOPE(sb_scope, PCI0,
>              ACPI_DEVICE(PCI0, MRES,
>                  ACPI_NAME(MRES, "_HID"); ACPI_EISAID(MRES, "PNP0C02");
> +                ACPI_NAME(MRES, "_CRS"); ACPI_RESOURCE_TEMPLATE(MRES, RESBUF,
> +                    ACPI_IO(RESBUF, Decode16,

> +                            pm->gpe0_blk,      /* _MIN */
> +                            pm->gpe0_blk,      /* _MAX */
> +                            0x0,               /* _ALN */
> +                            pm->gpe0_blk_len); /* _LEN */
> +                );
>              );
>          );

Ugh, that's too tricky I'm afraid.

how about:

        crs = build_alloc_array();
        buf = build_alloc_buffer();
        build_append_io(buf, .... );
        build_append_and_cleanup_buffer(crs, buf);

        
make everything use static functions, not macros.

>  
> -- 
> 1.7.1