[Qemu-devel] [RFC 0/9] generate dynamic _CRS for motherboard resources

[Qemu-devel] [RFC 0/9] generate dynamic _CRS for motherboard resources

[Igor Mammedov]

Since introduction of PCIHP, it became problematic to
punch hole in PCI0._CRS statically since PCI hotplug
region size became runtime changeable.

So replace static hole punching with dynamically consumed
resources in a child device on PCI0 bus. i.e generate
PNP0C02 device as a child of PCI0 bus at runtime and
consume GPE0, PCI/CPU hotplug IO resources in it instead
of punching holes in static PCI0._CRS.

Tested with Windows XPsp3, Vista, Windows Server 2003, 2008, 2012r2.

PS:
Series adds several ASL like macros to simplify
code for dynamic generation of AML structures.

Igor Mammedov (9):
  Revert "pc: Q35 DSDT: exclude CPU hotplug IO range from PCI bus
    resources"
  Revert "pc: PIIX DSDT: exclude CPU/PCI hotplug & GPE0 IO range from
    PCI bus resources"
  Partial revert "pc: ACPI: expose PRST IO range via _CRS"
  acpi: replace opencoded opcodes with defines
  acpi: add PNP0C02 to PCI0 bus
  acpi: consume GPE0 IO resources in PNP0C02 device
  acpi: consume CPU hotplug IO resource in PNP0C02 device
  pcihp: expose PCI hotplug MMIO base/length as properties of piix4pm
  acpi: consume PCIHP IO resource in PNP0C02 device

 hw/acpi/pcihp.c                   |   28 ++++++
 hw/acpi/piix4.c                   |    1 +
 hw/i386/acpi-build.c              |  177 ++++++++++++++++++++++++++++++++++--
 hw/i386/acpi-dsdt-cpu-hotplug.dsl |   11 ---
 hw/i386/acpi-dsdt-pci-crs.dsl     |   15 +++-
 hw/i386/acpi-dsdt.dsl             |   39 --------
 hw/i386/q35-acpi-dsdt.dsl         |   16 ----
 include/hw/acpi/pcihp.h           |    4 +
 8 files changed, 214 insertions(+), 77 deletions(-)

[Qemu-devel] [RFC 1/9] Revert "pc: Q35 DSDT: exclude CPU hotplug IO range from PCI bus resources"

[Igor Mammedov]

This reverts commit 562e56a9f8e627b2a4ef60037507361ce3cb4e6d.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-dsdt-pci-crs.dsl |    7 +++++++
 hw/i386/acpi-dsdt.dsl         |    7 -------
 hw/i386/q35-acpi-dsdt.dsl     |    8 --------
 3 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/hw/i386/acpi-dsdt-pci-crs.dsl b/hw/i386/acpi-dsdt-pci-crs.dsl
index 4648e90..8b631d1 100644
--- a/hw/i386/acpi-dsdt-pci-crs.dsl
+++ b/hw/i386/acpi-dsdt-pci-crs.dsl
@@ -30,6 +30,13 @@ Scope(\_SB.PCI0) {
             0x01,               // Address Alignment
             0x08,               // Address Length
             )
+        WordIO(ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,
+            0x0000,             // Address Space Granularity
+            0x0000,             // Address Range Minimum
+            0x0CF7,             // Address Range Maximum
+            0x0000,             // Address Translation Offset
+            0x0CF8,             // Address Length
+            ,, , TypeStatic)
         BOARD_SPECIFIC_PCI_RESOURSES
         DWordMemory(ResourceProducer, PosDecode, MinFixed, MaxFixed, Cacheable, ReadWrite,
             0x00000000,         // Address Space Granularity
diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
index b23d5e0..5546b4f 100644
--- a/hw/i386/acpi-dsdt.dsl
+++ b/hw/i386/acpi-dsdt.dsl
@@ -38,13 +38,6 @@ DefinitionBlock (
 #define BOARD_SPECIFIC_PCI_RESOURSES \
      WordIO(ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange, \
          0x0000, \
-         0x0000, \
-         0x0CF7, \
-         0x0000, \
-         0x0CF8, \
-         ,, , TypeStatic) \
-     WordIO(ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange, \
-         0x0000, \
          0x0D00, \
          0xADFF, \
          0x0000, \
diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
index d618e9e..cf6c03d 100644
--- a/hw/i386/q35-acpi-dsdt.dsl
+++ b/hw/i386/q35-acpi-dsdt.dsl
@@ -51,14 +51,6 @@ DefinitionBlock (
 #define BOARD_SPECIFIC_PCI_RESOURSES \
      WordIO(ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange, \
          0x0000, \
-         0x0000, \
-         0x0CD7, \
-         0x0000, \
-         0x0CD8, \
-         ,, , TypeStatic) \
-     /* 0xcd8-0xcf7 hole for CPU hotplug, hw/acpi/ich9.c:ICH9_PROC_BASE */ \
-     WordIO(ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange, \
-         0x0000, \
          0x0D00, \
          0xFFFF, \
          0x0000, \
-- 
1.7.1

[Qemu-devel] [RFC 2/9] Revert "pc: PIIX DSDT: exclude CPU/PCI hotplug & GPE0 IO range from PCI bus resources"

[Igor Mammedov]

This reverts commit 1aa149b479a479323121251f1e8e676765cb354d.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-dsdt-pci-crs.dsl |    8 +++++++-
 hw/i386/acpi-dsdt.dsl         |   32 --------------------------------
 hw/i386/q35-acpi-dsdt.dsl     |    8 --------
 3 files changed, 7 insertions(+), 41 deletions(-)

diff --git a/hw/i386/acpi-dsdt-pci-crs.dsl b/hw/i386/acpi-dsdt-pci-crs.dsl
index 8b631d1..b375a19 100644
--- a/hw/i386/acpi-dsdt-pci-crs.dsl
+++ b/hw/i386/acpi-dsdt-pci-crs.dsl
@@ -37,7 +37,13 @@ Scope(\_SB.PCI0) {
             0x0000,             // Address Translation Offset
             0x0CF8,             // Address Length
             ,, , TypeStatic)
-        BOARD_SPECIFIC_PCI_RESOURSES
+        WordIO(ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,
+            0x0000,             // Address Space Granularity
+            0x0D00,             // Address Range Minimum
+            0xFFFF,             // Address Range Maximum
+            0x0000,             // Address Translation Offset
+            0xF300,             // Address Length
+            ,, , TypeStatic)
         DWordMemory(ResourceProducer, PosDecode, MinFixed, MaxFixed, Cacheable, ReadWrite,
             0x00000000,         // Address Space Granularity
             0x000A0000,         // Address Range Minimum
diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
index 5546b4f..28ce1c6 100644
--- a/hw/i386/acpi-dsdt.dsl
+++ b/hw/i386/acpi-dsdt.dsl
@@ -35,38 +35,6 @@ DefinitionBlock (
 /****************************************************************
  * PCI Bus definition
  ****************************************************************/
-#define BOARD_SPECIFIC_PCI_RESOURSES \
-     WordIO(ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange, \
-         0x0000, \
-         0x0D00, \
-         0xADFF, \
-         0x0000, \
-         0xA100, \
-         ,, , TypeStatic) \
-     /* 0xae00-0xae0e hole for PCI hotplug, hw/acpi/piix4.c:PCI_HOTPLUG_ADDR */ \
-     WordIO(ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange, \
-         0x0000, \
-         0xAE0F, \
-         0xAEFF, \
-         0x0000, \
-         0x00F1, \
-         ,, , TypeStatic) \
-     /* 0xaf00-0xaf1f hole for CPU hotplug, hw/acpi/piix4.c:PIIX4_PROC_BASE */ \
-     WordIO(ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange, \
-         0x0000, \
-         0xAF20, \
-         0xAFDF, \
-         0x0000, \
-         0x00C0, \
-         ,, , TypeStatic) \
-     /* 0xafe0-0xafe3 hole for ACPI.GPE0, hw/acpi/piix4.c:GPE_BASE */ \
-     WordIO(ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange, \
-         0x0000, \
-         0xAFE4, \
-         0xFFFF, \
-         0x0000, \
-         0x501C, \
-         ,, , TypeStatic)
 
     Scope(\_SB) {
         Device(PCI0) {
diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
index cf6c03d..596d752 100644
--- a/hw/i386/q35-acpi-dsdt.dsl
+++ b/hw/i386/q35-acpi-dsdt.dsl
@@ -48,14 +48,6 @@ DefinitionBlock (
 /****************************************************************
  * PCI Bus definition
  ****************************************************************/
-#define BOARD_SPECIFIC_PCI_RESOURSES \
-     WordIO(ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange, \
-         0x0000, \
-         0x0D00, \
-         0xFFFF, \
-         0x0000, \
-         0xF300, \
-         ,, , TypeStatic)
 
     Scope(\_SB) {
         Device(PCI0) {
-- 
1.7.1

[Qemu-devel] [RFC 3/9] Partial revert "pc: ACPI: expose PRST IO range via _CRS"

[Igor Mammedov]

This reverts part of commit 61a3f63560ccd2b5e8c9134e9213a1cff36f26bf
that adds Device(CPU_HOTPLUG_RESOURCE_DEVICE) with CPU hotplug IO
resource in its _CRS.

Conflicts:

	hw/i386/acpi-dsdt-cpu-hotplug.dsl

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-dsdt-cpu-hotplug.dsl |   11 -----------
 1 files changed, 0 insertions(+), 11 deletions(-)

diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
index dee4843..3acfe1b 100644
--- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl
+++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
@@ -16,7 +16,6 @@
 /****************************************************************
  * CPU hotplug
  ****************************************************************/
-#define CPU_HOTPLUG_RESOURCE_DEVICE PRES
 
 Scope(\_SB) {
     /* Objects filled in by run-time generated SSDT */
@@ -91,14 +90,4 @@ Scope(\_SB) {
             Increment(Local0)
         }
     }
-
-    Device(CPU_HOTPLUG_RESOURCE_DEVICE) {
-        Name(_HID, "ACPI0004")
-
-        Name(_CRS, ResourceTemplate() {
-            IO(Decode16, CPU_STATUS_BASE, CPU_STATUS_BASE, 0, CPU_STATUS_LEN)
-        })
-
-        Name(_STA, 0xB) /* present, functioning, decoding, not shown in UI */
-    }
 }
-- 
1.7.1

[Qemu-devel] [RFC 4/9] acpi: replace opencoded opcodes with defines

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c |   28 ++++++++++++++++++----------
 1 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 6a43a7d..1dbe5ce 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -224,6 +224,14 @@ static void acpi_get_pci_info(PcPciInfo *info)
 #define ACPI_BUILD_TABLE_FILE "etc/acpi/tables"
 #define ACPI_BUILD_RSDP_FILE "etc/acpi/rsdp"
 
+#define BytePrefix         0x0A
+#define WordPrefix         0x0B
+#define DWordPrefix        0x0C
+
+#define NameOp             0x08
+#define ScopeOp            0x10
+#define DeviceOp           0x82
+
 static void
 build_header(GArray *linker, GArray *table_data,
              AcpiTableHeader *h, uint32_t sig, int len, uint8_t rev)
@@ -364,13 +372,13 @@ static void build_append_value(GArray *table, uint32_t value, int size)
 
     switch (size) {
     case 1:
-        prefix = 0x0A; /* BytePrefix */
+        prefix = BytePrefix;
         break;
     case 2:
-        prefix = 0x0B; /* WordPrefix */
+        prefix = WordPrefix;
         break;
     case 4:
-        prefix = 0x0C; /* DWordPrefix */
+        prefix = DWordPrefix;
         break;
     default:
         assert(0);
@@ -762,24 +770,24 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
     bool bus_hotplug_support = false;
 
     if (bus->parent_dev) {
-        op = 0x82; /* DeviceOp */
+        op = DeviceOp;
         build_append_nameseg(bus_table, "S%.02X_",
                              bus->parent_dev->devfn);
-        build_append_byte(bus_table, 0x08); /* NameOp */
+        build_append_byte(bus_table, NameOp);
         build_append_nameseg(bus_table, "_SUN");
         build_append_value(bus_table, PCI_SLOT(bus->parent_dev->devfn), 1);
-        build_append_byte(bus_table, 0x08); /* NameOp */
+        build_append_byte(bus_table, NameOp);
         build_append_nameseg(bus_table, "_ADR");
         build_append_value(bus_table, (PCI_SLOT(bus->parent_dev->devfn) << 16) |
                            PCI_FUNC(bus->parent_dev->devfn), 4);
     } else {
-        op = 0x10; /* ScopeOp */;
+        op = ScopeOp;
         build_append_nameseg(bus_table, "PCI0");
     }
 
     bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL);
     if (bsel) {
-        build_append_byte(bus_table, 0x08); /* NameOp */
+        build_append_byte(bus_table, NameOp);
         build_append_nameseg(bus_table, "BSEL");
         build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
     }
@@ -962,7 +970,7 @@ build_ssdt(GArray *table_data, GArray *linker,
 
     {
         GArray *sb_scope = build_alloc_array();
-        uint8_t op = 0x10; /* ScopeOp */
+        uint8_t op = ScopeOp;
 
         build_append_nameseg(sb_scope, "_SB_");
 
@@ -983,7 +991,7 @@ build_ssdt(GArray *table_data, GArray *linker,
         build_append_notify_method(sb_scope, "NTFY", "CP%0.02X", acpi_cpus);
 
         /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */
-        build_append_byte(sb_scope, 0x08); /* NameOp */
+        build_append_byte(sb_scope, NameOp);
         build_append_nameseg(sb_scope, "CPON");
 
         {
-- 
1.7.1

Re: [Qemu-devel] [RFC 4/9] acpi: replace opencoded opcodes with defines

[Michael S. Tsirkin]

On Fri, Feb 07, 2014 at 01:51:31PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>


The reason I avoided doing this is that this
conflicts with qemu coding style which
only uses camel case for types.

So as a minimum this needs a comment
explaining that we are using the names from
ACPI spec as-is, that's why we deviate from
the coding style, to simplify matching against
that.

Something like below:

> ---
>  hw/i386/acpi-build.c |   28 ++++++++++++++++++----------
>  1 files changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 6a43a7d..1dbe5ce 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -224,6 +224,14 @@ static void acpi_get_pci_info(PcPciInfo *info)
>  #define ACPI_BUILD_TABLE_FILE "etc/acpi/tables"
>  #define ACPI_BUILD_RSDP_FILE "etc/acpi/rsdp"


/* Constants from ACPI spec 5.0a:
 *  ACPI Machine Language (AML) Specification
 */

We probably should add in spec link as well.


> +#define BytePrefix         0x0A
> +#define WordPrefix         0x0B
> +#define DWordPrefix        0x0C

Not sure about these ones.
There's a single user, and naming is different
from rest of operators which makes it
a bit confusing.
Maybe define near the user?

> +
> +#define NameOp             0x08
> +#define ScopeOp            0x10
> +#define DeviceOp           0x82

Hmm if we are doing this let's do this for all Ops.

> +
>  static void
>  build_header(GArray *linker, GArray *table_data,
>               AcpiTableHeader *h, uint32_t sig, int len, uint8_t rev)
> @@ -364,13 +372,13 @@ static void build_append_value(GArray *table, uint32_t value, int size)
>  
>      switch (size) {
>      case 1:
> -        prefix = 0x0A; /* BytePrefix */
> +        prefix = BytePrefix;
>          break;
>      case 2:
> -        prefix = 0x0B; /* WordPrefix */
> +        prefix = WordPrefix;
>          break;
>      case 4:
> -        prefix = 0x0C; /* DWordPrefix */
> +        prefix = DWordPrefix;
>          break;
>      default:
>          assert(0);
> @@ -762,24 +770,24 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
>      bool bus_hotplug_support = false;
>  
>      if (bus->parent_dev) {
> -        op = 0x82; /* DeviceOp */
> +        op = DeviceOp;
>          build_append_nameseg(bus_table, "S%.02X_",
>                               bus->parent_dev->devfn);
> -        build_append_byte(bus_table, 0x08); /* NameOp */
> +        build_append_byte(bus_table, NameOp);
>          build_append_nameseg(bus_table, "_SUN");
>          build_append_value(bus_table, PCI_SLOT(bus->parent_dev->devfn), 1);
> -        build_append_byte(bus_table, 0x08); /* NameOp */
> +        build_append_byte(bus_table, NameOp);
>          build_append_nameseg(bus_table, "_ADR");
>          build_append_value(bus_table, (PCI_SLOT(bus->parent_dev->devfn) << 16) |
>                             PCI_FUNC(bus->parent_dev->devfn), 4);
>      } else {
> -        op = 0x10; /* ScopeOp */;
> +        op = ScopeOp;
>          build_append_nameseg(bus_table, "PCI0");
>      }
>  
>      bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL);
>      if (bsel) {
> -        build_append_byte(bus_table, 0x08); /* NameOp */
> +        build_append_byte(bus_table, NameOp);
>          build_append_nameseg(bus_table, "BSEL");
>          build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
>      }
> @@ -962,7 +970,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>  
>      {
>          GArray *sb_scope = build_alloc_array();
> -        uint8_t op = 0x10; /* ScopeOp */
> +        uint8_t op = ScopeOp;
>  
>          build_append_nameseg(sb_scope, "_SB_");
>  
> @@ -983,7 +991,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>          build_append_notify_method(sb_scope, "NTFY", "CP%0.02X", acpi_cpus);
>  
>          /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */
> -        build_append_byte(sb_scope, 0x08); /* NameOp */
> +        build_append_byte(sb_scope, NameOp);
>          build_append_nameseg(sb_scope, "CPON");
>  
>          {
> -- 
> 1.7.1

[Qemu-devel] [RFC 5/9] acpi: add PNP0C02 to PCI0 bus

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 55 insertions(+), 0 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 1dbe5ce..f0bedbd 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -492,6 +492,55 @@ static inline void acpi_add_table(GArray *table_offsets, GArray *table_data)
     g_array_append_val(table_offsets, offset);
 }
 
+static uint8_t Hex2Digit(char c)
+{
+    if (c >= 'A') {
+        return c - 'A' + 10;
+    }
+    return c - '0';
+}
+
+static uint32_t encodeEisaId(const char *str)
+{
+    uint32_t ret;
+    g_assert(strlen(str) == 7);
+    ret = (str[0] - 0x40) << 26 |
+          (str[1] - 0x40) << 21 |
+          (str[2] - 0x40) << 16 |
+          Hex2Digit(str[3]) << 12 |
+          Hex2Digit(str[4]) << 8 |
+          Hex2Digit(str[5]) << 4 |
+          Hex2Digit(str[6]);
+    return bswap32(ret);
+}
+
+#define ACPI_SCOPE(ctx, name, ...) {\
+    GArray *name = build_alloc_array(); \
+    build_append_nameseg(name, stringify(name)); \
+    __VA_ARGS__; \
+    build_package(name, ScopeOp, 0); \
+    build_append_array(ctx, name); \
+    build_free_array(name); \
+}
+
+#define ACPI_NAME(ctx, name) { \
+    build_append_byte(ctx, NameOp); \
+    build_append_nameseg(ctx, name); \
+}
+
+#define ACPI_EISAID(ctx, val) { \
+    build_append_value(ctx, encodeEisaId(val), sizeof(uint32_t)); \
+}
+
+#define ACPI_DEVICE(ctx, name, ...) {\
+    GArray *name = build_alloc_array(); \
+    build_append_nameseg(name, stringify(name)); \
+    __VA_ARGS__; \
+    build_extop_package(name, DeviceOp); \
+    build_append_array(ctx, name); \
+    build_free_array(name); \
+}
+
 /* FACS */
 static void
 build_facs(GArray *table_data, GArray *linker, PcGuestInfo *guest_info)
@@ -1032,6 +1081,12 @@ build_ssdt(GArray *table_data, GArray *linker,
             build_pci_bus_state_cleanup(&hotplug_state);
         }
 
+        ACPI_SCOPE(sb_scope, PCI0,
+            ACPI_DEVICE(PCI0, MRES,
+                ACPI_NAME(MRES, "_HID"); ACPI_EISAID(MRES, "PNP0C02");
+            );
+        );
+
         build_package(sb_scope, op, 3);
         build_append_array(table_data, sb_scope);
         build_free_array(sb_scope);
-- 
1.7.1

Re: [Qemu-devel] [RFC 5/9] acpi: add PNP0C02 to PCI0 bus

[Michael S. Tsirkin]

On Fri, Feb 07, 2014 at 01:51:32PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/i386/acpi-build.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 55 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 1dbe5ce..f0bedbd 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -492,6 +492,55 @@ static inline void acpi_add_table(GArray *table_offsets, GArray *table_data)
>      g_array_append_val(table_offsets, offset);
>  }
>  
> +static uint8_t Hex2Digit(char c)
> +{
> +    if (c >= 'A') {
> +        return c - 'A' + 10;
> +    }
> +    return c - '0';
> +}
> +
> +static uint32_t encodeEisaId(const char *str)
> +{
> +    uint32_t ret;
> +    g_assert(strlen(str) == 7);
> +    ret = (str[0] - 0x40) << 26 |
> +          (str[1] - 0x40) << 21 |
> +          (str[2] - 0x40) << 16 |
> +          Hex2Digit(str[3]) << 12 |
> +          Hex2Digit(str[4]) << 8 |
> +          Hex2Digit(str[5]) << 4 |
> +          Hex2Digit(str[6]);
> +    return bswap32(ret);
> +}
> +

Why the camel case here?
Seems uncalled for ...

> +#define ACPI_SCOPE(ctx, name, ...) {\
> +    GArray *name = build_alloc_array(); \
> +    build_append_nameseg(name, stringify(name)); \
> +    __VA_ARGS__; \
> +    build_package(name, ScopeOp, 0); \
> +    build_append_array(ctx, name); \
> +    build_free_array(name); \
> +}
> +
> +#define ACPI_NAME(ctx, name) { \
> +    build_append_byte(ctx, NameOp); \
> +    build_append_nameseg(ctx, name); \
> +}
> +
> +#define ACPI_EISAID(ctx, val) { \
> +    build_append_value(ctx, encodeEisaId(val), sizeof(uint32_t)); \
> +}
> +
> +#define ACPI_DEVICE(ctx, name, ...) {\
> +    GArray *name = build_alloc_array(); \
> +    build_append_nameseg(name, stringify(name)); \
> +    __VA_ARGS__; \
> +    build_extop_package(name, DeviceOp); \
> +    build_append_array(ctx, name); \
> +    build_free_array(name); \
> +}
> +
>  /* FACS */
>  static void
>  build_facs(GArray *table_data, GArray *linker, PcGuestInfo *guest_info)
> @@ -1032,6 +1081,12 @@ build_ssdt(GArray *table_data, GArray *linker,
>              build_pci_bus_state_cleanup(&hotplug_state);
>          }
>  
> +        ACPI_SCOPE(sb_scope, PCI0,
> +            ACPI_DEVICE(PCI0, MRES,
> +                ACPI_NAME(MRES, "_HID"); ACPI_EISAID(MRES, "PNP0C02");
> +            );
> +        );
> +

Wow you managed to make C look like ACPI,
including 4-character identifier limitation.
This needs some thought.

>          build_package(sb_scope, op, 3);
>          build_append_array(table_data, sb_scope);
>          build_free_array(sb_scope);
> -- 
> 1.7.1

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

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 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
+#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

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 <imammedo@redhat.com>
> ---
>  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

[Qemu-devel] [RFC 7/9] acpi: consume CPU hotplug IO resource in PNP0C02 device

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index ce5f715..5cd0c80 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -70,6 +70,8 @@ typedef struct AcpiPmInfo {
     uint32_t gpe0_blk;
     uint32_t gpe0_blk_len;
     uint32_t io_base;
+    uint16_t cpuhp_io_base;
+    uint16_t cpuhp_io_len;
 } AcpiPmInfo;
 
 typedef struct AcpiMiscInfo {
@@ -141,11 +143,14 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
     Object *obj = NULL;
     QObject *o;
 
+    pm->cpuhp_io_len = ACPI_GPE_PROC_LEN;
     if (piix) {
         obj = piix;
+        pm->cpuhp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
     }
     if (lpc) {
         obj = lpc;
+        pm->cpuhp_io_base = ICH9_CPU_HOTPLUG_IO_BASE;
     }
     assert(obj);
 
@@ -1145,6 +1150,11 @@ build_ssdt(GArray *table_data, GArray *linker,
                             pm->gpe0_blk,      /* _MAX */
                             0x0,               /* _ALN */
                             pm->gpe0_blk_len); /* _LEN */
+                    ACPI_IO(RESBUF, Decode16,
+                            pm->cpuhp_io_base, /* _MIN */
+                            pm->cpuhp_io_base, /* _MAX */
+                            0x0,               /* _ALN */
+                            pm->cpuhp_io_len); /* _LEN */
                 );
             );
         );
-- 
1.7.1

Re: [Qemu-devel] [RFC 7/9] acpi: consume CPU hotplug IO resource in PNP0C02 device

[Michael S. Tsirkin]

On Fri, Feb 07, 2014 at 01:51:34PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/i386/acpi-build.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index ce5f715..5cd0c80 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -70,6 +70,8 @@ typedef struct AcpiPmInfo {
>      uint32_t gpe0_blk;
>      uint32_t gpe0_blk_len;
>      uint32_t io_base;
> +    uint16_t cpuhp_io_base;
> +    uint16_t cpuhp_io_len;
>  } AcpiPmInfo;
>  
>  typedef struct AcpiMiscInfo {
> @@ -141,11 +143,14 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
>      Object *obj = NULL;
>      QObject *o;
>  
> +    pm->cpuhp_io_len = ACPI_GPE_PROC_LEN;
>      if (piix) {
>          obj = piix;
> +        pm->cpuhp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
>      }
>      if (lpc) {
>          obj = lpc;
> +        pm->cpuhp_io_base = ICH9_CPU_HOTPLUG_IO_BASE;
>      }
>      assert(obj);
>  

Not a must but would be nicer to get these as
device properties.

If we change this, cpuhp_io_len can be used directly ...


> @@ -1145,6 +1150,11 @@ build_ssdt(GArray *table_data, GArray *linker,
>                              pm->gpe0_blk,      /* _MAX */
>                              0x0,               /* _ALN */
>                              pm->gpe0_blk_len); /* _LEN */
> +                    ACPI_IO(RESBUF, Decode16,
> +                            pm->cpuhp_io_base, /* _MIN */
> +                            pm->cpuhp_io_base, /* _MAX */
> +                            0x0,               /* _ALN */
> +                            pm->cpuhp_io_len); /* _LEN */
>                  );
>              );
>          );
> -- 
> 1.7.1

[Qemu-devel] [RFC 8/9] pcihp: expose PCI hotplug MMIO base/length as properties of piix4pm

[Igor Mammedov]

with introduction of PCIHP, MMIO range becomes changable
at runtime so it's not possible to statically punch hole
PCI bus _CRS.

Making IO base/length available as readonly properties
allow acpi builder to get values and reserve PCI hotplug
IO range at runtime later.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/acpi/pcihp.c         |   28 ++++++++++++++++++++++++++++
 hw/acpi/piix4.c         |    1 +
 include/hw/acpi/pcihp.h |    4 ++++
 3 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 1ce6fc2..629d364 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -37,6 +37,7 @@
 #include "hw/pci/pci_bus.h"
 #include "qom/qom-qobject.h"
 #include "qapi/qmp/qint.h"
+#include "qapi/visitor.h"
 
 //#define DEBUG
 
@@ -309,3 +310,30 @@ const VMStateDescription vmstate_acpi_pcihp_pci_status = {
         VMSTATE_END_OF_LIST()
     }
 };
+
+static void acpi_pcihp_get_io_addr(Object *obj, Visitor *v, void *opaque,
+                                     const char *name, Error **errp)
+{
+    AcpiPciHpState *s = opaque;
+    MemoryRegionSection mr_info = memory_region_find(&s->io, 0, 1);
+    uint16_t value = mr_info.offset_within_address_space;
+
+    visit_type_uint16(v, &value, name, errp);
+}
+
+static void acpi_pcihp_get_io_len(Object *obj, Visitor *v, void *opaque,
+                                     const char *name, Error **errp)
+{
+    AcpiPciHpState *s = opaque;
+    uint16_t value = memory_region_size(&s->io);
+
+    visit_type_uint16(v, &value, name, errp);
+}
+
+void acpi_pcihp_add_mmio_properties(Object *obj, AcpiPciHpState *s)
+{
+    object_property_add(obj, ACPI_PCIHP_IO_ADDR, "uint16",
+                        acpi_pcihp_get_io_addr, NULL, NULL, s, NULL);
+    object_property_add(obj, ACPI_PCIHP_IO_LEN, "uint16",
+                        acpi_pcihp_get_io_len, NULL, NULL, s, NULL);
+}
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 7a0efcb..034c5cd 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -558,6 +558,7 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
                           "acpi-gpe0", GPE_LEN);
     memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
 
+    acpi_pcihp_add_mmio_properties(OBJECT(s), &s->acpi_pci_hotplug);
     acpi_pcihp_init(&s->acpi_pci_hotplug, bus, parent,
                     s->use_acpi_pci_hotplug);
 
diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
index 0a90e4a..0eb4e1c 100644
--- a/include/hw/acpi/pcihp.h
+++ b/include/hw/acpi/pcihp.h
@@ -70,4 +70,8 @@ extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
                                   vmstate_acpi_pcihp_pci_status, \
                                   AcpiPciHpPciStatus)
 
+#define ACPI_PCIHP_IO_ADDR "pcihp-io-addr"
+#define ACPI_PCIHP_IO_LEN  "pcihp-io-len"
+
+void acpi_pcihp_add_mmio_properties(Object *obj, AcpiPciHpState *s);
 #endif
-- 
1.7.1

Re: [Qemu-devel] [RFC 8/9] pcihp: expose PCI hotplug MMIO base/length as properties of piix4pm

[Michael S. Tsirkin]

On Fri, Feb 07, 2014 at 01:51:35PM +0100, Igor Mammedov wrote:
> with introduction of PCIHP, MMIO range becomes changable
> at runtime so it's not possible to statically punch hole
> PCI bus _CRS.
> 
> Making IO base/length available as readonly properties
> allow acpi builder to get values and reserve PCI hotplug
> IO range at runtime later.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

No objection but I wonder: isn't it already possible to enumerate
memory regions?

> ---
>  hw/acpi/pcihp.c         |   28 ++++++++++++++++++++++++++++
>  hw/acpi/piix4.c         |    1 +
>  include/hw/acpi/pcihp.h |    4 ++++
>  3 files changed, 33 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 1ce6fc2..629d364 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -37,6 +37,7 @@
>  #include "hw/pci/pci_bus.h"
>  #include "qom/qom-qobject.h"
>  #include "qapi/qmp/qint.h"
> +#include "qapi/visitor.h"
>  
>  //#define DEBUG
>  
> @@ -309,3 +310,30 @@ const VMStateDescription vmstate_acpi_pcihp_pci_status = {
>          VMSTATE_END_OF_LIST()
>      }
>  };
> +
> +static void acpi_pcihp_get_io_addr(Object *obj, Visitor *v, void *opaque,
> +                                     const char *name, Error **errp)
> +{
> +    AcpiPciHpState *s = opaque;
> +    MemoryRegionSection mr_info = memory_region_find(&s->io, 0, 1);
> +    uint16_t value = mr_info.offset_within_address_space;
> +
> +    visit_type_uint16(v, &value, name, errp);
> +}
> +
> +static void acpi_pcihp_get_io_len(Object *obj, Visitor *v, void *opaque,
> +                                     const char *name, Error **errp)
> +{
> +    AcpiPciHpState *s = opaque;
> +    uint16_t value = memory_region_size(&s->io);
> +
> +    visit_type_uint16(v, &value, name, errp);
> +}
> +
> +void acpi_pcihp_add_mmio_properties(Object *obj, AcpiPciHpState *s)
> +{
> +    object_property_add(obj, ACPI_PCIHP_IO_ADDR, "uint16",
> +                        acpi_pcihp_get_io_addr, NULL, NULL, s, NULL);
> +    object_property_add(obj, ACPI_PCIHP_IO_LEN, "uint16",
> +                        acpi_pcihp_get_io_len, NULL, NULL, s, NULL);
> +}
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 7a0efcb..034c5cd 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -558,6 +558,7 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
>                            "acpi-gpe0", GPE_LEN);
>      memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
>  
> +    acpi_pcihp_add_mmio_properties(OBJECT(s), &s->acpi_pci_hotplug);
>      acpi_pcihp_init(&s->acpi_pci_hotplug, bus, parent,
>                      s->use_acpi_pci_hotplug);
>  
> diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> index 0a90e4a..0eb4e1c 100644
> --- a/include/hw/acpi/pcihp.h
> +++ b/include/hw/acpi/pcihp.h
> @@ -70,4 +70,8 @@ extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
>                                    vmstate_acpi_pcihp_pci_status, \
>                                    AcpiPciHpPciStatus)
>  
> +#define ACPI_PCIHP_IO_ADDR "pcihp-io-addr"
> +#define ACPI_PCIHP_IO_LEN  "pcihp-io-len"
> +
> +void acpi_pcihp_add_mmio_properties(Object *obj, AcpiPciHpState *s);
>  #endif
> -- 
> 1.7.1

[Qemu-devel] [RFC 9/9] acpi: consume PCIHP IO resource in PNP0C02 device

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 5cd0c80..e3d145b 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -72,6 +72,8 @@ typedef struct AcpiPmInfo {
     uint32_t io_base;
     uint16_t cpuhp_io_base;
     uint16_t cpuhp_io_len;
+    uint16_t pcihp_io_base;
+    uint16_t pcihp_io_len;
 } AcpiPmInfo;
 
 typedef struct AcpiMiscInfo {
@@ -189,6 +191,19 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
                                            NULL);
     pm->gpe0_blk_len = object_property_get_int(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
                                                NULL);
+    {
+        Error  *local_error = NULL;
+        uint16_t val = object_property_get_int(obj, ACPI_PCIHP_IO_ADDR,
+                                               &local_error);
+        if (!local_error) {
+            pm->pcihp_io_base = val;
+        }
+
+        val = object_property_get_int(obj, ACPI_PCIHP_IO_LEN, &local_error);
+        if (!local_error) {
+            pm->pcihp_io_len = val;
+        }
+    }
 }
 
 static void acpi_get_misc_info(AcpiMiscInfo *info)
@@ -1155,6 +1170,13 @@ build_ssdt(GArray *table_data, GArray *linker,
                             pm->cpuhp_io_base, /* _MAX */
                             0x0,               /* _ALN */
                             pm->cpuhp_io_len); /* _LEN */
+                    if (pm->pcihp_io_base) {
+                        ACPI_IO(RESBUF, Decode16,
+                                pm->pcihp_io_base, /* _MIN */
+                                pm->pcihp_io_base, /* _MAX */
+                                0x0,               /* _ALN */
+                                pm->pcihp_io_len); /* _LEN */
+                    }
                 );
             );
         );
-- 
1.7.1

Re: [Qemu-devel] [RFC 0/9] generate dynamic _CRS for motherboard resources

[Michael S. Tsirkin]

On Fri, Feb 07, 2014 at 01:51:27PM +0100, Igor Mammedov wrote:
> Since introduction of PCIHP, it became problematic to
> punch hole in PCI0._CRS statically since PCI hotplug
> region size became runtime changeable.

What makes it runtime changeable?

> So replace static hole punching with dynamically consumed
> resources in a child device on PCI0 bus. i.e generate
> PNP0C02 device as a child of PCI0 bus at runtime and
> consume GPE0, PCI/CPU hotplug IO resources in it instead
> of punching holes in static PCI0._CRS.

It seems that we are being too exact with
IO resources here.
Can't we roughly reserve 0xae00 to 0xafff
and be done with it?

> Tested with Windows XPsp3, Vista, Windows Server 2003, 2008, 2012r2.
> 
> PS:
> Series adds several ASL like macros to simplify
> code for dynamic generation of AML structures.
> 
> Igor Mammedov (9):
>   Revert "pc: Q35 DSDT: exclude CPU hotplug IO range from PCI bus
>     resources"
>   Revert "pc: PIIX DSDT: exclude CPU/PCI hotplug & GPE0 IO range from
>     PCI bus resources"
>   Partial revert "pc: ACPI: expose PRST IO range via _CRS"
>   acpi: replace opencoded opcodes with defines
>   acpi: add PNP0C02 to PCI0 bus
>   acpi: consume GPE0 IO resources in PNP0C02 device
>   acpi: consume CPU hotplug IO resource in PNP0C02 device
>   pcihp: expose PCI hotplug MMIO base/length as properties of piix4pm
>   acpi: consume PCIHP IO resource in PNP0C02 device
> 
>  hw/acpi/pcihp.c                   |   28 ++++++
>  hw/acpi/piix4.c                   |    1 +
>  hw/i386/acpi-build.c              |  177 ++++++++++++++++++++++++++++++++++--
>  hw/i386/acpi-dsdt-cpu-hotplug.dsl |   11 ---
>  hw/i386/acpi-dsdt-pci-crs.dsl     |   15 +++-
>  hw/i386/acpi-dsdt.dsl             |   39 --------
>  hw/i386/q35-acpi-dsdt.dsl         |   16 ----
>  include/hw/acpi/pcihp.h           |    4 +
>  8 files changed, 214 insertions(+), 77 deletions(-)

Re: [Qemu-devel] [RFC 0/9] generate dynamic _CRS for motherboard resources

[Gerd Hoffmann]

On So, 2014-02-16 at 17:53 +0200, Michael S. Tsirkin wrote:
> On Fri, Feb 07, 2014 at 01:51:27PM +0100, Igor Mammedov wrote:
> > Since introduction of PCIHP, it became problematic to
> > punch hole in PCI0._CRS statically since PCI hotplug
> > region size became runtime changeable.
> 
> What makes it runtime changeable?

machine type.  q35 / piix map them at different locations.

Also we might want to this also for devices which are
runtime-configurable (isa-debugcon, pvpanic, ...).

cheers,
  Gerd

Re: [Qemu-devel] [RFC 0/9] generate dynamic _CRS for motherboard resources

[Michael S. Tsirkin]

On Mon, Feb 17, 2014 at 09:32:35AM +0100, Gerd Hoffmann wrote:
> On So, 2014-02-16 at 17:53 +0200, Michael S. Tsirkin wrote:
> > On Fri, Feb 07, 2014 at 01:51:27PM +0100, Igor Mammedov wrote:
> > > Since introduction of PCIHP, it became problematic to
> > > punch hole in PCI0._CRS statically since PCI hotplug
> > > region size became runtime changeable.
> > 
> > What makes it runtime changeable?
> 
> machine type.  q35 / piix map them at different locations.

That's not dynamic.  We can load the correct ones per DSDT.

> Also we might want to this also for devices which are
> runtime-configurable (isa-debugcon, pvpanic, ...).

That's more convincing, but I don't want
knowledge of all these devices in acpi-build.
Also we need to make seabios avoid these ranges
when enumerating devices.
How does it know to avoid them ATM?

> 
> cheers,
>   Gerd
> 

Re: [Qemu-devel] [RFC 0/9] generate dynamic _CRS for motherboard resources

[Gerd Hoffmann]

On Mo, 2014-02-17 at 12:28 +0200, Michael S. Tsirkin wrote:
> On Mon, Feb 17, 2014 at 09:32:35AM +0100, Gerd Hoffmann wrote:
> > On So, 2014-02-16 at 17:53 +0200, Michael S. Tsirkin wrote:
> > > On Fri, Feb 07, 2014 at 01:51:27PM +0100, Igor Mammedov wrote:
> > > > Since introduction of PCIHP, it became problematic to
> > > > punch hole in PCI0._CRS statically since PCI hotplug
> > > > region size became runtime changeable.
> > > 
> > > What makes it runtime changeable?
> > 
> > machine type.  q35 / piix map them at different locations.
> 
> That's not dynamic.  We can load the correct ones per DSDT.
> 
> > Also we might want to this also for devices which are
> > runtime-configurable (isa-debugcon, pvpanic, ...).
> 
> That's more convincing, but I don't want
> knowledge of all these devices in acpi-build.
> Also we need to make seabios avoid these ranges
> when enumerating devices.
> How does it know to avoid them ATM?

seabios maps io ports @ 0xc000 up.
recently it has changed to use 0x1000 -> 0xa000 region
in case the hole above 0xc000 is too small.

In other words: It doesn't map anything below 0x1000 and it avoids
0xa000 -> 0xbfff.  Hardcoded.  I want lift the later restriction on q35,
by moving pmbase (0xb000 atm) out of the way, so seabios can use the
whole 0x1000 -> 0xffff range, but that is still wip.

cheers,
  Gerd

Re: [Qemu-devel] [RFC 0/9] generate dynamic _CRS for motherboard resources

[Michael S. Tsirkin]

On Mon, Feb 17, 2014 at 11:46:13AM +0100, Gerd Hoffmann wrote:
> On Mo, 2014-02-17 at 12:28 +0200, Michael S. Tsirkin wrote:
> > On Mon, Feb 17, 2014 at 09:32:35AM +0100, Gerd Hoffmann wrote:
> > > On So, 2014-02-16 at 17:53 +0200, Michael S. Tsirkin wrote:
> > > > On Fri, Feb 07, 2014 at 01:51:27PM +0100, Igor Mammedov wrote:
> > > > > Since introduction of PCIHP, it became problematic to
> > > > > punch hole in PCI0._CRS statically since PCI hotplug
> > > > > region size became runtime changeable.
> > > > 
> > > > What makes it runtime changeable?
> > > 
> > > machine type.  q35 / piix map them at different locations.
> > 
> > That's not dynamic.  We can load the correct ones per DSDT.
> > 
> > > Also we might want to this also for devices which are
> > > runtime-configurable (isa-debugcon, pvpanic, ...).
> > 
> > That's more convincing, but I don't want
> > knowledge of all these devices in acpi-build.
> > Also we need to make seabios avoid these ranges
> > when enumerating devices.
> > How does it know to avoid them ATM?
> 
> seabios maps io ports @ 0xc000 up.
> recently it has changed to use 0x1000 -> 0xa000 region
> in case the hole above 0xc000 is too small.
> 
> In other words: It doesn't map anything below 0x1000 and it avoids
> 0xa000 -> 0xbfff.  Hardcoded.  I want lift the later restriction on q35,
> by moving pmbase (0xb000 atm) out of the way, so seabios can use the
> whole 0x1000 -> 0xffff range, but that is still wip.
> 
> cheers,
>   Gerd
> 

okay so we'll want some fwcfg for that correct?
whatever fills that, can share logic with acpi
generation :)

That might or might not make it dynamic enough to make it worth
bothering - alternative is just two version of acpi
depending on machine type.

-- 
MST

Re: [Qemu-devel] [RFC 0/9] generate dynamic _CRS for motherboard resources

[Igor Mammedov]

On Mon, 17 Feb 2014 09:32:35 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

> On So, 2014-02-16 at 17:53 +0200, Michael S. Tsirkin wrote:
> > On Fri, Feb 07, 2014 at 01:51:27PM +0100, Igor Mammedov wrote:
> > > Since introduction of PCIHP, it became problematic to
> > > punch hole in PCI0._CRS statically since PCI hotplug
> > > region size became runtime changeable.
> > 
> > What makes it runtime changeable?
> 
> machine type.  q35 / piix map them at different locations.
> 
> Also we might want to this also for devices which are
> runtime-configurable (isa-debugcon, pvpanic, ...).
I'd convert simple devices that conditionally enabled at
startup time, from static definition + patching into
completely dynamically generated when device present.
For example pvpanic falls in to this category.

That would result in smaller ACPI tables guest has to deal with.

> 
> cheers,
>   Gerd
> 
> 

Re: [Qemu-devel] [RFC 0/9] generate dynamic _CRS for motherboard resources

[Laszlo Ersek]

On 02/18/14 17:36, Igor Mammedov wrote:
> On Mon, 17 Feb 2014 09:32:35 +0100
> Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
>> On So, 2014-02-16 at 17:53 +0200, Michael S. Tsirkin wrote:
>>> On Fri, Feb 07, 2014 at 01:51:27PM +0100, Igor Mammedov wrote:
>>>> Since introduction of PCIHP, it became problematic to
>>>> punch hole in PCI0._CRS statically since PCI hotplug
>>>> region size became runtime changeable.
>>>
>>> What makes it runtime changeable?
>>
>> machine type.  q35 / piix map them at different locations.
>>
>> Also we might want to this also for devices which are
>> runtime-configurable (isa-debugcon, pvpanic, ...).
> I'd convert simple devices that conditionally enabled at
> startup time, from static definition + patching into
> completely dynamically generated when device present.
> For example pvpanic falls in to this category.
> 
> That would result in smaller ACPI tables guest has to deal with.

I could be mistaken, but AFAIR this caused the windows device manager to
pop up in windows? Ie. if you have a windows guest and cold-boot it
twice, once with the device present (generated into ACPI) and once with
the device absent (not generated into ACPI), then you get hardware
changes. Whereas, if the device is always present and you only patch
_STA, then windows doesn't perceive it as a hw change.

Do I recall it right?...

You could argue that "a new device indeed warrants a device manager
popup", but esp. for isa-debugcon and pvpanic, you might want to enable
those opportunistically, without triggering a new hw dialog. Pvpanic
triggering the device manager was exactly what drew frowns, for its
original implementation. IIRC.

Anyway pls. feel free to ignore this comment, it just crossed my mind.
(And of course it's not related to your series.)

Thanks
Laszlo

Re: [Qemu-devel] [RFC 0/9] generate dynamic _CRS for motherboard resources

[Igor Mammedov]

On Tue, 18 Feb 2014 23:04:13 +0100
Laszlo Ersek <lersek@redhat.com> wrote:

> On 02/18/14 17:36, Igor Mammedov wrote:
> > On Mon, 17 Feb 2014 09:32:35 +0100
> > Gerd Hoffmann <kraxel@redhat.com> wrote:
> > 
> >> On So, 2014-02-16 at 17:53 +0200, Michael S. Tsirkin wrote:
> >>> On Fri, Feb 07, 2014 at 01:51:27PM +0100, Igor Mammedov wrote:
> >>>> Since introduction of PCIHP, it became problematic to
> >>>> punch hole in PCI0._CRS statically since PCI hotplug
> >>>> region size became runtime changeable.
> >>>
> >>> What makes it runtime changeable?
> >>
> >> machine type.  q35 / piix map them at different locations.
> >>
> >> Also we might want to this also for devices which are
> >> runtime-configurable (isa-debugcon, pvpanic, ...).
> > I'd convert simple devices that conditionally enabled at
> > startup time, from static definition + patching into
> > completely dynamically generated when device present.
> > For example pvpanic falls in to this category.
> > 
> > That would result in smaller ACPI tables guest has to deal with.
> 
> I could be mistaken, but AFAIR this caused the windows device manager to
> pop up in windows? Ie. if you have a windows guest and cold-boot it
> twice, once with the device present (generated into ACPI) and once with
> the device absent (not generated into ACPI), then you get hardware
> changes. Whereas, if the device is always present and you only patch
> _STA, then windows doesn't perceive it as a hw change.

Is is irrelevant whether device is statically or dynamically created,
user will face the same issue and has a choice to install driver or tell
windows to ignore device forever.

> Do I recall it right?...
Device manager pop-ups only once when new device is added to install driver.
If later, the device in the same location with the same id appears/disappears,
Device manager handles it silently.

> 
> You could argue that "a new device indeed warrants a device manager
> popup", but esp. for isa-debugcon and pvpanic, you might want to enable
> those opportunistically, without triggering a new hw dialog. Pvpanic
> triggering the device manager was exactly what drew frowns, for its
> original implementation. IIRC.
the above applies to these cases as well, i.e. if you install driver for
it then there won't be any pop-ups later. If there is no driver
(which is the case) one needs to tell to Device manager to ignore
this device, and then there shouldn't be an additional pop-ups later.

> Anyway pls. feel free to ignore this comment, it just crossed my mind.
> (And of course it's not related to your series.)
> 
> Thanks
> Laszlo
> 

Re: [Qemu-devel] [RFC 0/9] generate dynamic _CRS for motherboard resources

[Igor Mammedov]

On Sun, 16 Feb 2014 17:53:45 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Feb 07, 2014 at 01:51:27PM +0100, Igor Mammedov wrote:
> > Since introduction of PCIHP, it became problematic to
> > punch hole in PCI0._CRS statically since PCI hotplug
> > region size became runtime changeable.
> 
> What makes it runtime changeable?
piix machine "acpi-pci-hotplug-with-bridge-support=on" effectively
changes size of pcihp MMIO region
> 
> > So replace static hole punching with dynamically consumed
> > resources in a child device on PCI0 bus. i.e generate
> > PNP0C02 device as a child of PCI0 bus at runtime and
> > consume GPE0, PCI/CPU hotplug IO resources in it instead
> > of punching holes in static PCI0._CRS.
> 
> It seems that we are being too exact with
> IO resources here.
> Can't we roughly reserve 0xae00 to 0xafff
> and be done with it?
that would be easiest way for this specific case if we could agree
for ranges on PIIX/Q35 machines.

But I also use it as excuse to introduce ASL like macros so that
building dynamic SSDT would be easier i.e. replace template patching
with single place where dynamic device is defined and its values
are set in simple and straightforward manner.

> > Tested with Windows XPsp3, Vista, Windows Server 2003, 2008, 2012r2.
> > 
> > PS:
> > Series adds several ASL like macros to simplify
> > code for dynamic generation of AML structures.
> > 
> > Igor Mammedov (9):
> >   Revert "pc: Q35 DSDT: exclude CPU hotplug IO range from PCI bus
> >     resources"
> >   Revert "pc: PIIX DSDT: exclude CPU/PCI hotplug & GPE0 IO range from
> >     PCI bus resources"
> >   Partial revert "pc: ACPI: expose PRST IO range via _CRS"
> >   acpi: replace opencoded opcodes with defines
> >   acpi: add PNP0C02 to PCI0 bus
> >   acpi: consume GPE0 IO resources in PNP0C02 device
> >   acpi: consume CPU hotplug IO resource in PNP0C02 device
> >   pcihp: expose PCI hotplug MMIO base/length as properties of piix4pm
> >   acpi: consume PCIHP IO resource in PNP0C02 device
> > 
> >  hw/acpi/pcihp.c                   |   28 ++++++
> >  hw/acpi/piix4.c                   |    1 +
> >  hw/i386/acpi-build.c              |  177 ++++++++++++++++++++++++++++++++++--
> >  hw/i386/acpi-dsdt-cpu-hotplug.dsl |   11 ---
> >  hw/i386/acpi-dsdt-pci-crs.dsl     |   15 +++-
> >  hw/i386/acpi-dsdt.dsl             |   39 --------
> >  hw/i386/q35-acpi-dsdt.dsl         |   16 ----
> >  include/hw/acpi/pcihp.h           |    4 +
> >  8 files changed, 214 insertions(+), 77 deletions(-)


-- 
Regards,
  Igor

Re: [Qemu-devel] [RFC 0/9] generate dynamic _CRS for motherboard resources

[Michael S. Tsirkin]

On Mon, Feb 17, 2014 at 11:33:23AM +0100, Igor Mammedov wrote:
> On Sun, 16 Feb 2014 17:53:45 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Fri, Feb 07, 2014 at 01:51:27PM +0100, Igor Mammedov wrote:
> > > Since introduction of PCIHP, it became problematic to
> > > punch hole in PCI0._CRS statically since PCI hotplug
> > > region size became runtime changeable.
> > 
> > What makes it runtime changeable?
> piix machine "acpi-pci-hotplug-with-bridge-support=on" effectively
> changes size of pcihp MMIO region

It adds 4 bytes. Let's just reserve a reasonably sized region,
like 512 bytes.

> > 
> > > So replace static hole punching with dynamically consumed
> > > resources in a child device on PCI0 bus. i.e generate
> > > PNP0C02 device as a child of PCI0 bus at runtime and
> > > consume GPE0, PCI/CPU hotplug IO resources in it instead
> > > of punching holes in static PCI0._CRS.
> > 
> > It seems that we are being too exact with
> > IO resources here.
> > Can't we roughly reserve 0xae00 to 0xafff
> > and be done with it?
> that would be easiest way for this specific case if we could agree
> for ranges on PIIX/Q35 machines.
> 
> But I also use it as excuse to introduce ASL like macros so that
> building dynamic SSDT would be easier i.e. replace template patching
> with single place where dynamic device is defined and its values
> are set in simple and straightforward manner.

We don't need excuses really :)
But the approach itself needs some work IMHO, in particular
ASL like syntax by using macros and varargs is not something to strive
for IMHO :)

Why don't we just pass GArrays around?

crs = build_alloc_crs();
build_append_resource(crs, ....)



Or if you really want this, using array of GArray:

build_append_crs(ssdt,
		{
			build_alloc_resource(...),
			build_alloc_resource(...),
			build_alloc_resource(...),
			build_alloc_resource(...),
			NULL
		})


> > > Tested with Windows XPsp3, Vista, Windows Server 2003, 2008, 2012r2.
> > > 
> > > PS:
> > > Series adds several ASL like macros to simplify
> > > code for dynamic generation of AML structures.
> > > 
> > > Igor Mammedov (9):
> > >   Revert "pc: Q35 DSDT: exclude CPU hotplug IO range from PCI bus
> > >     resources"
> > >   Revert "pc: PIIX DSDT: exclude CPU/PCI hotplug & GPE0 IO range from
> > >     PCI bus resources"
> > >   Partial revert "pc: ACPI: expose PRST IO range via _CRS"
> > >   acpi: replace opencoded opcodes with defines
> > >   acpi: add PNP0C02 to PCI0 bus
> > >   acpi: consume GPE0 IO resources in PNP0C02 device
> > >   acpi: consume CPU hotplug IO resource in PNP0C02 device
> > >   pcihp: expose PCI hotplug MMIO base/length as properties of piix4pm
> > >   acpi: consume PCIHP IO resource in PNP0C02 device
> > > 
> > >  hw/acpi/pcihp.c                   |   28 ++++++
> > >  hw/acpi/piix4.c                   |    1 +
> > >  hw/i386/acpi-build.c              |  177 ++++++++++++++++++++++++++++++++++--
> > >  hw/i386/acpi-dsdt-cpu-hotplug.dsl |   11 ---
> > >  hw/i386/acpi-dsdt-pci-crs.dsl     |   15 +++-
> > >  hw/i386/acpi-dsdt.dsl             |   39 --------
> > >  hw/i386/q35-acpi-dsdt.dsl         |   16 ----
> > >  include/hw/acpi/pcihp.h           |    4 +
> > >  8 files changed, 214 insertions(+), 77 deletions(-)
> 
> 
> -- 
> Regards,
>   Igor

Re: [Qemu-devel] [RFC 0/9] generate dynamic _CRS for motherboard resources

[Igor Mammedov]

On Mon, 17 Feb 2014 13:02:18 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Feb 17, 2014 at 11:33:23AM +0100, Igor Mammedov wrote:
> > On Sun, 16 Feb 2014 17:53:45 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Fri, Feb 07, 2014 at 01:51:27PM +0100, Igor Mammedov wrote:
> > > > Since introduction of PCIHP, it became problematic to
> > > > punch hole in PCI0._CRS statically since PCI hotplug
> > > > region size became runtime changeable.
> > > 
> > > What makes it runtime changeable?
> > piix machine "acpi-pci-hotplug-with-bridge-support=on" effectively
> > changes size of pcihp MMIO region
> 
> It adds 4 bytes. Let's just reserve a reasonably sized region,
> like 512 bytes.
Reserve where and for how log it will be enough?
Eventually we will continue punching holes or increasing size of
this one big hole.

Q35 adds more to the problem with guest programmable pm_io_base,
we can't statically punch hole for it.

> 
> > > 
> > > > So replace static hole punching with dynamically consumed
> > > > resources in a child device on PCI0 bus. i.e generate
> > > > PNP0C02 device as a child of PCI0 bus at runtime and
> > > > consume GPE0, PCI/CPU hotplug IO resources in it instead
> > > > of punching holes in static PCI0._CRS.
> > > 
> > > It seems that we are being too exact with
> > > IO resources here.
> > > Can't we roughly reserve 0xae00 to 0xafff
> > > and be done with it?
> > that would be easiest way for this specific case if we could agree
> > for ranges on PIIX/Q35 machines.
> > 
> > But I also use it as excuse to introduce ASL like macros so that
> > building dynamic SSDT would be easier i.e. replace template patching
> > with single place where dynamic device is defined and its values
> > are set in simple and straightforward manner.
> 
> We don't need excuses really :)
> But the approach itself needs some work IMHO, in particular
> ASL like syntax by using macros and varargs is not something to strive
> for IMHO :)
> 
> Why don't we just pass GArrays around?
> 
> crs = build_alloc_crs();
> build_append_resource(crs, ....)

I don't like much macros myself, but compared to alternative
approach ^^^ (I've tried it) it's still harder to read/write
ASL constructs right.

I think that creating AML using ASL like flow and primitives
is better than building it using custom APIs, since it looks
like it was documented in spec.
Macros+vararg provide a necessary degree of flexibility to
create AML using ASL like flow and constructs.

> 
> 
> Or if you really want this, using array of GArray:
> 
> build_append_crs(ssdt,
> 		{
> 			build_alloc_resource(...),
> 			build_alloc_resource(...),
> 			build_alloc_resource(...),
> 			build_alloc_resource(...),
> 			NULL
> 		})
that would work if there was a way to specify arbitrary statements
inside embedded array (like in the last hunk of [9/9]).

> 
> 
> > > > Tested with Windows XPsp3, Vista, Windows Server 2003, 2008, 2012r2.
> > > > 
> > > > PS:
> > > > Series adds several ASL like macros to simplify
> > > > code for dynamic generation of AML structures.
> > > > 
> > > > Igor Mammedov (9):
> > > >   Revert "pc: Q35 DSDT: exclude CPU hotplug IO range from PCI bus
> > > >     resources"
> > > >   Revert "pc: PIIX DSDT: exclude CPU/PCI hotplug & GPE0 IO range from
> > > >     PCI bus resources"
> > > >   Partial revert "pc: ACPI: expose PRST IO range via _CRS"
> > > >   acpi: replace opencoded opcodes with defines
> > > >   acpi: add PNP0C02 to PCI0 bus
> > > >   acpi: consume GPE0 IO resources in PNP0C02 device
> > > >   acpi: consume CPU hotplug IO resource in PNP0C02 device
> > > >   pcihp: expose PCI hotplug MMIO base/length as properties of piix4pm
> > > >   acpi: consume PCIHP IO resource in PNP0C02 device
> > > > 
> > > >  hw/acpi/pcihp.c                   |   28 ++++++
> > > >  hw/acpi/piix4.c                   |    1 +
> > > >  hw/i386/acpi-build.c              |  177 ++++++++++++++++++++++++++++++++++--
> > > >  hw/i386/acpi-dsdt-cpu-hotplug.dsl |   11 ---
> > > >  hw/i386/acpi-dsdt-pci-crs.dsl     |   15 +++-
> > > >  hw/i386/acpi-dsdt.dsl             |   39 --------
> > > >  hw/i386/q35-acpi-dsdt.dsl         |   16 ----
> > > >  include/hw/acpi/pcihp.h           |    4 +
> > > >  8 files changed, 214 insertions(+), 77 deletions(-)
> > 
> > 
> > -- 
> > Regards,
> >   Igor

Re: [Qemu-devel] [RFC 0/9] generate dynamic _CRS for motherboard resources

[Michael S. Tsirkin]

On Tue, Feb 18, 2014 at 12:10:27PM +0100, Igor Mammedov wrote:
> On Mon, 17 Feb 2014 13:02:18 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Feb 17, 2014 at 11:33:23AM +0100, Igor Mammedov wrote:
> > > On Sun, 16 Feb 2014 17:53:45 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Fri, Feb 07, 2014 at 01:51:27PM +0100, Igor Mammedov wrote:
> > > > > Since introduction of PCIHP, it became problematic to
> > > > > punch hole in PCI0._CRS statically since PCI hotplug
> > > > > region size became runtime changeable.
> > > > 
> > > > What makes it runtime changeable?
> > > piix machine "acpi-pci-hotplug-with-bridge-support=on" effectively
> > > changes size of pcihp MMIO region
> > 
> > It adds 4 bytes. Let's just reserve a reasonably sized region,
> > like 512 bytes.
> Reserve where and for how log it will be enough?
> Eventually we will continue punching holes or increasing size of
> this one big hole.

I agree this might become necessary.  But why not keep it simple while
we can? The advantage of ACPI in qemu is we can fix it without
touching guest code. Let's use it.

So if there's just pm_io_base, we can just write
it in ASL and patch in that value.

> 
> Q35 adds more to the problem with guest programmable pm_io_base,
> we can't statically punch hole for it.

Aha. That's a good point.
But I still worry how we'll handle e.g. pvpanic.
Paolo had an idea to scan the memory range,
and punch holes for each section, making an
exception for regions owned by PCI devices.
I thought it would work originally too, but it
seems that e.g. VGA range is also owned by pci
devices sometimes.


> > 
> > > > 
> > > > > So replace static hole punching with dynamically consumed
> > > > > resources in a child device on PCI0 bus. i.e generate
> > > > > PNP0C02 device as a child of PCI0 bus at runtime and
> > > > > consume GPE0, PCI/CPU hotplug IO resources in it instead
> > > > > of punching holes in static PCI0._CRS.
> > > > 
> > > > It seems that we are being too exact with
> > > > IO resources here.
> > > > Can't we roughly reserve 0xae00 to 0xafff
> > > > and be done with it?
> > > that would be easiest way for this specific case if we could agree
> > > for ranges on PIIX/Q35 machines.
> > > 
> > > But I also use it as excuse to introduce ASL like macros so that
> > > building dynamic SSDT would be easier i.e. replace template patching
> > > with single place where dynamic device is defined and its values
> > > are set in simple and straightforward manner.
> > 
> > We don't need excuses really :)
> > But the approach itself needs some work IMHO, in particular
> > ASL like syntax by using macros and varargs is not something to strive
> > for IMHO :)
> > 
> > Why don't we just pass GArrays around?
> > 
> > crs = build_alloc_crs();
> > build_append_resource(crs, ....)
> 
> I don't like much macros myself, but compared to alternative
> approach ^^^ (I've tried it) it's still harder to read/write
> ASL constructs right.
> 
> I think that creating AML using ASL like flow and primitives
> is better than building it using custom APIs, since it looks
> like it was documented in spec.
> Macros+vararg provide a necessary degree of flexibility to
> create AML using ASL like flow and constructs.

Only you lose all type safety. When compiling ASL with IASL
at least it does some type checking.

> > 
> > 
> > Or if you really want this, using array of GArray:
> > 
> > build_append_crs(ssdt,
> > 		{
> > 			build_alloc_resource(...),
> > 			build_alloc_resource(...),
> > 			build_alloc_resource(...),
> > 			build_alloc_resource(...),
> > 			NULL
> > 		})
> that would work if there was a way to specify arbitrary statements
> inside embedded array (like in the last hunk of [9/9]).

In C we have functions for this :)
Last chunk:

-                    if (pm->pcihp_io_base) {
-                        ACPI_IO(RESBUF, Decode16,
-                                pm->pcihp_io_base, /* _MIN */
-                                pm->pcihp_io_base, /* _MAX */
-                                0x0,               /* _ALN */
-                                pm->pcihp_io_len); /* _LEN */
-                    }


+	build_io_resource_16(pm->pcihp_io_base, pm->pcihp_io_base,
			     0x0, pm->pcihp_io_len);


Which actually makes one think. Isn't _MAX wrong here?
I'd expect pm->pcihp_io_base + pm->pcihp_io_len - 1

I would also key this off pcihp_io_len.

> > 
> > 
> > > > > Tested with Windows XPsp3, Vista, Windows Server 2003, 2008, 2012r2.
> > > > > 
> > > > > PS:
> > > > > Series adds several ASL like macros to simplify
> > > > > code for dynamic generation of AML structures.
> > > > > 
> > > > > Igor Mammedov (9):
> > > > >   Revert "pc: Q35 DSDT: exclude CPU hotplug IO range from PCI bus
> > > > >     resources"
> > > > >   Revert "pc: PIIX DSDT: exclude CPU/PCI hotplug & GPE0 IO range from
> > > > >     PCI bus resources"
> > > > >   Partial revert "pc: ACPI: expose PRST IO range via _CRS"
> > > > >   acpi: replace opencoded opcodes with defines
> > > > >   acpi: add PNP0C02 to PCI0 bus
> > > > >   acpi: consume GPE0 IO resources in PNP0C02 device
> > > > >   acpi: consume CPU hotplug IO resource in PNP0C02 device
> > > > >   pcihp: expose PCI hotplug MMIO base/length as properties of piix4pm
> > > > >   acpi: consume PCIHP IO resource in PNP0C02 device
> > > > > 
> > > > >  hw/acpi/pcihp.c                   |   28 ++++++
> > > > >  hw/acpi/piix4.c                   |    1 +
> > > > >  hw/i386/acpi-build.c              |  177 ++++++++++++++++++++++++++++++++++--
> > > > >  hw/i386/acpi-dsdt-cpu-hotplug.dsl |   11 ---
> > > > >  hw/i386/acpi-dsdt-pci-crs.dsl     |   15 +++-
> > > > >  hw/i386/acpi-dsdt.dsl             |   39 --------
> > > > >  hw/i386/q35-acpi-dsdt.dsl         |   16 ----
> > > > >  include/hw/acpi/pcihp.h           |    4 +
> > > > >  8 files changed, 214 insertions(+), 77 deletions(-)
> > > 
> > > 
> > > -- 
> > > Regards,
> > >   Igor

Re: [Qemu-devel] [RFC 0/9] generate dynamic _CRS for motherboard resources

[Igor Mammedov]

On Tue, 18 Feb 2014 13:33:17 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Feb 18, 2014 at 12:10:27PM +0100, Igor Mammedov wrote:
> > On Mon, 17 Feb 2014 13:02:18 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Mon, Feb 17, 2014 at 11:33:23AM +0100, Igor Mammedov wrote:
> > > > On Sun, 16 Feb 2014 17:53:45 +0200
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > 
> > > > > On Fri, Feb 07, 2014 at 01:51:27PM +0100, Igor Mammedov wrote:
> > > > > > Since introduction of PCIHP, it became problematic to
> > > > > > punch hole in PCI0._CRS statically since PCI hotplug
> > > > > > region size became runtime changeable.
> > > > > 
> > > > > What makes it runtime changeable?
> > > > piix machine "acpi-pci-hotplug-with-bridge-support=on" effectively
> > > > changes size of pcihp MMIO region
> > > 
> > > It adds 4 bytes. Let's just reserve a reasonably sized region,
> > > like 512 bytes.
> > Reserve where and for how log it will be enough?
> > Eventually we will continue punching holes or increasing size of
> > this one big hole.
> 
> I agree this might become necessary.  But why not keep it simple while
> we can? The advantage of ACPI in qemu is we can fix it without
> touching guest code. Let's use it.
> 
> So if there's just pm_io_base, we can just write
> it in ASL and patch in that value.
> 
> > 
> > Q35 adds more to the problem with guest programmable pm_io_base,
> > we can't statically punch hole for it.
> 
> Aha. That's a good point.
> But I still worry how we'll handle e.g. pvpanic.
> Paolo had an idea to scan the memory range,
> and punch holes for each section, making an
> exception for regions owned by PCI devices.
> I thought it would work originally too, but it
> seems that e.g. VGA range is also owned by pci
> devices sometimes.
from ACPI pov, I think we need to punch holes or define
_CRS in respective consuming device explicitly, doing it
automatically looping through MemoryRegions going to
be tricky.

but for SeaBIOS to find out what ports are reserved
looping might be a right way, if it needs this information.

> 
> > > 
> > > > > 
> > > > > > So replace static hole punching with dynamically consumed
> > > > > > resources in a child device on PCI0 bus. i.e generate
> > > > > > PNP0C02 device as a child of PCI0 bus at runtime and
> > > > > > consume GPE0, PCI/CPU hotplug IO resources in it instead
> > > > > > of punching holes in static PCI0._CRS.
> > > > > 
> > > > > It seems that we are being too exact with
> > > > > IO resources here.
> > > > > Can't we roughly reserve 0xae00 to 0xafff
> > > > > and be done with it?
> > > > that would be easiest way for this specific case if we could agree
> > > > for ranges on PIIX/Q35 machines.
> > > > 
> > > > But I also use it as excuse to introduce ASL like macros so that
> > > > building dynamic SSDT would be easier i.e. replace template patching
> > > > with single place where dynamic device is defined and its values
> > > > are set in simple and straightforward manner.
> > > 
> > > We don't need excuses really :)
> > > But the approach itself needs some work IMHO, in particular
> > > ASL like syntax by using macros and varargs is not something to strive
> > > for IMHO :)
> > > 
> > > Why don't we just pass GArrays around?
> > > 
> > > crs = build_alloc_crs();
> > > build_append_resource(crs, ....)
> > 
> > I don't like much macros myself, but compared to alternative
> > approach ^^^ (I've tried it) it's still harder to read/write
> > ASL constructs right.
> > 
> > I think that creating AML using ASL like flow and primitives
> > is better than building it using custom APIs, since it looks
> > like it was documented in spec.
> > Macros+vararg provide a necessary degree of flexibility to
> > create AML using ASL like flow and constructs.
> 
> Only you lose all type safety. When compiling ASL with IASL
> at least it does some type checking.
short of embedded IASL solution, it's the same type safety as we
could provide with C, with extra checks inside primitives.
So that API user won't have to worry about it.

> 
> > > 
> > > 
> > > Or if you really want this, using array of GArray:
> > > 
> > > build_append_crs(ssdt,
> > > 		{
> > > 			build_alloc_resource(...),
> > > 			build_alloc_resource(...),
> > > 			build_alloc_resource(...),
> > > 			build_alloc_resource(...),
> > > 			NULL
> > > 		})
> > that would work if there was a way to specify arbitrary statements
> > inside embedded array (like in the last hunk of [9/9]).
> 
> In C we have functions for this :)
I don't say it's impossible to write using C functions to express the same
logic, but it would be harder to read/write code that way. For example below
snippet would look like:

GArray *return_array_if(bool, garray){
 if (bool == true) return garray;
 return empty_garray;
}

build_append_crs(ssdt,
 		{
 			build_alloc_resource(...),
                        ...,
                        return_array_if(have_pcihp,
                                        build_alloc_resource(...)
                        ),

and that will turn out in explosive growth of "return_array_if" like
functions to accommodate every tweak we would to do.
On contrary, macros with inline code make it simpler/easier
to write/read code, almost like embedded ASL.

> Last chunk:
> 
> -                    if (pm->pcihp_io_base) {
> -                        ACPI_IO(RESBUF, Decode16,
> -                                pm->pcihp_io_base, /* _MIN */
> -                                pm->pcihp_io_base, /* _MAX */
> -                                0x0,               /* _ALN */
> -                                pm->pcihp_io_len); /* _LEN */
> -                    }
> 
> 
> +	build_io_resource_16(pm->pcihp_io_base, pm->pcihp_io_base,
> 			     0x0, pm->pcihp_io_len);
That's not the same, Do you mean:

build_io_resource_16_IF(have_pcihp, ....);

and returning empty GArray if have_pcihp == false?

If it's the case than that's what I'm trying to avoid,
in favor of pretty well described in spec ASL like primitives
with some C snippets to tweak generated AML object content.

If we use C++11 lambda feature we could achieve almost
the same simple to use API as macro based, but I guess it's
not an option yet.

> 
> 
> Which actually makes one think. Isn't _MAX wrong here?
> I'd expect pm->pcihp_io_base + pm->pcihp_io_len - 1

nope,
"
19.5.62 IO (IO Resource Descriptor Macro)
...
AddressMax evaluates to a 16-bit integer that specifies the maximum acceptable STARTING address for
the I/O range. 
"

> 
> I would also key this off pcihp_io_len.
> 
> > > 
> > > 
> > > > > > Tested with Windows XPsp3, Vista, Windows Server 2003, 2008, 2012r2.
> > > > > > 
> > > > > > PS:
> > > > > > Series adds several ASL like macros to simplify
> > > > > > code for dynamic generation of AML structures.
> > > > > > 
> > > > > > Igor Mammedov (9):
> > > > > >   Revert "pc: Q35 DSDT: exclude CPU hotplug IO range from PCI bus
> > > > > >     resources"
> > > > > >   Revert "pc: PIIX DSDT: exclude CPU/PCI hotplug & GPE0 IO range from
> > > > > >     PCI bus resources"
> > > > > >   Partial revert "pc: ACPI: expose PRST IO range via _CRS"
> > > > > >   acpi: replace opencoded opcodes with defines
> > > > > >   acpi: add PNP0C02 to PCI0 bus
> > > > > >   acpi: consume GPE0 IO resources in PNP0C02 device
> > > > > >   acpi: consume CPU hotplug IO resource in PNP0C02 device
> > > > > >   pcihp: expose PCI hotplug MMIO base/length as properties of piix4pm
> > > > > >   acpi: consume PCIHP IO resource in PNP0C02 device
> > > > > > 
> > > > > >  hw/acpi/pcihp.c                   |   28 ++++++
> > > > > >  hw/acpi/piix4.c                   |    1 +
> > > > > >  hw/i386/acpi-build.c              |  177 ++++++++++++++++++++++++++++++++++--
> > > > > >  hw/i386/acpi-dsdt-cpu-hotplug.dsl |   11 ---
> > > > > >  hw/i386/acpi-dsdt-pci-crs.dsl     |   15 +++-
> > > > > >  hw/i386/acpi-dsdt.dsl             |   39 --------
> > > > > >  hw/i386/q35-acpi-dsdt.dsl         |   16 ----
> > > > > >  include/hw/acpi/pcihp.h           |    4 +
> > > > > >  8 files changed, 214 insertions(+), 77 deletions(-)
> > > > 
> > > > 
> > > > -- 
> > > > Regards,
> > > >   Igor

Re: [Qemu-devel] [RFC 0/9] generate dynamic _CRS for motherboard resources

[Gerd Hoffmann]

On Fr, 2014-02-07 at 13:51 +0100, Igor Mammedov wrote:
> Since introduction of PCIHP, it became problematic to
> punch hole in PCI0._CRS statically since PCI hotplug
> region size became runtime changeable.
> 
> So replace static hole punching with dynamically consumed
> resources in a child device on PCI0 bus. i.e generate
> PNP0C02 device as a child of PCI0 bus at runtime and
> consume GPE0, PCI/CPU hotplug IO resources in it instead
> of punching holes in static PCI0._CRS.

Nice.  Can you try to do that for the mmconf xbar in memory space too
while being at it?

cheers,
  Gerd

Re: [Qemu-devel] [RFC 0/9] generate dynamic _CRS for motherboard resources

[Igor Mammedov]

On Mon, 17 Feb 2014 09:29:20 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Fr, 2014-02-07 at 13:51 +0100, Igor Mammedov wrote:
> > Since introduction of PCIHP, it became problematic to
> > punch hole in PCI0._CRS statically since PCI hotplug
> > region size became runtime changeable.
> > 
> > So replace static hole punching with dynamically consumed
> > resources in a child device on PCI0 bus. i.e generate
> > PNP0C02 device as a child of PCI0 bus at runtime and
> > consume GPE0, PCI/CPU hotplug IO resources in it instead
> > of punching holes in static PCI0._CRS.
> 
> Nice.  Can you try to do that for the mmconf xbar in memory space too
> while being at it?
If I manage to convince mst in using macro approach.

PS:
Although his build_append_foo() functions have tremendously improved
dynamic AML generation (made it much less error-prone), the
current acpi-build.c becomes harder to read (just look at new pcihp
parts). So I'm arguing in favor of more simple/intuitive API
approach even if it uses macros (I'm really not fun of it,
but result is much clear/maintainable code).

> 
> cheers,
>   Gerd
> 
>