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

2014-02-16 Thread Michael S. Tsirkin
On Fri, Feb 07, 2014 at 01:51:33PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov 
> ---
>  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 ScopeOp0x10
> +#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_a

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

2014-02-07 Thread Igor Mammedov
Signed-off-by: Igor Mammedov 
---
 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 ScopeOp0x10
+#define BufferOp   0x11
 #define DeviceOp   0x82
 
+#define EndTag 0x79
+#define Decode16   0x1
+#define Decode10   0x0
+
 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 */
+build_prepend_int(package, len);
+build_prepend_package_length(package, 0);
+build_prepend_byte(package, BufferOp);
+}
+
 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, ...) { \
+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); \
+}
+
+#define ACPI_RESOURCE_TEMPLATE(ctx, name, ...) { \
+ACPI_BUFFER(ctx, name, 0, \
+__VA_ARGS__; \
+ACPI_ENDTAG(name); \
+) \
+}
+
+#define ACPI_IO(ctx, _DEC, _MIN_BASE, _MAX_BASE, _ALN, _LEN) { \
+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 */
+);
 );
 );
 
-- 
1.7.1