[Qemu-devel] [PATCH hack dontapply v2 0/7] Dynamic _CST generation

[Qemu-devel] [PATCH hack dontapply v2 0/7] Dynamic _CST generation

[Michael S. Tsirkin]

Now that basic support for guest CPU PM is upstream, I started looking
for making it migrateable.  Since a VM can be migrated between different
hosts, PM info needs to change each time with move the VM to a different
host.

This adds infrastructure - based on Load/Unload - to support exactly
that: QEMU generates AML (changing it on migration) and stores it in
reserved memory, OSPM loads _CST from there on demand.

This is a partially functional prototype.

What works:
    loading _CST dynamically and reporting it to OSPM

What doesn't:
    detecting host configuration and generating correct _CST package
    notifying guest about the change to trigger _CST re-evaluation
    disabling mwait/halt exists as appropriate

Michael S. Tsirkin (6):
  acpi: aml: add aml_register()
  acpi: generalize aml_package / aml_varpackage
  acpi: aml_load/aml_unload
  acpi: export acpi_checksum
  acpi: init header without linking
  acpi: aml generation for _CST
  pc: HACK: acpi: tie in _CST object to Processor

 include/hw/acpi/acpi.h      |   2 +
 include/hw/acpi/aml-build.h |  14 ++-
 include/hw/acpi/cst.h       |   8 ++
 include/hw/i386/pc.h        |   5 ++
 hw/acpi/aml-build.c         |  81 ++++++++++++++---
 hw/acpi/core.c              |   2 +-
 hw/acpi/cpu.c               |   5 ++
 hw/acpi/cpu_hotplug.c       |  11 +--
 hw/acpi/cst.c               | 173 ++++++++++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c    |   2 +-
 hw/i386/acpi-build.c        |  10 ++-
 hw/acpi/Makefile.objs       |   2 +-
 12 files changed, 290 insertions(+), 25 deletions(-)
 create mode 100644 include/hw/acpi/cst.h
 create mode 100644 hw/acpi/cst.c

-- 
MST

[Qemu-devel] [PATCH hack dontapply v2 1/7] acpi: aml: add aml_register()

[Michael S. Tsirkin]

Based on a patch by Igor Mammedov.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/acpi/aml-build.h |  5 +++++
 hw/acpi/aml-build.c         | 21 +++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 6c36903c0a..10c7946028 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -346,6 +346,11 @@ Aml *aml_qword_memory(AmlDecode dec, AmlMinFixed min_fixed,
                       uint64_t len);
 Aml *aml_dma(AmlDmaType typ, AmlDmaBusMaster bm, AmlTransferSize sz,
              uint8_t channel);
+Aml *aml_register(AmlAddressSpace as,
+                  uint8_t bit_width,
+                  uint8_t bit_offset,
+                  uint64_t address,
+                  uint8_t access_size);
 Aml *aml_sleep(uint64_t msec);
 
 /* Block AML object primitives */
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 1e43cd736d..def62b3112 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -874,6 +874,27 @@ Aml *aml_irq_no_flags(uint8_t irq)
     return var;
 }
 
+/*
+ * ACPI: 2.0: 16.2.4.16 ASL Macro for Generic Register Descriptor
+ *
+ * access_size comes from:
+ *     ACPI 3.0: 17.5.98 Register (Generic Register Resource Descriptor Macro)
+ */
+Aml *aml_register(AmlAddressSpace as,
+                  uint8_t bit_width,
+                  uint8_t bit_offset,
+                  uint64_t address,
+                  uint8_t access_size)
+{
+    Aml *var = aml_alloc();
+
+    build_append_byte(var->buf, 0x82);        /* Generic Register Descriptor */
+    build_append_byte(var->buf, 0x0C);        /* Length, bits[7:0] */
+    build_append_byte(var->buf, 0x0);         /* Length, bits[15:8] */
+    build_append_gas(var->buf, as, bit_width, bit_offset, access_size, address);
+    return var;
+}
+
 /* ACPI 1.0b: 16.2.5.4 Type 2 Opcodes Encoding: DefLNot */
 Aml *aml_lnot(Aml *arg)
 {
-- 
MST

[Qemu-devel] [PATCH hack dontapply v2 2/7] acpi: generalize aml_package / aml_varpackage

[Michael S. Tsirkin]

VarPackage can accept an expression evaluating to int, not just an int.
Change the API to make it more generic.
Further, rather than have users call the correct API depending on
value passed, use either PackageOp or VarPackageOp automatically.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/acpi/aml-build.h |  4 ++--
 hw/acpi/aml-build.c         | 19 +++++++++++++++----
 hw/acpi/cpu_hotplug.c       | 11 ++---------
 hw/arm/virt-acpi-build.c    |  2 +-
 4 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 10c7946028..7cf2cf64bf 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -360,7 +360,7 @@ Aml *aml_method(const char *name, int arg_count, AmlSerializeFlag sflag);
 Aml *aml_if(Aml *predicate);
 Aml *aml_else(void);
 Aml *aml_while(Aml *predicate);
-Aml *aml_package(uint8_t num_elements);
+Aml *aml_package(uint64_t num_elements);
 Aml *aml_buffer(int buffer_size, uint8_t *byte_list);
 Aml *aml_resource_template(void);
 Aml *aml_field(const char *name, AmlAccessType type, AmlLockRule lock,
@@ -373,7 +373,7 @@ Aml *aml_create_field(Aml *srcbuf, Aml *bit_index, Aml *num_bits,
                       const char *name);
 Aml *aml_create_dword_field(Aml *srcbuf, Aml *index, const char *name);
 Aml *aml_create_qword_field(Aml *srcbuf, Aml *index, const char *name);
-Aml *aml_varpackage(uint32_t num_elements);
+Aml *aml_varpackage(Aml *num_elements);
 Aml *aml_touuid(const char *uuid);
 Aml *aml_unicode(const char *str);
 Aml *aml_refof(Aml *arg);
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index def62b3112..7165f19585 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1016,9 +1016,20 @@ Aml *aml_buffer(int buffer_size, uint8_t *byte_list)
 }
 
 /* ACPI 1.0b: 16.2.5.4 Type 2 Opcodes Encoding: DefPackage */
-Aml *aml_package(uint8_t num_elements)
+/* Note: The ability to create variable-sized packages was first
+ * introduced in ACPI 2.0. ACPI 1.0 only allowed fixed-size packages
+ * with up to 255 elements. Windows guests up to win2k8 fail when
+ * VarPackageOp is used.
+ */
+Aml *aml_package(uint64_t num_elements)
 {
-    Aml *var = aml_bundle(0x12 /* PackageOp */, AML_PACKAGE);
+    Aml *var;
+
+    if (num_elements > 0xFF) {
+        return aml_varpackage(aml_int(num_elements));
+    }
+
+    var = aml_bundle(0x12 /* PackageOp */, AML_PACKAGE);
     build_append_byte(var->buf, num_elements);
     return var;
 }
@@ -1136,10 +1147,10 @@ Aml *aml_local(int num)
 }
 
 /* ACPI 2.0a: 17.2.2 Data Objects Encoding: DefVarPackage */
-Aml *aml_varpackage(uint32_t num_elements)
+Aml *aml_varpackage(Aml *num_elements)
 {
     Aml *var = aml_bundle(0x13 /* VarPackageOp */, AML_PACKAGE);
-    build_append_int(var->buf, num_elements);
+    aml_append(var, num_elements);
     return var;
 }
 
diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
index 5243918125..f8246fca6f 100644
--- a/hw/acpi/cpu_hotplug.c
+++ b/hw/acpi/cpu_hotplug.c
@@ -309,15 +309,8 @@ void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
     }
     aml_append(sb_scope, method);
 
-    /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })"
-     *
-     * Note: The ability to create variable-sized packages was first
-     * introduced in ACPI 2.0. ACPI 1.0 only allowed fixed-size packages
-     * ith up to 255 elements. Windows guests up to win2k8 fail when
-     * VarPackageOp is used.
-     */
-    pkg = pcms->apic_id_limit <= 255 ? aml_package(pcms->apic_id_limit) :
-                                       aml_varpackage(pcms->apic_id_limit);
+    /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */
+    pkg = aml_package(pcms->apic_id_limit);
 
     for (i = 0, apic_idx = 0; i < apic_ids->len; i++) {
         int apic_id = apic_ids->cpus[i].arch_id;
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 6ea47e2588..2adb1de378 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -174,7 +174,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
     aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
 
     /* Declare the PCI Routing Table. */
-    Aml *rt_pkg = aml_varpackage(nr_pcie_buses * PCI_NUM_PINS);
+    Aml *rt_pkg = aml_package(nr_pcie_buses * PCI_NUM_PINS);
     for (bus_no = 0; bus_no < nr_pcie_buses; bus_no++) {
         for (i = 0; i < PCI_NUM_PINS; i++) {
             int gsi = (i + bus_no) % PCI_NUM_PINS;
-- 
MST

[Qemu-devel] [PATCH hack dontapply v2 3/7] acpi: aml_load/aml_unload

[Michael S. Tsirkin]

Dynamic loading/unloading of tables.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/acpi/aml-build.h |  2 ++
 hw/acpi/aml-build.c         | 21 +++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 7cf2cf64bf..f764f71e25 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -381,6 +381,8 @@ Aml *aml_derefof(Aml *arg);
 Aml *aml_sizeof(Aml *arg);
 Aml *aml_concatenate(Aml *source1, Aml *source2, Aml *target);
 Aml *aml_object_type(Aml *object);
+Aml *aml_load(const char *name, Aml *ddbhandle);
+Aml *aml_unload(Aml *ddbhandle);
 
 void build_append_int_noprefix(GArray *table, uint64_t value, int size);
 void
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 7165f19585..ce691c1618 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1552,6 +1552,27 @@ Aml *aml_object_type(Aml *object)
     return var;
 }
 
+/* ACPI 2.0: 17.2.4.3 Type 1 Opcodes Encoding: DefLoad */
+Aml *aml_load(const char *name, Aml *ddbhandle)
+{
+    Aml *var = aml_alloc();
+    build_append_byte(var->buf, 0x5B); /* ExtOpPrefix */
+    build_append_byte(var->buf, 0x20); /* LoadOp */
+    aml_append(var, aml_name("%s", name));
+    aml_append(var, ddbhandle);
+    return var;
+}
+
+/* ACPI 2.0: 17.2.4.3 Type 1 Opcodes Encoding: DefUnload */
+Aml *aml_unload(Aml *ddbhandle)
+{
+    Aml *var = aml_alloc();
+    build_append_byte(var->buf, 0x5B); /* ExtOpPrefix */
+    build_append_byte(var->buf, 0x2A); /* UnloadOp */
+    aml_append(var, ddbhandle);
+    return var;
+}
+
 void
 build_header(BIOSLinker *linker, GArray *table_data,
              AcpiTableHeader *h, const char *sig, int len, uint8_t rev,
-- 
MST

[Qemu-devel] [PATCH hack dontapply v2 4/7] acpi: export acpi_checksum

[Michael S. Tsirkin]

For most tables we supply to guests checksum is
calculated by the bios at load time.

However, when table needs to be changed later dynamically,
QEMU has to calculate the checksum.

Export acpi_checksum so ACPI generation code can re-use it.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/acpi/acpi.h | 2 ++
 hw/acpi/core.c         | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
index c20ace0d0b..957a064d58 100644
--- a/include/hw/acpi/acpi.h
+++ b/include/hw/acpi/acpi.h
@@ -148,6 +148,8 @@ struct ACPIREGS {
     Notifier wakeup;
 };
 
+int acpi_checksum(const uint8_t *data, int len);
+
 /* PM_TMR */
 void acpi_pm_tmr_update(ACPIREGS *ar, bool enable);
 void acpi_pm_tmr_calc_overflow_time(ACPIREGS *ar);
diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index b8d39012cd..ae24d104d4 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -72,7 +72,7 @@ static void acpi_register_config(void)
 
 opts_init(acpi_register_config);
 
-static int acpi_checksum(const uint8_t *data, int len)
+int acpi_checksum(const uint8_t *data, int len)
 {
     int sum, i;
     sum = 0;
-- 
MST

[Qemu-devel] [PATCH hack dontapply v2 6/7] acpi: aml generation for _CST

[Michael S. Tsirkin]

This adds a method called CCST under CPUS.
Each CPU will add _CST calling in turn CCST from there.

As _CST needs to change across migration, we use
dynamic loading of tables as follows:

- a special scratch buffer, 4K in size is requested from bios
  through the loader.
- each time CCST executes, buffer address is passed to QEMU
  which will write an SSDT table with the _CST package into the buffer.
- table is then loaded and a package named \\_SB.CPUS.CSTL is loaded from there.
- table is unloaded, package is then returned to caller

In this way, QEMU can change the package across migration.
It will then notify the OSPM which will re-evaluate
_CST.

In this proof of concept patch, package generation itself is
still a stub, and change notifications are missing.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/acpi/cst.h |   8 ++
 hw/acpi/cst.c         | 173 ++++++++++++++++++++++++++++++++++++++++++
 hw/acpi/Makefile.objs |   2 +-
 3 files changed, 182 insertions(+), 1 deletion(-)
 create mode 100644 include/hw/acpi/cst.h
 create mode 100644 hw/acpi/cst.c

diff --git a/include/hw/acpi/cst.h b/include/hw/acpi/cst.h
new file mode 100644
index 0000000000..2e40e87881
--- /dev/null
+++ b/include/hw/acpi/cst.h
@@ -0,0 +1,8 @@
+#ifndef HW_ACPI_CST_H
+#define HW_ACPI_CST_H
+
+#include "hw/acpi/bios-linker-loader.h"
+
+void cst_build_acpi(GArray *table_data, BIOSLinker *linker, uint16_t ioport);
+void cst_register(FWCfgState *s, uint16_t ioport);
+#endif
diff --git a/hw/acpi/cst.c b/hw/acpi/cst.c
new file mode 100644
index 0000000000..20dd80aef8
--- /dev/null
+++ b/hw/acpi/cst.c
@@ -0,0 +1,173 @@
+#include "qemu/osdep.h"
+#include "exec/address-spaces.h"
+#include "hw/acpi/aml-build.h"
+#include "hw/acpi/cst.h"
+#include "hw/acpi/acpi.h"
+#include "hw/nvram/fw_cfg.h"
+
+#define ACPI_SCRATCH_BUFFER_NAME "etc/scratch"
+
+/* Hack! Incomplete! */
+static Aml *build_cst_package(void)
+{
+    int i;
+    Aml *crs;
+    Aml *pkg;
+    int cs_num = 3;
+
+    pkg = aml_package(cs_num + 1); /* # of ACPI Cx states + state count */
+    aml_append(pkg, aml_int(cs_num)); /* # of ACPI Cx states */
+
+    for (i = 0; i < cs_num; i++) {
+        Aml *cstate = aml_package(4);
+
+        crs = aml_resource_template();
+        aml_append(crs, aml_register(AML_AS_SYSTEM_IO,
+            0x8,
+            0x0,
+            0x100,
+            0x1));
+        aml_append(cstate, crs);
+        aml_append(cstate, aml_int(i + 1)); /* Cx ACPI state */
+        aml_append(cstate, aml_int((i + 1) * 10)); /* Latency */
+        aml_append(cstate, aml_int(cs_num - i - 1));/* Power */
+        aml_append(pkg, cstate);
+    }
+
+    return pkg;
+}
+
+static GArray *cst_scratch;
+
+/*
+ * Add an SSDT with a dynamic method named CCST. The method uses the specified
+ * ioport to load a table from QEMU, then returns an object named CSTL from
+ * it.
+ * Everything is scoped under \\_SB.CPUS.CSTP.
+ */
+void cst_build_acpi(GArray *table_data, BIOSLinker *linker, uint16_t ioport)
+{
+    Aml *ssdt, *scope, *field, *method;
+    uint32_t cstp_offset;
+
+    /* Put this in a separate SSDT table */
+    ssdt = init_aml_allocator();
+
+    /* Reserve space for header */
+    acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));
+
+    cstp_offset = table_data->len +
+        build_append_named_dword(ssdt->buf, "\\_SB.CPUS.CSTP");
+    scope = aml_scope("\\_SB.CPUS");
+    {
+        /* buffer in reserved memory to load the table from */
+        aml_append(scope, aml_operation_region("CSTB", AML_SYSTEM_MEMORY,
+                                               aml_name("\\_SB.CPUS.CSTP"),
+                                               4096));
+        /* write address here to update the table in memory */
+        aml_append(scope, aml_operation_region("CSTR", AML_SYSTEM_IO,
+                                               aml_int(ioport),
+                                               4));
+        field = aml_field("CSTR", AML_DWORD_ACC, AML_LOCK,
+                          AML_WRITE_AS_ZEROS);
+        {
+            aml_append(field, aml_named_field("CSTU", 32));
+        }
+        aml_append(scope, field);
+        method = aml_method("CCST", 0, AML_SERIALIZED);
+        {
+            Aml *ddbhandle = aml_local(0);
+            Aml *cst = aml_local(1);
+            /* Write buffer address to update table in memory. */
+            aml_append(method, aml_store(aml_name("CSTP"), aml_name("CSTU")));
+            aml_append(method, aml_load("CSTB", ddbhandle));
+            aml_append(method, aml_store(aml_name("CSTL"), cst));
+            aml_append(method, aml_unload(ddbhandle));
+            aml_append(method, aml_return(cst));
+        }
+        aml_append(scope, method);
+    }
+    aml_append(ssdt, scope);
+    g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
+
+    /* Why page boundary? no special reason right now but seems like
+     * a good idea for future extensions.      
+    */
+    bios_linker_loader_alloc(linker, ACPI_SCRATCH_BUFFER_NAME, cst_scratch,
+                             4096, false /* page boundary, high memory */);
+    /* Patch address of allocated memory into the AML so OSPM can retrieve
+     * and read it.
+     */
+    bios_linker_loader_add_pointer(linker,
+        ACPI_BUILD_TABLE_FILE, cstp_offset, sizeof(uint32_t),
+        ACPI_SCRATCH_BUFFER_NAME, 0);
+
+    //table_data->data[cstp_offset] = 0x8; /* hack */
+
+    build_header(linker, table_data,
+        (void *)(table_data->data + table_data->len - ssdt->buf->len),
+        "SSDT", ssdt->buf->len, 1, NULL, "CSTSSDT");
+
+    free_aml_allocator();
+}
+
+static GArray *cst_ssdt;
+
+static void cst_ssdt_setup(void)
+{
+    AcpiTableHeader *dyn_ssdt_hdr;
+    Aml *dyn_ssdt = init_aml_allocator();
+
+    /* Reserve space for header */
+    acpi_data_push(dyn_ssdt->buf, sizeof(AcpiTableHeader));
+    aml_append(dyn_ssdt, aml_name_decl("\\_SB.CPUS.CSTL", build_cst_package()));
+
+    dyn_ssdt_hdr = (AcpiTableHeader *)dyn_ssdt->buf->data;
+
+    acpi_init_header(dyn_ssdt_hdr, "SSDT", dyn_ssdt->buf->len, 1, NULL, "DYNSSDT");
+
+    dyn_ssdt_hdr->checksum = acpi_checksum((uint8_t *)dyn_ssdt_hdr,
+                                           dyn_ssdt->buf->len);
+
+    /* dyn_ssdt->buf will be freed. copy to cst_ssdt */
+    cst_ssdt = g_array_new(false, true, 1);
+    g_array_append_vals(cst_ssdt, dyn_ssdt->buf->data, dyn_ssdt->buf->len);
+
+    free_aml_allocator();
+}
+
+/* Update CST in system memory */
+static void cst_ioport_write(void *opaque, hwaddr addr, uint64_t data, unsigned size)
+{
+    assert(cst_ssdt);
+
+    cpu_physical_memory_write(data, cst_ssdt->data, cst_ssdt->len);
+}
+
+static const MemoryRegionOps cst_ops = {
+    .write = cst_ioport_write,
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static MemoryRegion cst_mr;
+
+void cst_register(FWCfgState *s, uint16_t ioport)
+{
+    cst_ssdt_setup();
+
+    /* Allocate guest scratch memory for the table */
+    cst_scratch = g_array_new(false, true, 1);
+    acpi_data_push(cst_scratch, 4096);
+    fw_cfg_add_file(s, ACPI_SCRATCH_BUFFER_NAME, cst_scratch->data,
+                    cst_scratch->len);
+
+    /* setup io to trigger updates */
+    memory_region_init_io(&cst_mr, NULL, &cst_ops, NULL, "cst-update-request", 4);
+    memory_region_add_subregion(get_system_io(), ioport, &cst_mr);
+}
+
+/* TODO: API to notify guest of changes */
diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
index 11c35bcb44..0119367455 100644
--- a/hw/acpi/Makefile.objs
+++ b/hw/acpi/Makefile.objs
@@ -1,5 +1,5 @@
 ifeq ($(CONFIG_ACPI),y)
-common-obj-$(CONFIG_ACPI_X86) += core.o piix4.o pcihp.o
+common-obj-$(CONFIG_ACPI_X86) += core.o piix4.o pcihp.o cst.o
 common-obj-$(CONFIG_ACPI_X86_ICH) += ich9.o tco.o
 common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu_hotplug.o
 common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o
-- 
MST

Re: [Qemu-devel] [PATCH hack dontapply v2 6/7] acpi: aml generation for _CST

[Igor Mammedov]

On Tue, 10 Jul 2018 03:01:33 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> This adds a method called CCST under CPUS.
> Each CPU will add _CST calling in turn CCST from there.
> 
> As _CST needs to change across migration, we use
> dynamic loading of tables as follows:
> 
> - a special scratch buffer, 4K in size is requested from bios
>   through the loader.
> - each time CCST executes, buffer address is passed to QEMU
>   which will write an SSDT table with the _CST package into the buffer.
> - table is then loaded and a package named \\_SB.CPUS.CSTL is loaded from there.
> - table is unloaded, package is then returned to caller
> 
> In this way, QEMU can change the package across migration.
> It will then notify the OSPM which will re-evaluate
> _CST.
> 
> In this proof of concept patch, package generation itself is
> still a stub, and change notifications are missing.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/hw/acpi/cst.h |   8 ++
>  hw/acpi/cst.c         | 173 ++++++++++++++++++++++++++++++++++++++++++
>  hw/acpi/Makefile.objs |   2 +-
>  3 files changed, 182 insertions(+), 1 deletion(-)
>  create mode 100644 include/hw/acpi/cst.h
>  create mode 100644 hw/acpi/cst.c
> 
> diff --git a/include/hw/acpi/cst.h b/include/hw/acpi/cst.h
> new file mode 100644
> index 0000000000..2e40e87881
> --- /dev/null
> +++ b/include/hw/acpi/cst.h
> @@ -0,0 +1,8 @@
> +#ifndef HW_ACPI_CST_H
> +#define HW_ACPI_CST_H
> +
> +#include "hw/acpi/bios-linker-loader.h"
> +
> +void cst_build_acpi(GArray *table_data, BIOSLinker *linker, uint16_t ioport);
> +void cst_register(FWCfgState *s, uint16_t ioport);
> +#endif
> diff --git a/hw/acpi/cst.c b/hw/acpi/cst.c
> new file mode 100644
> index 0000000000..20dd80aef8
> --- /dev/null
> +++ b/hw/acpi/cst.c
> @@ -0,0 +1,173 @@
> +#include "qemu/osdep.h"
> +#include "exec/address-spaces.h"
> +#include "hw/acpi/aml-build.h"
> +#include "hw/acpi/cst.h"
> +#include "hw/acpi/acpi.h"
> +#include "hw/nvram/fw_cfg.h"
> +
> +#define ACPI_SCRATCH_BUFFER_NAME "etc/scratch"
> +
> +/* Hack! Incomplete! */
> +static Aml *build_cst_package(void)
> +{
> +    int i;
> +    Aml *crs;
> +    Aml *pkg;
> +    int cs_num = 3;
> +
> +    pkg = aml_package(cs_num + 1); /* # of ACPI Cx states + state count */
> +    aml_append(pkg, aml_int(cs_num)); /* # of ACPI Cx states */
> +
> +    for (i = 0; i < cs_num; i++) {
> +        Aml *cstate = aml_package(4);
> +
> +        crs = aml_resource_template();
> +        aml_append(crs, aml_register(AML_AS_SYSTEM_IO,
> +            0x8,
> +            0x0,
> +            0x100,
> +            0x1));
> +        aml_append(cstate, crs);
> +        aml_append(cstate, aml_int(i + 1)); /* Cx ACPI state */
> +        aml_append(cstate, aml_int((i + 1) * 10)); /* Latency */
> +        aml_append(cstate, aml_int(cs_num - i - 1));/* Power */
> +        aml_append(pkg, cstate);
> +    }
> +
> +    return pkg;
> +}
> +
> +static GArray *cst_scratch;
> +
> +/*
> + * Add an SSDT with a dynamic method named CCST. The method uses the specified
> + * ioport to load a table from QEMU, then returns an object named CSTL from
> + * it.
> + * Everything is scoped under \\_SB.CPUS.CSTP.
> + */
> +void cst_build_acpi(GArray *table_data, BIOSLinker *linker, uint16_t ioport)
> +{
> +    Aml *ssdt, *scope, *field, *method;
> +    uint32_t cstp_offset;
> +
> +    /* Put this in a separate SSDT table */
> +    ssdt = init_aml_allocator();
> +
> +    /* Reserve space for header */
> +    acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));
> +
> +    cstp_offset = table_data->len +
> +        build_append_named_dword(ssdt->buf, "\\_SB.CPUS.CSTP");
> +    scope = aml_scope("\\_SB.CPUS");
> +    {
> +        /* buffer in reserved memory to load the table from */
> +        aml_append(scope, aml_operation_region("CSTB", AML_SYSTEM_MEMORY,
> +                                               aml_name("\\_SB.CPUS.CSTP"),
> +                                               4096));
> +        /* write address here to update the table in memory */
> +        aml_append(scope, aml_operation_region("CSTR", AML_SYSTEM_IO,
> +                                               aml_int(ioport),
> +                                               4));
> +        field = aml_field("CSTR", AML_DWORD_ACC, AML_LOCK,
> +                          AML_WRITE_AS_ZEROS);
> +        {
> +            aml_append(field, aml_named_field("CSTU", 32));
> +        }
> +        aml_append(scope, field);
> +        method = aml_method("CCST", 0, AML_SERIALIZED);
> +        {
> +            Aml *ddbhandle = aml_local(0);
> +            Aml *cst = aml_local(1);
> +            /* Write buffer address to update table in memory. */
> +            aml_append(method, aml_store(aml_name("CSTP"), aml_name("CSTU")));
> +            aml_append(method, aml_load("CSTB", ddbhandle));
> +            aml_append(method, aml_store(aml_name("CSTL"), cst));
> +            aml_append(method, aml_unload(ddbhandle));
> +            aml_append(method, aml_return(cst));
> +        }
> +        aml_append(scope, method);
> +    }
> +    aml_append(ssdt, scope);
> +    g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
> +
> +    /* Why page boundary? no special reason right now but seems like
> +     * a good idea for future extensions.      
> +    */
> +    bios_linker_loader_alloc(linker, ACPI_SCRATCH_BUFFER_NAME, cst_scratch,
> +                             4096, false /* page boundary, high memory */);
> +    /* Patch address of allocated memory into the AML so OSPM can retrieve
> +     * and read it.
> +     */
> +    bios_linker_loader_add_pointer(linker,
> +        ACPI_BUILD_TABLE_FILE, cstp_offset, sizeof(uint32_t),
> +        ACPI_SCRATCH_BUFFER_NAME, 0);
> +
> +    //table_data->data[cstp_offset] = 0x8; /* hack */
> +
> +    build_header(linker, table_data,
> +        (void *)(table_data->data + table_data->len - ssdt->buf->len),
> +        "SSDT", ssdt->buf->len, 1, NULL, "CSTSSDT");
> +
> +    free_aml_allocator();
> +}
> +
> +static GArray *cst_ssdt;
> +
> +static void cst_ssdt_setup(void)
> +{
> +    AcpiTableHeader *dyn_ssdt_hdr;
> +    Aml *dyn_ssdt = init_aml_allocator();
> +
> +    /* Reserve space for header */
> +    acpi_data_push(dyn_ssdt->buf, sizeof(AcpiTableHeader));
> +    aml_append(dyn_ssdt, aml_name_decl("\\_SB.CPUS.CSTL", build_cst_package()));
> +
> +    dyn_ssdt_hdr = (AcpiTableHeader *)dyn_ssdt->buf->data;
> +
> +    acpi_init_header(dyn_ssdt_hdr, "SSDT", dyn_ssdt->buf->len, 1, NULL, "DYNSSDT");
> +
> +    dyn_ssdt_hdr->checksum = acpi_checksum((uint8_t *)dyn_ssdt_hdr,
> +                                           dyn_ssdt->buf->len);
> +
> +    /* dyn_ssdt->buf will be freed. copy to cst_ssdt */
> +    cst_ssdt = g_array_new(false, true, 1);
> +    g_array_append_vals(cst_ssdt, dyn_ssdt->buf->data, dyn_ssdt->buf->len);
> +
> +    free_aml_allocator();
> +}
> +
> +/* Update CST in system memory */
> +static void cst_ioport_write(void *opaque, hwaddr addr, uint64_t data, unsigned size)
> +{
> +    assert(cst_ssdt);
> +
> +    cpu_physical_memory_write(data, cst_ssdt->data, cst_ssdt->len);
> +}
> +
> +static const MemoryRegionOps cst_ops = {
> +    .write = cst_ioport_write,
> +    .impl = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static MemoryRegion cst_mr;
> +
> +void cst_register(FWCfgState *s, uint16_t ioport)
> +{
> +    cst_ssdt_setup();
> +
> +    /* Allocate guest scratch memory for the table */
> +    cst_scratch = g_array_new(false, true, 1);
> +    acpi_data_push(cst_scratch, 4096);
> +    fw_cfg_add_file(s, ACPI_SCRATCH_BUFFER_NAME, cst_scratch->data,
> +                    cst_scratch->len);
> +
> +    /* setup io to trigger updates */
> +    memory_region_init_io(&cst_mr, NULL, &cst_ops, NULL, "cst-update-request", 4);
> +    memory_region_add_subregion(get_system_io(), ioport, &cst_mr);
it eats yet another IO port and a 4K page just for CST.
I'd prefer to extend docs/specs/acpi_cpu_hotplug.txt protocol
to support passing _CST values via existing registers instead.


> +}
> +
> +/* TODO: API to notify guest of changes */
> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> index 11c35bcb44..0119367455 100644
> --- a/hw/acpi/Makefile.objs
> +++ b/hw/acpi/Makefile.objs
> @@ -1,5 +1,5 @@
>  ifeq ($(CONFIG_ACPI),y)
> -common-obj-$(CONFIG_ACPI_X86) += core.o piix4.o pcihp.o
> +common-obj-$(CONFIG_ACPI_X86) += core.o piix4.o pcihp.o cst.o
>  common-obj-$(CONFIG_ACPI_X86_ICH) += ich9.o tco.o
>  common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu_hotplug.o
>  common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o

Re: [Qemu-devel] [PATCH hack dontapply v2 6/7] acpi: aml generation for _CST

[Michael S. Tsirkin]

On Wed, Jul 25, 2018 at 02:42:05PM +0200, Igor Mammedov wrote:
> On Tue, 10 Jul 2018 03:01:33 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > This adds a method called CCST under CPUS.
> > Each CPU will add _CST calling in turn CCST from there.
> > 
> > As _CST needs to change across migration, we use
> > dynamic loading of tables as follows:
> > 
> > - a special scratch buffer, 4K in size is requested from bios
> >   through the loader.
> > - each time CCST executes, buffer address is passed to QEMU
> >   which will write an SSDT table with the _CST package into the buffer.
> > - table is then loaded and a package named \\_SB.CPUS.CSTL is loaded from there.
> > - table is unloaded, package is then returned to caller
> > 
> > In this way, QEMU can change the package across migration.
> > It will then notify the OSPM which will re-evaluate
> > _CST.
> > 
> > In this proof of concept patch, package generation itself is
> > still a stub, and change notifications are missing.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  include/hw/acpi/cst.h |   8 ++
> >  hw/acpi/cst.c         | 173 ++++++++++++++++++++++++++++++++++++++++++
> >  hw/acpi/Makefile.objs |   2 +-
> >  3 files changed, 182 insertions(+), 1 deletion(-)
> >  create mode 100644 include/hw/acpi/cst.h
> >  create mode 100644 hw/acpi/cst.c
> > 
> > diff --git a/include/hw/acpi/cst.h b/include/hw/acpi/cst.h
> > new file mode 100644
> > index 0000000000..2e40e87881
> > --- /dev/null
> > +++ b/include/hw/acpi/cst.h
> > @@ -0,0 +1,8 @@
> > +#ifndef HW_ACPI_CST_H
> > +#define HW_ACPI_CST_H
> > +
> > +#include "hw/acpi/bios-linker-loader.h"
> > +
> > +void cst_build_acpi(GArray *table_data, BIOSLinker *linker, uint16_t ioport);
> > +void cst_register(FWCfgState *s, uint16_t ioport);
> > +#endif
> > diff --git a/hw/acpi/cst.c b/hw/acpi/cst.c
> > new file mode 100644
> > index 0000000000..20dd80aef8
> > --- /dev/null
> > +++ b/hw/acpi/cst.c
> > @@ -0,0 +1,173 @@
> > +#include "qemu/osdep.h"
> > +#include "exec/address-spaces.h"
> > +#include "hw/acpi/aml-build.h"
> > +#include "hw/acpi/cst.h"
> > +#include "hw/acpi/acpi.h"
> > +#include "hw/nvram/fw_cfg.h"
> > +
> > +#define ACPI_SCRATCH_BUFFER_NAME "etc/scratch"
> > +
> > +/* Hack! Incomplete! */
> > +static Aml *build_cst_package(void)
> > +{
> > +    int i;
> > +    Aml *crs;
> > +    Aml *pkg;
> > +    int cs_num = 3;
> > +
> > +    pkg = aml_package(cs_num + 1); /* # of ACPI Cx states + state count */
> > +    aml_append(pkg, aml_int(cs_num)); /* # of ACPI Cx states */
> > +
> > +    for (i = 0; i < cs_num; i++) {
> > +        Aml *cstate = aml_package(4);
> > +
> > +        crs = aml_resource_template();
> > +        aml_append(crs, aml_register(AML_AS_SYSTEM_IO,
> > +            0x8,
> > +            0x0,
> > +            0x100,
> > +            0x1));
> > +        aml_append(cstate, crs);
> > +        aml_append(cstate, aml_int(i + 1)); /* Cx ACPI state */
> > +        aml_append(cstate, aml_int((i + 1) * 10)); /* Latency */
> > +        aml_append(cstate, aml_int(cs_num - i - 1));/* Power */
> > +        aml_append(pkg, cstate);
> > +    }
> > +
> > +    return pkg;
> > +}
> > +
> > +static GArray *cst_scratch;
> > +
> > +/*
> > + * Add an SSDT with a dynamic method named CCST. The method uses the specified
> > + * ioport to load a table from QEMU, then returns an object named CSTL from
> > + * it.
> > + * Everything is scoped under \\_SB.CPUS.CSTP.
> > + */
> > +void cst_build_acpi(GArray *table_data, BIOSLinker *linker, uint16_t ioport)
> > +{
> > +    Aml *ssdt, *scope, *field, *method;
> > +    uint32_t cstp_offset;
> > +
> > +    /* Put this in a separate SSDT table */
> > +    ssdt = init_aml_allocator();
> > +
> > +    /* Reserve space for header */
> > +    acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));
> > +
> > +    cstp_offset = table_data->len +
> > +        build_append_named_dword(ssdt->buf, "\\_SB.CPUS.CSTP");
> > +    scope = aml_scope("\\_SB.CPUS");
> > +    {
> > +        /* buffer in reserved memory to load the table from */
> > +        aml_append(scope, aml_operation_region("CSTB", AML_SYSTEM_MEMORY,
> > +                                               aml_name("\\_SB.CPUS.CSTP"),
> > +                                               4096));
> > +        /* write address here to update the table in memory */
> > +        aml_append(scope, aml_operation_region("CSTR", AML_SYSTEM_IO,
> > +                                               aml_int(ioport),
> > +                                               4));
> > +        field = aml_field("CSTR", AML_DWORD_ACC, AML_LOCK,
> > +                          AML_WRITE_AS_ZEROS);
> > +        {
> > +            aml_append(field, aml_named_field("CSTU", 32));
> > +        }
> > +        aml_append(scope, field);
> > +        method = aml_method("CCST", 0, AML_SERIALIZED);
> > +        {
> > +            Aml *ddbhandle = aml_local(0);
> > +            Aml *cst = aml_local(1);
> > +            /* Write buffer address to update table in memory. */
> > +            aml_append(method, aml_store(aml_name("CSTP"), aml_name("CSTU")));
> > +            aml_append(method, aml_load("CSTB", ddbhandle));
> > +            aml_append(method, aml_store(aml_name("CSTL"), cst));
> > +            aml_append(method, aml_unload(ddbhandle));
> > +            aml_append(method, aml_return(cst));
> > +        }
> > +        aml_append(scope, method);
> > +    }
> > +    aml_append(ssdt, scope);
> > +    g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
> > +
> > +    /* Why page boundary? no special reason right now but seems like
> > +     * a good idea for future extensions.      
> > +    */
> > +    bios_linker_loader_alloc(linker, ACPI_SCRATCH_BUFFER_NAME, cst_scratch,
> > +                             4096, false /* page boundary, high memory */);
> > +    /* Patch address of allocated memory into the AML so OSPM can retrieve
> > +     * and read it.
> > +     */
> > +    bios_linker_loader_add_pointer(linker,
> > +        ACPI_BUILD_TABLE_FILE, cstp_offset, sizeof(uint32_t),
> > +        ACPI_SCRATCH_BUFFER_NAME, 0);
> > +
> > +    //table_data->data[cstp_offset] = 0x8; /* hack */
> > +
> > +    build_header(linker, table_data,
> > +        (void *)(table_data->data + table_data->len - ssdt->buf->len),
> > +        "SSDT", ssdt->buf->len, 1, NULL, "CSTSSDT");
> > +
> > +    free_aml_allocator();
> > +}
> > +
> > +static GArray *cst_ssdt;
> > +
> > +static void cst_ssdt_setup(void)
> > +{
> > +    AcpiTableHeader *dyn_ssdt_hdr;
> > +    Aml *dyn_ssdt = init_aml_allocator();
> > +
> > +    /* Reserve space for header */
> > +    acpi_data_push(dyn_ssdt->buf, sizeof(AcpiTableHeader));
> > +    aml_append(dyn_ssdt, aml_name_decl("\\_SB.CPUS.CSTL", build_cst_package()));
> > +
> > +    dyn_ssdt_hdr = (AcpiTableHeader *)dyn_ssdt->buf->data;
> > +
> > +    acpi_init_header(dyn_ssdt_hdr, "SSDT", dyn_ssdt->buf->len, 1, NULL, "DYNSSDT");
> > +
> > +    dyn_ssdt_hdr->checksum = acpi_checksum((uint8_t *)dyn_ssdt_hdr,
> > +                                           dyn_ssdt->buf->len);
> > +
> > +    /* dyn_ssdt->buf will be freed. copy to cst_ssdt */
> > +    cst_ssdt = g_array_new(false, true, 1);
> > +    g_array_append_vals(cst_ssdt, dyn_ssdt->buf->data, dyn_ssdt->buf->len);
> > +
> > +    free_aml_allocator();
> > +}
> > +
> > +/* Update CST in system memory */
> > +static void cst_ioport_write(void *opaque, hwaddr addr, uint64_t data, unsigned size)
> > +{
> > +    assert(cst_ssdt);
> > +
> > +    cpu_physical_memory_write(data, cst_ssdt->data, cst_ssdt->len);
> > +}
> > +
> > +static const MemoryRegionOps cst_ops = {
> > +    .write = cst_ioport_write,
> > +    .impl = {
> > +        .min_access_size = 4,
> > +        .max_access_size = 4,
> > +    },
> > +    .endianness = DEVICE_LITTLE_ENDIAN,
> > +};
> > +
> > +static MemoryRegion cst_mr;
> > +
> > +void cst_register(FWCfgState *s, uint16_t ioport)
> > +{
> > +    cst_ssdt_setup();
> > +
> > +    /* Allocate guest scratch memory for the table */
> > +    cst_scratch = g_array_new(false, true, 1);
> > +    acpi_data_push(cst_scratch, 4096);
> > +    fw_cfg_add_file(s, ACPI_SCRATCH_BUFFER_NAME, cst_scratch->data,
> > +                    cst_scratch->len);
> > +
> > +    /* setup io to trigger updates */
> > +    memory_region_init_io(&cst_mr, NULL, &cst_ops, NULL, "cst-update-request", 4);
> > +    memory_region_add_subregion(get_system_io(), ioport, &cst_mr);
> it eats yet another IO port and a 4K page just for CST.

4K is a scratch pad we can reuse for any dynamic table.

Address is in fact 4K aligned - what if we reuse low bits in the io port
for signal type?  This way we won't burn more ports for the next dynamic
table.

> I'd prefer to extend docs/specs/acpi_cpu_hotplug.txt protocol
> to support passing _CST values via existing registers instead.

That's back to generating AML on the fly all over again.
There's a good reason we moved ACPI to QEMU.
I did code most of it up, and it's an unreadable mess.
Besides, next time we want to add another dynamic table
(like NUMA affinity) we'll have to re-do it again.

> 
> > +}
> > +
> > +/* TODO: API to notify guest of changes */
> > diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> > index 11c35bcb44..0119367455 100644
> > --- a/hw/acpi/Makefile.objs
> > +++ b/hw/acpi/Makefile.objs
> > @@ -1,5 +1,5 @@
> >  ifeq ($(CONFIG_ACPI),y)
> > -common-obj-$(CONFIG_ACPI_X86) += core.o piix4.o pcihp.o
> > +common-obj-$(CONFIG_ACPI_X86) += core.o piix4.o pcihp.o cst.o
> >  common-obj-$(CONFIG_ACPI_X86_ICH) += ich9.o tco.o
> >  common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu_hotplug.o
> >  common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o

Re: [Qemu-devel] [PATCH hack dontapply v2 6/7] acpi: aml generation for _CST

[Igor Mammedov]

On Wed, 25 Jul 2018 15:54:26 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jul 25, 2018 at 02:42:05PM +0200, Igor Mammedov wrote:
> > On Tue, 10 Jul 2018 03:01:33 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > This adds a method called CCST under CPUS.
> > > Each CPU will add _CST calling in turn CCST from there.
> > > 
> > > As _CST needs to change across migration, we use
> > > dynamic loading of tables as follows:
> > > 
> > > - a special scratch buffer, 4K in size is requested from bios
> > >   through the loader.
> > > - each time CCST executes, buffer address is passed to QEMU
> > >   which will write an SSDT table with the _CST package into the buffer.
> > > - table is then loaded and a package named \\_SB.CPUS.CSTL is loaded from there.
> > > - table is unloaded, package is then returned to caller
> > > 
> > > In this way, QEMU can change the package across migration.
> > > It will then notify the OSPM which will re-evaluate
> > > _CST.
> > > 
> > > In this proof of concept patch, package generation itself is
> > > still a stub, and change notifications are missing.
> > > 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  include/hw/acpi/cst.h |   8 ++
> > >  hw/acpi/cst.c         | 173 ++++++++++++++++++++++++++++++++++++++++++
> > >  hw/acpi/Makefile.objs |   2 +-
> > >  3 files changed, 182 insertions(+), 1 deletion(-)
> > >  create mode 100644 include/hw/acpi/cst.h
> > >  create mode 100644 hw/acpi/cst.c
> > > 
> > > diff --git a/include/hw/acpi/cst.h b/include/hw/acpi/cst.h
> > > new file mode 100644
> > > index 0000000000..2e40e87881
> > > --- /dev/null
> > > +++ b/include/hw/acpi/cst.h
> > > @@ -0,0 +1,8 @@
> > > +#ifndef HW_ACPI_CST_H
> > > +#define HW_ACPI_CST_H
> > > +
> > > +#include "hw/acpi/bios-linker-loader.h"
> > > +
> > > +void cst_build_acpi(GArray *table_data, BIOSLinker *linker, uint16_t ioport);
> > > +void cst_register(FWCfgState *s, uint16_t ioport);
> > > +#endif
> > > diff --git a/hw/acpi/cst.c b/hw/acpi/cst.c
> > > new file mode 100644
> > > index 0000000000..20dd80aef8
> > > --- /dev/null
> > > +++ b/hw/acpi/cst.c
> > > @@ -0,0 +1,173 @@
> > > +#include "qemu/osdep.h"
> > > +#include "exec/address-spaces.h"
> > > +#include "hw/acpi/aml-build.h"
> > > +#include "hw/acpi/cst.h"
> > > +#include "hw/acpi/acpi.h"
> > > +#include "hw/nvram/fw_cfg.h"
> > > +
> > > +#define ACPI_SCRATCH_BUFFER_NAME "etc/scratch"
> > > +
> > > +/* Hack! Incomplete! */
> > > +static Aml *build_cst_package(void)
> > > +{
> > > +    int i;
> > > +    Aml *crs;
> > > +    Aml *pkg;
> > > +    int cs_num = 3;
> > > +
> > > +    pkg = aml_package(cs_num + 1); /* # of ACPI Cx states + state count */
> > > +    aml_append(pkg, aml_int(cs_num)); /* # of ACPI Cx states */
> > > +
> > > +    for (i = 0; i < cs_num; i++) {
> > > +        Aml *cstate = aml_package(4);
> > > +
> > > +        crs = aml_resource_template();
> > > +        aml_append(crs, aml_register(AML_AS_SYSTEM_IO,
> > > +            0x8,
> > > +            0x0,
> > > +            0x100,
> > > +            0x1));
> > > +        aml_append(cstate, crs);
> > > +        aml_append(cstate, aml_int(i + 1)); /* Cx ACPI state */
> > > +        aml_append(cstate, aml_int((i + 1) * 10)); /* Latency */
> > > +        aml_append(cstate, aml_int(cs_num - i - 1));/* Power */
> > > +        aml_append(pkg, cstate);
> > > +    }
> > > +
> > > +    return pkg;
> > > +}
> > > +
> > > +static GArray *cst_scratch;
> > > +
> > > +/*
> > > + * Add an SSDT with a dynamic method named CCST. The method uses the specified
> > > + * ioport to load a table from QEMU, then returns an object named CSTL from
> > > + * it.
> > > + * Everything is scoped under \\_SB.CPUS.CSTP.
> > > + */
> > > +void cst_build_acpi(GArray *table_data, BIOSLinker *linker, uint16_t ioport)
> > > +{
> > > +    Aml *ssdt, *scope, *field, *method;
> > > +    uint32_t cstp_offset;
> > > +
> > > +    /* Put this in a separate SSDT table */
> > > +    ssdt = init_aml_allocator();
> > > +
> > > +    /* Reserve space for header */
> > > +    acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));
> > > +
> > > +    cstp_offset = table_data->len +
> > > +        build_append_named_dword(ssdt->buf, "\\_SB.CPUS.CSTP");
> > > +    scope = aml_scope("\\_SB.CPUS");
> > > +    {
> > > +        /* buffer in reserved memory to load the table from */
> > > +        aml_append(scope, aml_operation_region("CSTB", AML_SYSTEM_MEMORY,
> > > +                                               aml_name("\\_SB.CPUS.CSTP"),
> > > +                                               4096));
> > > +        /* write address here to update the table in memory */
> > > +        aml_append(scope, aml_operation_region("CSTR", AML_SYSTEM_IO,
> > > +                                               aml_int(ioport),
> > > +                                               4));
> > > +        field = aml_field("CSTR", AML_DWORD_ACC, AML_LOCK,
> > > +                          AML_WRITE_AS_ZEROS);
> > > +        {
> > > +            aml_append(field, aml_named_field("CSTU", 32));
> > > +        }
> > > +        aml_append(scope, field);
> > > +        method = aml_method("CCST", 0, AML_SERIALIZED);
> > > +        {
> > > +            Aml *ddbhandle = aml_local(0);
> > > +            Aml *cst = aml_local(1);
> > > +            /* Write buffer address to update table in memory. */
> > > +            aml_append(method, aml_store(aml_name("CSTP"), aml_name("CSTU")));
> > > +            aml_append(method, aml_load("CSTB", ddbhandle));
> > > +            aml_append(method, aml_store(aml_name("CSTL"), cst));
> > > +            aml_append(method, aml_unload(ddbhandle));
> > > +            aml_append(method, aml_return(cst));
> > > +        }
> > > +        aml_append(scope, method);
> > > +    }
> > > +    aml_append(ssdt, scope);
> > > +    g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
> > > +
> > > +    /* Why page boundary? no special reason right now but seems like
> > > +     * a good idea for future extensions.      
> > > +    */
> > > +    bios_linker_loader_alloc(linker, ACPI_SCRATCH_BUFFER_NAME, cst_scratch,
> > > +                             4096, false /* page boundary, high memory */);
> > > +    /* Patch address of allocated memory into the AML so OSPM can retrieve
> > > +     * and read it.
> > > +     */
> > > +    bios_linker_loader_add_pointer(linker,
> > > +        ACPI_BUILD_TABLE_FILE, cstp_offset, sizeof(uint32_t),
> > > +        ACPI_SCRATCH_BUFFER_NAME, 0);
> > > +
> > > +    //table_data->data[cstp_offset] = 0x8; /* hack */
> > > +
> > > +    build_header(linker, table_data,
> > > +        (void *)(table_data->data + table_data->len - ssdt->buf->len),
> > > +        "SSDT", ssdt->buf->len, 1, NULL, "CSTSSDT");
> > > +
> > > +    free_aml_allocator();
> > > +}
> > > +
> > > +static GArray *cst_ssdt;
> > > +
> > > +static void cst_ssdt_setup(void)
> > > +{
> > > +    AcpiTableHeader *dyn_ssdt_hdr;
> > > +    Aml *dyn_ssdt = init_aml_allocator();
> > > +
> > > +    /* Reserve space for header */
> > > +    acpi_data_push(dyn_ssdt->buf, sizeof(AcpiTableHeader));
> > > +    aml_append(dyn_ssdt, aml_name_decl("\\_SB.CPUS.CSTL", build_cst_package()));
> > > +
> > > +    dyn_ssdt_hdr = (AcpiTableHeader *)dyn_ssdt->buf->data;
> > > +
> > > +    acpi_init_header(dyn_ssdt_hdr, "SSDT", dyn_ssdt->buf->len, 1, NULL, "DYNSSDT");
> > > +
> > > +    dyn_ssdt_hdr->checksum = acpi_checksum((uint8_t *)dyn_ssdt_hdr,
> > > +                                           dyn_ssdt->buf->len);
> > > +
> > > +    /* dyn_ssdt->buf will be freed. copy to cst_ssdt */
> > > +    cst_ssdt = g_array_new(false, true, 1);
> > > +    g_array_append_vals(cst_ssdt, dyn_ssdt->buf->data, dyn_ssdt->buf->len);
> > > +
> > > +    free_aml_allocator();
> > > +}
> > > +
> > > +/* Update CST in system memory */
> > > +static void cst_ioport_write(void *opaque, hwaddr addr, uint64_t data, unsigned size)
> > > +{
> > > +    assert(cst_ssdt);
> > > +
> > > +    cpu_physical_memory_write(data, cst_ssdt->data, cst_ssdt->len);
> > > +}
> > > +
> > > +static const MemoryRegionOps cst_ops = {
> > > +    .write = cst_ioport_write,
> > > +    .impl = {
> > > +        .min_access_size = 4,
> > > +        .max_access_size = 4,
> > > +    },
> > > +    .endianness = DEVICE_LITTLE_ENDIAN,
> > > +};
> > > +
> > > +static MemoryRegion cst_mr;
> > > +
> > > +void cst_register(FWCfgState *s, uint16_t ioport)
> > > +{
> > > +    cst_ssdt_setup();
> > > +
> > > +    /* Allocate guest scratch memory for the table */
> > > +    cst_scratch = g_array_new(false, true, 1);
> > > +    acpi_data_push(cst_scratch, 4096);
> > > +    fw_cfg_add_file(s, ACPI_SCRATCH_BUFFER_NAME, cst_scratch->data,
> > > +                    cst_scratch->len);
> > > +
> > > +    /* setup io to trigger updates */
> > > +    memory_region_init_io(&cst_mr, NULL, &cst_ops, NULL, "cst-update-request", 4);
> > > +    memory_region_add_subregion(get_system_io(), ioport, &cst_mr);  
> > it eats yet another IO port and a 4K page just for CST.  
> 
> 4K is a scratch pad we can reuse for any dynamic table.
> 
> Address is in fact 4K aligned - what if we reuse low bits in the io port
> for signal type?  This way we won't burn more ports for the next dynamic
> table.
yep, and we already use it for nvdimm's NFIT table.
Earlier I've commented on HMAT series that tried to allocate another
IO&4K page that we should generalize and reuse NFIT implementation.

> > I'd prefer to extend docs/specs/acpi_cpu_hotplug.txt protocol
> > to support passing _CST values via existing registers instead.  
> 
> That's back to generating AML on the fly all over again.
> There's a good reason we moved ACPI to QEMU.
> I did code most of it up, and it's an unreadable mess.
It will be much more of a mess if we add AML versioning there
so we won't end up with mixed (different versions possibly conflicting)
static and dynamic AML in a guest after migration.

> Besides, next time we want to add another dynamic table
> (like NUMA affinity) we'll have to re-do it again.
CPU's dynamic numa affinity is already in protocol, we just made
it static because numa mapping is currently fixed and predefined.

Fixed dynamic tables are usually more or less self sufficient
so it's safer to reload so I'm ok with their reloading.

If it would be possible to reload whole AML namespace, I'd be
the first in a queue to advocate for it.

As for current cpuhp interface, it was designed to be extendable
and we can extend it for CST. It most likely would be much less
code to extend than it is in this series.

> >   
> > > +}
> > > +
> > > +/* TODO: API to notify guest of changes */
> > > diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> > > index 11c35bcb44..0119367455 100644
> > > --- a/hw/acpi/Makefile.objs
> > > +++ b/hw/acpi/Makefile.objs
> > > @@ -1,5 +1,5 @@
> > >  ifeq ($(CONFIG_ACPI),y)
> > > -common-obj-$(CONFIG_ACPI_X86) += core.o piix4.o pcihp.o
> > > +common-obj-$(CONFIG_ACPI_X86) += core.o piix4.o pcihp.o cst.o
> > >  common-obj-$(CONFIG_ACPI_X86_ICH) += ich9.o tco.o
> > >  common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu_hotplug.o
> > >  common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o  

Re: [Qemu-devel] [PATCH hack dontapply v2 6/7] acpi: aml generation for _CST

[Michael S. Tsirkin]

On Wed, Jul 25, 2018 at 04:39:28PM +0200, Igor Mammedov wrote:
> Fixed dynamic tables are usually more or less self sufficient
> so it's safer to reload so I'm ok with their reloading.

Well it's just a matter of excercising self-control and
building sane APIs to make sure we do.
E.g. let's say we agree on prefix DQ for dynamic tables.

Now, isn't

Name(DQCS, Package() {...} )

which is constrained not to use any names from the main
table not self-sufficient?

To ensure this contraint, we can add APIs like dynamic_name
that add the "DQ" prefix, and we can verify that a dynamic
table does not use a name without this prefix.

> As for current cpuhp interface, it was designed to be extendable
> and we can extend it for CST. It most likely would be much less
> code to extend than it is in this series.

I do have patches that use cpuhp to signal CST changes.
I don't think cpuhp solves the mess that is dynamic
package generation though.

-- 
MST

Re: [Qemu-devel] [PATCH hack dontapply v2 6/7] acpi: aml generation for _CST

[Michael S. Tsirkin]

On Wed, Jul 25, 2018 at 04:39:28PM +0200, Igor Mammedov wrote:
> > > > +void cst_register(FWCfgState *s, uint16_t ioport)
> > > > +{
> > > > +    cst_ssdt_setup();
> > > > +
> > > > +    /* Allocate guest scratch memory for the table */
> > > > +    cst_scratch = g_array_new(false, true, 1);
> > > > +    acpi_data_push(cst_scratch, 4096);
> > > > +    fw_cfg_add_file(s, ACPI_SCRATCH_BUFFER_NAME, cst_scratch->data,
> > > > +                    cst_scratch->len);
> > > > +
> > > > +    /* setup io to trigger updates */
> > > > +    memory_region_init_io(&cst_mr, NULL, &cst_ops, NULL, "cst-update-request", 4);
> > > > +    memory_region_add_subregion(get_system_io(), ioport, &cst_mr);  
> > > it eats yet another IO port and a 4K page just for CST.  
> > 
> > 4K is a scratch pad we can reuse for any dynamic table.
> > 
> > Address is in fact 4K aligned - what if we reuse low bits in the io port
> > for signal type?  This way we won't burn more ports for the next dynamic
> > table.
> yep, and we already use it for nvdimm's NFIT table.
> Earlier I've commented on HMAT series that tried to allocate another
> IO&4K page that we should generalize and reuse NFIT implementation.

That's a good point, I forgot about NFIT.

I agree there's no good reason to burn up more IO ports and memory,
I'll look into reusing NFIT buffer and ports.

-- 
MST

[Qemu-devel] [PATCH hack dontapply v2 5/7] acpi: init header without linking

[Michael S. Tsirkin]

build_header inits the table header and then adds it to list
for checksumming and linking into XSDT.

Dynamically loaded SSDTs are not linked into XSDT so
they just need the header initialized.
Split out the relevant part of build_header so it can
be invoked externally for this purpose.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/acpi/aml-build.h |  3 +++
 hw/acpi/aml-build.c         | 18 +++++++++++++-----
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index f764f71e25..0371a96c46 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -389,6 +389,9 @@ void
 build_header(BIOSLinker *linker, GArray *table_data,
              AcpiTableHeader *h, const char *sig, int len, uint8_t rev,
              const char *oem_id, const char *oem_table_id);
+void
+acpi_init_header(AcpiTableHeader *h, const char *sig, int len, uint8_t rev,
+                 const char *oem_id, const char *oem_table_id);
 void *acpi_data_push(GArray *table_data, unsigned size);
 unsigned acpi_data_len(GArray *table);
 void acpi_add_table(GArray *table_offsets, GArray *table_data);
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index ce691c1618..02f9710caa 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1574,12 +1574,9 @@ Aml *aml_unload(Aml *ddbhandle)
 }
 
 void
-build_header(BIOSLinker *linker, GArray *table_data,
-             AcpiTableHeader *h, const char *sig, int len, uint8_t rev,
-             const char *oem_id, const char *oem_table_id)
+acpi_init_header(AcpiTableHeader *h, const char *sig, int len, uint8_t rev,
+                 const char *oem_id, const char *oem_table_id)
 {
-    unsigned tbl_offset = (char *)h - table_data->data;
-    unsigned checksum_offset = (char *)&h->checksum - table_data->data;
     memcpy(&h->signature, sig, 4);
     h->length = cpu_to_le32(len);
     h->revision = rev;
@@ -1600,6 +1597,17 @@ build_header(BIOSLinker *linker, GArray *table_data,
     h->oem_revision = cpu_to_le32(1);
     memcpy(h->asl_compiler_id, ACPI_BUILD_APPNAME4, 4);
     h->asl_compiler_revision = cpu_to_le32(1);
+}
+
+void
+build_header(BIOSLinker *linker, GArray *table_data,
+             AcpiTableHeader *h, const char *sig, int len, uint8_t rev,
+             const char *oem_id, const char *oem_table_id)
+{
+    unsigned tbl_offset = (char *)h - table_data->data;
+    unsigned checksum_offset = (char *)&h->checksum - table_data->data;
+
+    acpi_init_header(h, sig, len, rev, oem_id, oem_table_id);
     /* Checksum to be filled in by Guest linker */
     bios_linker_loader_add_checksum(linker, ACPI_BUILD_TABLE_FILE,
         tbl_offset, len, checksum_offset);
-- 
MST

[Qemu-devel] [PATCH hack dontapply v2 7/7] pc: HACK: acpi: tie in _CST object to Processor

[Michael S. Tsirkin]

Based on patch by Igor Mammedov.

This is a hack: we definitely shouldn't do it
unconditionally, and initialization should be handled
differently (through an isa device?). io port to use
should depend on the PC type and should be documented.

Notifications should be supported.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/i386/pc.h |  5 +++++
 hw/acpi/cpu.c        |  5 +++++
 hw/i386/acpi-build.c | 10 +++++++++-
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 316230e570..83b3a84322 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -20,6 +20,11 @@
 
 #define HPET_INTCAP "hpet-intcap"
 
+typedef struct PCCstate {
+    uint32_t latency;
+    uint32_t hint;
+} PCCstate;
+
 /**
  * PCMachineState:
  * @acpi_dev: link to ACPI PM device that performs ACPI hotplug handling
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 5ae595ecbe..e9e207b033 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -561,6 +561,11 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
                            aml_int(arch_ids->cpus[i].props.node_id)));
             }
 
+            if (1) {
+                method = aml_method("_CST", 0, AML_NOTSERIALIZED);
+                aml_append(method, aml_return(aml_call0("CCST")));
+                aml_append(dev, method);
+            }
             aml_append(cpus_dev, dev);
         }
     }
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index fff1059a31..da2c830db7 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -64,6 +64,7 @@
 #include "hw/i386/intel_iommu.h"
 
 #include "hw/acpi/ipmi.h"
+#include "hw/acpi/cst.h"
 
 /* These are used to size the ACPI tables for -M pc-i440fx-1.7 and
  * -M pc-i440fx-2.0.  Even if the actual amount of AML generated grows
@@ -1840,7 +1841,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
         build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
     } else {
         CPUHotplugFeatures opts = {
-            .apci_1_compatible = true, .has_legacy_cphp = true
+            .apci_1_compatible = true, .has_legacy_cphp = true,
         };
         build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
                        "\\_SB.PCI0", "\\_GPE._E02");
@@ -2693,6 +2694,10 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
                            tables->vmgenid, tables->linker);
     }
 
+    /* TODO: get a free ioport. This one is PIIX specific. */
+    acpi_add_table(table_offsets, tables_blob);
+    cst_build_acpi(tables_blob, tables->linker, 0xaf20);
+
     if (misc.has_hpet) {
         acpi_add_table(table_offsets, tables_blob);
         build_hpet(tables_blob, tables->linker);
@@ -2891,6 +2896,9 @@ void acpi_setup(void)
 
     build_state = g_malloc0(sizeof *build_state);
 
+    /* TODO: this is not the best place to do it */
+    cst_register(pcms->fw_cfg, 0xaf20);
+
     acpi_build_tables_init(&tables);
     acpi_build(&tables, MACHINE(pcms));
 
-- 
MST

Re: [Qemu-devel] [PATCH hack dontapply v2 7/7] pc: HACK: acpi: tie in _CST object to Processor

[Igor Mammedov]

On Tue, 10 Jul 2018 03:01:34 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> Based on patch by Igor Mammedov.
> 
> This is a hack: we definitely shouldn't do it
> unconditionally, and initialization should be handled
> differently (through an isa device?). io port to use
> should depend on the PC type and should be documented.
> 
> Notifications should be supported.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/hw/i386/pc.h |  5 +++++
>  hw/acpi/cpu.c        |  5 +++++
>  hw/i386/acpi-build.c | 10 +++++++++-
>  3 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 316230e570..83b3a84322 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -20,6 +20,11 @@
>  
>  #define HPET_INTCAP "hpet-intcap"
>  
> +typedef struct PCCstate {
> +    uint32_t latency;
> +    uint32_t hint;
> +} PCCstate;
> +
>  /**
>   * PCMachineState:
>   * @acpi_dev: link to ACPI PM device that performs ACPI hotplug handling
> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> index 5ae595ecbe..e9e207b033 100644
> --- a/hw/acpi/cpu.c
> +++ b/hw/acpi/cpu.c
> @@ -561,6 +561,11 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
>                             aml_int(arch_ids->cpus[i].props.node_id)));
>              }
>  
> +            if (1) {
> +                method = aml_method("_CST", 0, AML_NOTSERIALIZED);
> +                aml_append(method, aml_return(aml_call0("CCST")));
in case of not loaded CCST it would be unresolved reference.
It would be better to have a package /SB.CPUS.CCST filled with some startup values
and than that could be updated on the fly with new package.

> +                aml_append(dev, method);
> +            }
>              aml_append(cpus_dev, dev);
>          }
>      }
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index fff1059a31..da2c830db7 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -64,6 +64,7 @@
>  #include "hw/i386/intel_iommu.h"
>  
>  #include "hw/acpi/ipmi.h"
> +#include "hw/acpi/cst.h"
>  
>  /* These are used to size the ACPI tables for -M pc-i440fx-1.7 and
>   * -M pc-i440fx-2.0.  Even if the actual amount of AML generated grows
> @@ -1840,7 +1841,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>          build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
>      } else {
>          CPUHotplugFeatures opts = {
> -            .apci_1_compatible = true, .has_legacy_cphp = true
> +            .apci_1_compatible = true, .has_legacy_cphp = true,
unrelated change???

>          };
>          build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
>                         "\\_SB.PCI0", "\\_GPE._E02");
> @@ -2693,6 +2694,10 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>                             tables->vmgenid, tables->linker);
>      }
>  
> +    /* TODO: get a free ioport. This one is PIIX specific. */
> +    acpi_add_table(table_offsets, tables_blob);
> +    cst_build_acpi(tables_blob, tables->linker, 0xaf20);
> +
>      if (misc.has_hpet) {
>          acpi_add_table(table_offsets, tables_blob);
>          build_hpet(tables_blob, tables->linker);
> @@ -2891,6 +2896,9 @@ void acpi_setup(void)
>  
>      build_state = g_malloc0(sizeof *build_state);
>  
> +    /* TODO: this is not the best place to do it */
> +    cst_register(pcms->fw_cfg, 0xaf20);
> +
>      acpi_build_tables_init(&tables);
>      acpi_build(&tables, MACHINE(pcms));
>  

Re: [Qemu-devel] [PATCH hack dontapply v2 7/7] pc: HACK: acpi: tie in _CST object to Processor

[Michael S. Tsirkin]

On Wed, Jul 25, 2018 at 02:37:24PM +0200, Igor Mammedov wrote:
> On Tue, 10 Jul 2018 03:01:34 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > Based on patch by Igor Mammedov.
> > 
> > This is a hack: we definitely shouldn't do it
> > unconditionally, and initialization should be handled
> > differently (through an isa device?). io port to use
> > should depend on the PC type and should be documented.
> > 
> > Notifications should be supported.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  include/hw/i386/pc.h |  5 +++++
> >  hw/acpi/cpu.c        |  5 +++++
> >  hw/i386/acpi-build.c | 10 +++++++++-
> >  3 files changed, 19 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 316230e570..83b3a84322 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -20,6 +20,11 @@
> >  
> >  #define HPET_INTCAP "hpet-intcap"
> >  
> > +typedef struct PCCstate {
> > +    uint32_t latency;
> > +    uint32_t hint;
> > +} PCCstate;
> > +
> >  /**
> >   * PCMachineState:
> >   * @acpi_dev: link to ACPI PM device that performs ACPI hotplug handling
> > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> > index 5ae595ecbe..e9e207b033 100644
> > --- a/hw/acpi/cpu.c
> > +++ b/hw/acpi/cpu.c
> > @@ -561,6 +561,11 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
> >                             aml_int(arch_ids->cpus[i].props.node_id)));
> >              }
> >  
> > +            if (1) {
> > +                method = aml_method("_CST", 0, AML_NOTSERIALIZED);
> > +                aml_append(method, aml_return(aml_call0("CCST")));
> in case of not loaded CCST it would be unresolved reference.
> It would be better to have a package /SB.CPUS.CCST filled with some startup values
> and than that could be updated on the fly with new package.

I think it will conflict if we do. But yes we could load \SB.CCST and
then \SB.CPUS.CCST will override that. Would it help though?
ACPI spec does not describe what happens on load failure.
For linux it does not matter much -
it just handles the error. What happens on windows with an unresolved
reference? Is failure to load the object any better?


> > +                aml_append(dev, method);
> > +            }
> >              aml_append(cpus_dev, dev);
> >          }
> >      }
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index fff1059a31..da2c830db7 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -64,6 +64,7 @@
> >  #include "hw/i386/intel_iommu.h"
> >  
> >  #include "hw/acpi/ipmi.h"
> > +#include "hw/acpi/cst.h"
> >  
> >  /* These are used to size the ACPI tables for -M pc-i440fx-1.7 and
> >   * -M pc-i440fx-2.0.  Even if the actual amount of AML generated grows
> > @@ -1840,7 +1841,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >          build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
> >      } else {
> >          CPUHotplugFeatures opts = {
> > -            .apci_1_compatible = true, .has_legacy_cphp = true
> > +            .apci_1_compatible = true, .has_legacy_cphp = true,
> unrelated change???

Good catch.

> >          };
> >          build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
> >                         "\\_SB.PCI0", "\\_GPE._E02");
> > @@ -2693,6 +2694,10 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> >                             tables->vmgenid, tables->linker);
> >      }
> >  
> > +    /* TODO: get a free ioport. This one is PIIX specific. */
> > +    acpi_add_table(table_offsets, tables_blob);
> > +    cst_build_acpi(tables_blob, tables->linker, 0xaf20);
> > +
> >      if (misc.has_hpet) {
> >          acpi_add_table(table_offsets, tables_blob);
> >          build_hpet(tables_blob, tables->linker);
> > @@ -2891,6 +2896,9 @@ void acpi_setup(void)
> >  
> >      build_state = g_malloc0(sizeof *build_state);
> >  
> > +    /* TODO: this is not the best place to do it */
> > +    cst_register(pcms->fw_cfg, 0xaf20);
> > +
> >      acpi_build_tables_init(&tables);
> >      acpi_build(&tables, MACHINE(pcms));
> >  

Re: [Qemu-devel] [PATCH hack dontapply v2 7/7] pc: HACK: acpi: tie in _CST object to Processor

[Igor Mammedov]

On Wed, 25 Jul 2018 15:50:09 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jul 25, 2018 at 02:37:24PM +0200, Igor Mammedov wrote:
> > On Tue, 10 Jul 2018 03:01:34 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > Based on patch by Igor Mammedov.
> > > 
> > > This is a hack: we definitely shouldn't do it
> > > unconditionally, and initialization should be handled
> > > differently (through an isa device?). io port to use
> > > should depend on the PC type and should be documented.
> > > 
> > > Notifications should be supported.
> > > 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  include/hw/i386/pc.h |  5 +++++
> > >  hw/acpi/cpu.c        |  5 +++++
> > >  hw/i386/acpi-build.c | 10 +++++++++-
> > >  3 files changed, 19 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > index 316230e570..83b3a84322 100644
> > > --- a/include/hw/i386/pc.h
> > > +++ b/include/hw/i386/pc.h
> > > @@ -20,6 +20,11 @@
> > >  
> > >  #define HPET_INTCAP "hpet-intcap"
> > >  
> > > +typedef struct PCCstate {
> > > +    uint32_t latency;
> > > +    uint32_t hint;
> > > +} PCCstate;
> > > +
> > >  /**
> > >   * PCMachineState:
> > >   * @acpi_dev: link to ACPI PM device that performs ACPI hotplug handling
> > > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> > > index 5ae595ecbe..e9e207b033 100644
> > > --- a/hw/acpi/cpu.c
> > > +++ b/hw/acpi/cpu.c
> > > @@ -561,6 +561,11 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
> > >                             aml_int(arch_ids->cpus[i].props.node_id)));
> > >              }
> > >  
> > > +            if (1) {
> > > +                method = aml_method("_CST", 0, AML_NOTSERIALIZED);
> > > +                aml_append(method, aml_return(aml_call0("CCST")));  
> > in case of not loaded CCST it would be unresolved reference.
> > It would be better to have a package /SB.CPUS.CCST filled with some startup values
> > and than that could be updated on the fly with new package.  
> 
> I think it will conflict if we do. But yes we could load \SB.CCST and
> then \SB.CPUS.CCST will override that. Would it help though?
> ACPI spec does not describe what happens on load failure.
> For linux it does not matter much -
> it just handles the error. What happens on windows with an unresolved
> reference? Is failure to load the object any better?
I was thinking more in a align of assigning a new temporary package value to
a statically named CCST:

  update_cst_METHOD()
     build package in local0
     \SB.CPUS.CCST = local0

so guest gets valid initial CCST with values on source host at boot time
and on target values are replaced on update.

It doesn't have to be named variable/could be a method but with always
valid values.

> 
> > > +                aml_append(dev, method);
> > > +            }
> > >              aml_append(cpus_dev, dev);
> > >          }
> > >      }
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index fff1059a31..da2c830db7 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -64,6 +64,7 @@
> > >  #include "hw/i386/intel_iommu.h"
> > >  
> > >  #include "hw/acpi/ipmi.h"
> > > +#include "hw/acpi/cst.h"
> > >  
> > >  /* These are used to size the ACPI tables for -M pc-i440fx-1.7 and
> > >   * -M pc-i440fx-2.0.  Even if the actual amount of AML generated grows
> > > @@ -1840,7 +1841,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > >          build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
> > >      } else {
> > >          CPUHotplugFeatures opts = {
> > > -            .apci_1_compatible = true, .has_legacy_cphp = true
> > > +            .apci_1_compatible = true, .has_legacy_cphp = true,  
> > unrelated change???  
> 
> Good catch.
> 
> > >          };
> > >          build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
> > >                         "\\_SB.PCI0", "\\_GPE._E02");
> > > @@ -2693,6 +2694,10 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> > >                             tables->vmgenid, tables->linker);
> > >      }
> > >  
> > > +    /* TODO: get a free ioport. This one is PIIX specific. */
> > > +    acpi_add_table(table_offsets, tables_blob);
> > > +    cst_build_acpi(tables_blob, tables->linker, 0xaf20);
> > > +
> > >      if (misc.has_hpet) {
> > >          acpi_add_table(table_offsets, tables_blob);
> > >          build_hpet(tables_blob, tables->linker);
> > > @@ -2891,6 +2896,9 @@ void acpi_setup(void)
> > >  
> > >      build_state = g_malloc0(sizeof *build_state);
> > >  
> > > +    /* TODO: this is not the best place to do it */
> > > +    cst_register(pcms->fw_cfg, 0xaf20);
> > > +
> > >      acpi_build_tables_init(&tables);
> > >      acpi_build(&tables, MACHINE(pcms));
> > >    

Re: [Qemu-devel] [PATCH hack dontapply v2 7/7] pc: HACK: acpi: tie in _CST object to Processor

[Michael S. Tsirkin]

On Wed, Jul 25, 2018 at 04:49:08PM +0200, Igor Mammedov wrote:
> On Wed, 25 Jul 2018 15:50:09 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jul 25, 2018 at 02:37:24PM +0200, Igor Mammedov wrote:
> > > On Tue, 10 Jul 2018 03:01:34 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >   
> > > > Based on patch by Igor Mammedov.
> > > > 
> > > > This is a hack: we definitely shouldn't do it
> > > > unconditionally, and initialization should be handled
> > > > differently (through an isa device?). io port to use
> > > > should depend on the PC type and should be documented.
> > > > 
> > > > Notifications should be supported.
> > > > 
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >  include/hw/i386/pc.h |  5 +++++
> > > >  hw/acpi/cpu.c        |  5 +++++
> > > >  hw/i386/acpi-build.c | 10 +++++++++-
> > > >  3 files changed, 19 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > > index 316230e570..83b3a84322 100644
> > > > --- a/include/hw/i386/pc.h
> > > > +++ b/include/hw/i386/pc.h
> > > > @@ -20,6 +20,11 @@
> > > >  
> > > >  #define HPET_INTCAP "hpet-intcap"
> > > >  
> > > > +typedef struct PCCstate {
> > > > +    uint32_t latency;
> > > > +    uint32_t hint;
> > > > +} PCCstate;
> > > > +
> > > >  /**
> > > >   * PCMachineState:
> > > >   * @acpi_dev: link to ACPI PM device that performs ACPI hotplug handling
> > > > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> > > > index 5ae595ecbe..e9e207b033 100644
> > > > --- a/hw/acpi/cpu.c
> > > > +++ b/hw/acpi/cpu.c
> > > > @@ -561,6 +561,11 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
> > > >                             aml_int(arch_ids->cpus[i].props.node_id)));
> > > >              }
> > > >  
> > > > +            if (1) {
> > > > +                method = aml_method("_CST", 0, AML_NOTSERIALIZED);
> > > > +                aml_append(method, aml_return(aml_call0("CCST")));  
> > > in case of not loaded CCST it would be unresolved reference.
> > > It would be better to have a package /SB.CPUS.CCST filled with some startup values
> > > and than that could be updated on the fly with new package.  
> > 
> > I think it will conflict if we do. But yes we could load \SB.CCST and
> > then \SB.CPUS.CCST will override that. Would it help though?
> > ACPI spec does not describe what happens on load failure.
> > For linux it does not matter much -
> > it just handles the error. What happens on windows with an unresolved
> > reference? Is failure to load the object any better?
> I was thinking more in a align of assigning a new temporary package value to
> a statically named CCST:
> 
>   update_cst_METHOD()
>      build package in local0
>      \SB.CPUS.CCST = local0
> 
> so guest gets valid initial CCST with values on source host at boot time
> and on target values are replaced on update.
> 
> It doesn't have to be named variable/could be a method but with always
> valid values.

Right but what are we trying to achieve here?

> > 
> > > > +                aml_append(dev, method);
> > > > +            }
> > > >              aml_append(cpus_dev, dev);
> > > >          }
> > > >      }
> > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > index fff1059a31..da2c830db7 100644
> > > > --- a/hw/i386/acpi-build.c
> > > > +++ b/hw/i386/acpi-build.c
> > > > @@ -64,6 +64,7 @@
> > > >  #include "hw/i386/intel_iommu.h"
> > > >  
> > > >  #include "hw/acpi/ipmi.h"
> > > > +#include "hw/acpi/cst.h"
> > > >  
> > > >  /* These are used to size the ACPI tables for -M pc-i440fx-1.7 and
> > > >   * -M pc-i440fx-2.0.  Even if the actual amount of AML generated grows
> > > > @@ -1840,7 +1841,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > > >          build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
> > > >      } else {
> > > >          CPUHotplugFeatures opts = {
> > > > -            .apci_1_compatible = true, .has_legacy_cphp = true
> > > > +            .apci_1_compatible = true, .has_legacy_cphp = true,  
> > > unrelated change???  
> > 
> > Good catch.
> > 
> > > >          };
> > > >          build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
> > > >                         "\\_SB.PCI0", "\\_GPE._E02");
> > > > @@ -2693,6 +2694,10 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> > > >                             tables->vmgenid, tables->linker);
> > > >      }
> > > >  
> > > > +    /* TODO: get a free ioport. This one is PIIX specific. */
> > > > +    acpi_add_table(table_offsets, tables_blob);
> > > > +    cst_build_acpi(tables_blob, tables->linker, 0xaf20);
> > > > +
> > > >      if (misc.has_hpet) {
> > > >          acpi_add_table(table_offsets, tables_blob);
> > > >          build_hpet(tables_blob, tables->linker);
> > > > @@ -2891,6 +2896,9 @@ void acpi_setup(void)
> > > >  
> > > >      build_state = g_malloc0(sizeof *build_state);
> > > >  
> > > > +    /* TODO: this is not the best place to do it */
> > > > +    cst_register(pcms->fw_cfg, 0xaf20);
> > > > +
> > > >      acpi_build_tables_init(&tables);
> > > >      acpi_build(&tables, MACHINE(pcms));
> > > >    

Re: [Qemu-devel] [PATCH hack dontapply v2 7/7] pc: HACK: acpi: tie in _CST object to Processor

[Igor Mammedov]

On Thu, 26 Jul 2018 18:49:57 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jul 25, 2018 at 04:49:08PM +0200, Igor Mammedov wrote:
> > On Wed, 25 Jul 2018 15:50:09 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Wed, Jul 25, 2018 at 02:37:24PM +0200, Igor Mammedov wrote:  
> > > > On Tue, 10 Jul 2018 03:01:34 +0300
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >     
> > > > > Based on patch by Igor Mammedov.
> > > > > 
> > > > > This is a hack: we definitely shouldn't do it
> > > > > unconditionally, and initialization should be handled
> > > > > differently (through an isa device?). io port to use
> > > > > should depend on the PC type and should be documented.
> > > > > 
> > > > > Notifications should be supported.
> > > > > 
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > ---
> > > > >  include/hw/i386/pc.h |  5 +++++
> > > > >  hw/acpi/cpu.c        |  5 +++++
> > > > >  hw/i386/acpi-build.c | 10 +++++++++-
> > > > >  3 files changed, 19 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > > > index 316230e570..83b3a84322 100644
> > > > > --- a/include/hw/i386/pc.h
> > > > > +++ b/include/hw/i386/pc.h
> > > > > @@ -20,6 +20,11 @@
> > > > >  
> > > > >  #define HPET_INTCAP "hpet-intcap"
> > > > >  
> > > > > +typedef struct PCCstate {
> > > > > +    uint32_t latency;
> > > > > +    uint32_t hint;
> > > > > +} PCCstate;
> > > > > +
> > > > >  /**
> > > > >   * PCMachineState:
> > > > >   * @acpi_dev: link to ACPI PM device that performs ACPI hotplug handling
> > > > > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> > > > > index 5ae595ecbe..e9e207b033 100644
> > > > > --- a/hw/acpi/cpu.c
> > > > > +++ b/hw/acpi/cpu.c
> > > > > @@ -561,6 +561,11 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
> > > > >                             aml_int(arch_ids->cpus[i].props.node_id)));
> > > > >              }
> > > > >  
> > > > > +            if (1) {
> > > > > +                method = aml_method("_CST", 0, AML_NOTSERIALIZED);
> > > > > +                aml_append(method, aml_return(aml_call0("CCST")));    
> > > > in case of not loaded CCST it would be unresolved reference.
> > > > It would be better to have a package /SB.CPUS.CCST filled with some startup values
> > > > and than that could be updated on the fly with new package.    
> > > 
> > > I think it will conflict if we do. But yes we could load \SB.CCST and
> > > then \SB.CPUS.CCST will override that. Would it help though?
> > > ACPI spec does not describe what happens on load failure.
> > > For linux it does not matter much -
> > > it just handles the error. What happens on windows with an unresolved
> > > reference? Is failure to load the object any better?  
> > I was thinking more in a align of assigning a new temporary package value to
> > a statically named CCST:
> > 
> >   update_cst_METHOD()
> >      build package in local0
> >      \SB.CPUS.CCST = local0
> > 
> > so guest gets valid initial CCST with values on source host at boot time
> > and on target values are replaced on update.
> > 
> > It doesn't have to be named variable/could be a method but with always
> > valid values.  
> 
> Right but what are we trying to achieve here?
have a valid package not only on update but on boot up as well

> 
> > >   
> > > > > +                aml_append(dev, method);
> > > > > +            }
> > > > >              aml_append(cpus_dev, dev);
> > > > >          }
> > > > >      }
> > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > index fff1059a31..da2c830db7 100644
> > > > > --- a/hw/i386/acpi-build.c
> > > > > +++ b/hw/i386/acpi-build.c
> > > > > @@ -64,6 +64,7 @@
> > > > >  #include "hw/i386/intel_iommu.h"
> > > > >  
> > > > >  #include "hw/acpi/ipmi.h"
> > > > > +#include "hw/acpi/cst.h"
> > > > >  
> > > > >  /* These are used to size the ACPI tables for -M pc-i440fx-1.7 and
> > > > >   * -M pc-i440fx-2.0.  Even if the actual amount of AML generated grows
> > > > > @@ -1840,7 +1841,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > > > >          build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
> > > > >      } else {
> > > > >          CPUHotplugFeatures opts = {
> > > > > -            .apci_1_compatible = true, .has_legacy_cphp = true
> > > > > +            .apci_1_compatible = true, .has_legacy_cphp = true,    
> > > > unrelated change???    
> > > 
> > > Good catch.
> > >   
> > > > >          };
> > > > >          build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
> > > > >                         "\\_SB.PCI0", "\\_GPE._E02");
> > > > > @@ -2693,6 +2694,10 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> > > > >                             tables->vmgenid, tables->linker);
> > > > >      }
> > > > >  
> > > > > +    /* TODO: get a free ioport. This one is PIIX specific. */
> > > > > +    acpi_add_table(table_offsets, tables_blob);
> > > > > +    cst_build_acpi(tables_blob, tables->linker, 0xaf20);
> > > > > +
> > > > >      if (misc.has_hpet) {
> > > > >          acpi_add_table(table_offsets, tables_blob);
> > > > >          build_hpet(tables_blob, tables->linker);
> > > > > @@ -2891,6 +2896,9 @@ void acpi_setup(void)
> > > > >  
> > > > >      build_state = g_malloc0(sizeof *build_state);
> > > > >  
> > > > > +    /* TODO: this is not the best place to do it */
> > > > > +    cst_register(pcms->fw_cfg, 0xaf20);
> > > > > +
> > > > >      acpi_build_tables_init(&tables);
> > > > >      acpi_build(&tables, MACHINE(pcms));
> > > > >      

Re: [Qemu-devel] [PATCH hack dontapply v2 7/7] pc: HACK: acpi: tie in _CST object to Processor

[Michael S. Tsirkin]

On Fri, Jul 27, 2018 at 05:02:06PM +0200, Igor Mammedov wrote:
> On Thu, 26 Jul 2018 18:49:57 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jul 25, 2018 at 04:49:08PM +0200, Igor Mammedov wrote:
> > > On Wed, 25 Jul 2018 15:50:09 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >   
> > > > On Wed, Jul 25, 2018 at 02:37:24PM +0200, Igor Mammedov wrote:  
> > > > > On Tue, 10 Jul 2018 03:01:34 +0300
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > >     
> > > > > > Based on patch by Igor Mammedov.
> > > > > > 
> > > > > > This is a hack: we definitely shouldn't do it
> > > > > > unconditionally, and initialization should be handled
> > > > > > differently (through an isa device?). io port to use
> > > > > > should depend on the PC type and should be documented.
> > > > > > 
> > > > > > Notifications should be supported.
> > > > > > 
> > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > > ---
> > > > > >  include/hw/i386/pc.h |  5 +++++
> > > > > >  hw/acpi/cpu.c        |  5 +++++
> > > > > >  hw/i386/acpi-build.c | 10 +++++++++-
> > > > > >  3 files changed, 19 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > > > > index 316230e570..83b3a84322 100644
> > > > > > --- a/include/hw/i386/pc.h
> > > > > > +++ b/include/hw/i386/pc.h
> > > > > > @@ -20,6 +20,11 @@
> > > > > >  
> > > > > >  #define HPET_INTCAP "hpet-intcap"
> > > > > >  
> > > > > > +typedef struct PCCstate {
> > > > > > +    uint32_t latency;
> > > > > > +    uint32_t hint;
> > > > > > +} PCCstate;
> > > > > > +
> > > > > >  /**
> > > > > >   * PCMachineState:
> > > > > >   * @acpi_dev: link to ACPI PM device that performs ACPI hotplug handling
> > > > > > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> > > > > > index 5ae595ecbe..e9e207b033 100644
> > > > > > --- a/hw/acpi/cpu.c
> > > > > > +++ b/hw/acpi/cpu.c
> > > > > > @@ -561,6 +561,11 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
> > > > > >                             aml_int(arch_ids->cpus[i].props.node_id)));
> > > > > >              }
> > > > > >  
> > > > > > +            if (1) {
> > > > > > +                method = aml_method("_CST", 0, AML_NOTSERIALIZED);
> > > > > > +                aml_append(method, aml_return(aml_call0("CCST")));    
> > > > > in case of not loaded CCST it would be unresolved reference.
> > > > > It would be better to have a package /SB.CPUS.CCST filled with some startup values
> > > > > and than that could be updated on the fly with new package.    
> > > > 
> > > > I think it will conflict if we do. But yes we could load \SB.CCST and
> > > > then \SB.CPUS.CCST will override that. Would it help though?
> > > > ACPI spec does not describe what happens on load failure.
> > > > For linux it does not matter much -
> > > > it just handles the error. What happens on windows with an unresolved
> > > > reference? Is failure to load the object any better?  
> > > I was thinking more in a align of assigning a new temporary package value to
> > > a statically named CCST:
> > > 
> > >   update_cst_METHOD()
> > >      build package in local0
> > >      \SB.CPUS.CCST = local0
> > > 
> > > so guest gets valid initial CCST with values on source host at boot time
> > > and on target values are replaced on update.
> > > 
> > > It doesn't have to be named variable/could be a method but with always
> > > valid values.  
> > 
> > Right but what are we trying to achieve here?
> have a valid package not only on update but on boot up as well


So my patches basically do:

(1)
on interrupt:
	notify OSPM
on _CST
	get CST from host and return

What you propose:

(2)
on interrupt:
	get CST from host and store in CCST
	notify OSPM
on _CST
	return CCST

Is this a fair summary? If so I'd like to understand what do you mean
when you say "have a valid package on boot up as well".  In both
approaches _CST always returns a valid package *on each call*.

Is it that in absence of migration the configuration is easier to debug
since you can just dump SSDT to see the _CST value?

I think that's fair - unfortunately with (2) it is by comparison to (1)
harder to debug and test _CST updates since that flow looks quite
different from how normal _CST works. E.g. testing is harder as you need
migration to trigger DMA.  I would prefer a single path that works the
same with and without migration.

Let me know if I understand your concern correctly, if yes I'll try to
think of solutions.


> > 
> > > >   
> > > > > > +                aml_append(dev, method);
> > > > > > +            }
> > > > > >              aml_append(cpus_dev, dev);
> > > > > >          }
> > > > > >      }
> > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > > index fff1059a31..da2c830db7 100644
> > > > > > --- a/hw/i386/acpi-build.c
> > > > > > +++ b/hw/i386/acpi-build.c
> > > > > > @@ -64,6 +64,7 @@
> > > > > >  #include "hw/i386/intel_iommu.h"
> > > > > >  
> > > > > >  #include "hw/acpi/ipmi.h"
> > > > > > +#include "hw/acpi/cst.h"
> > > > > >  
> > > > > >  /* These are used to size the ACPI tables for -M pc-i440fx-1.7 and
> > > > > >   * -M pc-i440fx-2.0.  Even if the actual amount of AML generated grows
> > > > > > @@ -1840,7 +1841,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > > > > >          build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
> > > > > >      } else {
> > > > > >          CPUHotplugFeatures opts = {
> > > > > > -            .apci_1_compatible = true, .has_legacy_cphp = true
> > > > > > +            .apci_1_compatible = true, .has_legacy_cphp = true,    
> > > > > unrelated change???    
> > > > 
> > > > Good catch.
> > > >   
> > > > > >          };
> > > > > >          build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
> > > > > >                         "\\_SB.PCI0", "\\_GPE._E02");
> > > > > > @@ -2693,6 +2694,10 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> > > > > >                             tables->vmgenid, tables->linker);
> > > > > >      }
> > > > > >  
> > > > > > +    /* TODO: get a free ioport. This one is PIIX specific. */
> > > > > > +    acpi_add_table(table_offsets, tables_blob);
> > > > > > +    cst_build_acpi(tables_blob, tables->linker, 0xaf20);
> > > > > > +
> > > > > >      if (misc.has_hpet) {
> > > > > >          acpi_add_table(table_offsets, tables_blob);
> > > > > >          build_hpet(tables_blob, tables->linker);
> > > > > > @@ -2891,6 +2896,9 @@ void acpi_setup(void)
> > > > > >  
> > > > > >      build_state = g_malloc0(sizeof *build_state);
> > > > > >  
> > > > > > +    /* TODO: this is not the best place to do it */
> > > > > > +    cst_register(pcms->fw_cfg, 0xaf20);
> > > > > > +
> > > > > >      acpi_build_tables_init(&tables);
> > > > > >      acpi_build(&tables, MACHINE(pcms));
> > > > > >      

Re: [Qemu-devel] [PATCH hack dontapply v2 0/7] Dynamic _CST generation

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180710000024.542612-1-mst@redhat.com
Subject: [Qemu-devel] [PATCH hack dontapply v2 0/7] Dynamic _CST generation

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]            patchew/20180626135035.133432-1-vsementsov@virtuozzo.com -> patchew/20180626135035.133432-1-vsementsov@virtuozzo.com
 * [new tag]               patchew/20180710000024.542612-1-mst@redhat.com -> patchew/20180710000024.542612-1-mst@redhat.com
Switched to a new branch 'test'
eea669947b pc: HACK: acpi: tie in _CST object to Processor
51455e6143 acpi: aml generation for _CST
be172c3f6f acpi: init header without linking
8986b8080b acpi: export acpi_checksum
3c7a978139 acpi: aml_load/aml_unload
4fc698863e acpi: generalize aml_package / aml_varpackage
75d0818dd8 acpi: aml: add aml_register()

=== OUTPUT BEGIN ===
Checking PATCH 1/7: acpi: aml: add aml_register()...
Checking PATCH 2/7: acpi: generalize aml_package / aml_varpackage...
Checking PATCH 3/7: acpi: aml_load/aml_unload...
Checking PATCH 4/7: acpi: export acpi_checksum...
Checking PATCH 5/7: acpi: init header without linking...
Checking PATCH 6/7: acpi: aml generation for _CST...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#41: 
new file mode 100644

ERROR: trailing whitespace
#139: FILE: hw/acpi/cst.c:94:
+     * a good idea for future extensions.      $

ERROR: do not use C99 // comments
#150: FILE: hw/acpi/cst.c:105:
+    //table_data->data[cstp_offset] = 0x8; /* hack */

WARNING: line over 80 characters
#172: FILE: hw/acpi/cst.c:127:
+    acpi_init_header(dyn_ssdt_hdr, "SSDT", dyn_ssdt->buf->len, 1, NULL, "DYNSSDT");

WARNING: line over 80 characters
#185: FILE: hw/acpi/cst.c:140:
+static void cst_ioport_write(void *opaque, hwaddr addr, uint64_t data, unsigned size)

WARNING: line over 80 characters
#214: FILE: hw/acpi/cst.c:169:
+    memory_region_init_io(&cst_mr, NULL, &cst_ops, NULL, "cst-update-request", 4);

total: 2 errors, 4 warnings, 187 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 7/7: pc: HACK: acpi: tie in _CST object to Processor...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

[Qemu-devel] [PATCH hack dontapply v2 8/7 untested] acpi: support cst change notifications

[Michael S. Tsirkin]

(Ab)use cpu hotplug event infrastructure to report per-CPU _CST change
events.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Untested infrastructure to notify about _CST changes.

 include/hw/acpi/cpu.h |  4 ++++
 hw/acpi/cpu.c         | 39 +++++++++++++++++++++++++++++++++++----
 hw/acpi/trace-events  |  3 ++-
 3 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
index 89ce172941..31e20c590b 100644
--- a/include/hw/acpi/cpu.h
+++ b/include/hw/acpi/cpu.h
@@ -22,6 +22,7 @@ typedef struct AcpiCpuStatus {
     uint64_t arch_id;
     bool is_inserting;
     bool is_removing;
+    bool is_cst_changing;
     uint32_t ost_event;
     uint32_t ost_status;
 } AcpiCpuStatus;
@@ -37,6 +38,9 @@ typedef struct CPUHotplugState {
 void acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
                       CPUHotplugState *cpu_st, DeviceState *dev, Error **errp);
 
+void acpi_cpu_change_cst(HotplugHandler *hotplug_dev,
+                         CPUHotplugState *cpu_st, DeviceState *dev, Error **errp);
+
 void acpi_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
                                 CPUHotplugState *cpu_st,
                                 DeviceState *dev, Error **errp);
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index e9e207b033..6dc0cfb901 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -66,6 +66,7 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr, unsigned size)
         val |= cdev->cpu ? 1 : 0;
         val |= cdev->is_inserting ? 2 : 0;
         val |= cdev->is_removing  ? 4 : 0;
+        val |= cdev->is_cst_changing  ? 0x10 : 0;
         trace_cpuhp_acpi_read_flags(cpu_st->selector, val);
         break;
     case ACPI_CPU_CMD_DATA_OFFSET_RW:
@@ -126,6 +127,9 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr, uint64_t data,
             dev = DEVICE(cdev->cpu);
             hotplug_ctrl = qdev_get_hotplug_handler(dev);
             hotplug_handler_unplug(hotplug_ctrl, dev, NULL);
+        } else if (data & 0x10) {
+            cdev->is_cst_changing = false;
+            trace_cpuhp_acpi_clear_cst_evt(cpu_st->selector);
         }
         break;
     case ACPI_CPU_CMD_OFFSET_WR:
@@ -137,10 +141,12 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr, uint64_t data,
 
                 do {
                     cdev = &cpu_st->devs[iter];
-                    if (cdev->is_inserting || cdev->is_removing) {
+                    if (cdev->is_inserting || cdev->is_removing ||
+                        cdev->is_cst_changing) {
                         cpu_st->selector = iter;
                         trace_cpuhp_acpi_cpu_has_events(cpu_st->selector,
-                            cdev->is_inserting, cdev->is_removing);
+                            cdev->is_inserting, cdev->is_removing,
+                            cdev->is_cst_changing);
                         break;
                     }
                     iter = iter + 1 < cpu_st->dev_count ? iter + 1 : 0;
@@ -237,6 +243,18 @@ void acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
     }
 }
 
+void acpi_cpu_change_cst(HotplugHandler *hotplug_dev,
+                         CPUHotplugState *cpu_st, DeviceState *dev, Error **errp)
+{
+    AcpiCpuStatus *cdev = get_cpu_status(cpu_st, dev);
+    if (!cdev) {
+        return;
+    }
+
+    cdev->is_cst_changing = true;
+    acpi_send_event(DEVICE(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS);
+}
+
 void acpi_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
                                 CPUHotplugState *cpu_st,
                                 DeviceState *dev, Error **errp)
@@ -273,6 +291,7 @@ static const VMStateDescription vmstate_cpuhp_sts = {
     .fields      = (VMStateField[]) {
         VMSTATE_BOOL(is_inserting, AcpiCpuStatus),
         VMSTATE_BOOL(is_removing, AcpiCpuStatus),
+        VMSTATE_BOOL(is_cst_changing, AcpiCpuStatus),
         VMSTATE_UINT32(ost_event, AcpiCpuStatus),
         VMSTATE_UINT32(ost_status, AcpiCpuStatus),
         VMSTATE_END_OF_LIST()
@@ -309,6 +328,7 @@ const VMStateDescription vmstate_cpu_hotplug = {
 #define CPU_INSERT_EVENT  "CINS"
 #define CPU_REMOVE_EVENT  "CRMV"
 #define CPU_EJECT_EVENT   "CEJ0"
+#define CPU_CSTATE_EVENT  "CSTE"
 
 void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
                     hwaddr io_base,
@@ -361,7 +381,8 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
         aml_append(field, aml_named_field(CPU_REMOVE_EVENT, 1));
         /* initiates device eject, write only */
         aml_append(field, aml_named_field(CPU_EJECT_EVENT, 1));
-        aml_append(field, aml_reserved_field(4));
+        aml_append(field, aml_named_field(CPU_CSTATE_EVENT, 1));
+        aml_append(field, aml_reserved_field(3));
         aml_append(field, aml_named_field(CPU_COMMAND, 8));
         aml_append(cpu_ctrl_dev, field);
 
@@ -396,6 +417,7 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
         Aml *ins_evt = aml_name("%s.%s", cphp_res_path, CPU_INSERT_EVENT);
         Aml *rm_evt = aml_name("%s.%s", cphp_res_path, CPU_REMOVE_EVENT);
         Aml *ej_evt = aml_name("%s.%s", cphp_res_path, CPU_EJECT_EVENT);
+        Aml *cst_evt = aml_name("%s.%s", cphp_res_path, CPU_CSTATE_EVENT);
 
         aml_append(cpus_dev, aml_name_decl("_HID", aml_string("ACPI0010")));
         aml_append(cpus_dev, aml_name_decl("_CID", aml_eisaid("PNP0A05")));
@@ -450,17 +472,26 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
             Aml *has_event = aml_local(0);
             Aml *dev_chk = aml_int(1);
             Aml *eject_req = aml_int(3);
+            Aml *cst_changed = aml_int(0x81); /* Re-evaluate _CST: ACPI 2.0: 8.3.2 _CST (C States) */
             Aml *next_cpu_cmd = aml_int(CPHP_GET_NEXT_CPU_WITH_EVENT_CMD);
 
             aml_append(method, aml_acquire(ctrl_lock, 0xFFFF));
             aml_append(method, aml_store(one, has_event));
             while_ctx = aml_while(aml_equal(has_event, one));
             {
-                 /* clear loop exit condition, ins_evt/rm_evt checks
+                 /* clear loop exit condition, ins_evt/rm_evt/cst_evt checks
                   * will set it to 1 while next_cpu_cmd returns a CPU
                   * with events */
                  aml_append(while_ctx, aml_store(zero, has_event));
                  aml_append(while_ctx, aml_store(next_cpu_cmd, cpu_cmd));
+                 ifctx = aml_if(aml_equal(cst_evt, one));
+                 {
+                     aml_append(ifctx,
+                         aml_call2(CPU_NOTIFY_METHOD, cpu_data, cst_changed));
+                     aml_append(ifctx, aml_store(one, cst_evt));
+                     aml_append(ifctx, aml_store(one, has_event));
+                 }
+                 aml_append(while_ctx, ifctx);
                  ifctx = aml_if(aml_equal(ins_evt, one));
                  {
                      aml_append(ifctx,
diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events
index df0024f8b2..d9ea0dc0e3 100644
--- a/hw/acpi/trace-events
+++ b/hw/acpi/trace-events
@@ -23,9 +23,10 @@ cpuhp_acpi_read_flags(uint32_t idx, uint8_t flags) "idx[0x%"PRIx32"] flags: 0x%"
 cpuhp_acpi_write_idx(uint32_t idx) "set active cpu idx: 0x%"PRIx32
 cpuhp_acpi_write_cmd(uint32_t idx, uint8_t cmd) "idx[0x%"PRIx32"] cmd: 0x%"PRIx8
 cpuhp_acpi_read_cmd_data(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 0x%"PRIx32
-cpuhp_acpi_cpu_has_events(uint32_t idx, bool ins, bool rm) "idx[0x%"PRIx32"] inserting: %d, removing: %d"
+cpuhp_acpi_cpu_has_events(uint32_t idx, bool ins, bool rm, bool cst) "idx[0x%"PRIx32"] inserting: %d, removing: %d, cst: %d"
 cpuhp_acpi_clear_inserting_evt(uint32_t idx) "idx[0x%"PRIx32"]"
 cpuhp_acpi_clear_remove_evt(uint32_t idx) "idx[0x%"PRIx32"]"
+cpuhp_acpi_clear_cst_evt(uint32_t idx) "idx[0x%"PRIx32"]"
 cpuhp_acpi_ejecting_invalid_cpu(uint32_t idx) "0x%"PRIx32
 cpuhp_acpi_ejecting_cpu(uint32_t idx) "0x%"PRIx32
 cpuhp_acpi_write_ost_ev(uint32_t slot, uint32_t ev) "idx[0x%"PRIx32"] OST EVENT: 0x%"PRIx32
-- 
MST

Re: [Qemu-devel] [PATCH hack dontapply v2 0/7] Dynamic _CST generation

[Igor Mammedov]

On Tue, 10 Jul 2018 03:01:30 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> Now that basic support for guest CPU PM is upstream, I started looking
> for making it migrateable.  Since a VM can be migrated between different
> hosts, PM info needs to change each time with move the VM to a different
> host.
Considering notification is async, so there will be a window when
guest will be using old Cstates on new host. What will happen if
machine is migrated to host that doesn't support a Cstate
that was used on source when guest was started?

> This adds infrastructure - based on Load/Unload - to support exactly
> that: QEMU generates AML (changing it on migration) and stores it in
> reserved memory, OSPM loads _CST from there on demand.
Cool and very tempting idea but I have 2 worries about this approach:
1. How well does it work with Windows based guests?
   (I've tried something similar but generating new AML from AML itself
    to get rid of our long if/else chains there to make up Notify target name.
    I ditched it (unfortunately I don't recall why) )

2. (probably biggest issue) Loading dynamically generated AML
   basically would make all AML code ABI, so that static AML
   in RAM of old QEMU version would match dynamic generated
   one on target side with new QEMU (/me generalizing approach beyond _CST support).
   I'd try to keep our AML version less as much as possible
   and go this route only if there is no other way to do it.

> This is a partially functional prototype.
> 
> What works:
>     loading _CST dynamically and reporting it to OSPM
> 
> What doesn't:
>     detecting host configuration and generating correct _CST package
>     notifying guest about the change to trigger _CST re-evaluation
>     disabling mwait/halt exists as appropriate
> 
> Michael S. Tsirkin (6):
>   acpi: aml: add aml_register()
>   acpi: generalize aml_package / aml_varpackage
>   acpi: aml_load/aml_unload
>   acpi: export acpi_checksum
>   acpi: init header without linking
>   acpi: aml generation for _CST
>   pc: HACK: acpi: tie in _CST object to Processor
> 
>  include/hw/acpi/acpi.h      |   2 +
>  include/hw/acpi/aml-build.h |  14 ++-
>  include/hw/acpi/cst.h       |   8 ++
>  include/hw/i386/pc.h        |   5 ++
>  hw/acpi/aml-build.c         |  81 ++++++++++++++---
>  hw/acpi/core.c              |   2 +-
>  hw/acpi/cpu.c               |   5 ++
>  hw/acpi/cpu_hotplug.c       |  11 +--
>  hw/acpi/cst.c               | 173 ++++++++++++++++++++++++++++++++++++
>  hw/arm/virt-acpi-build.c    |   2 +-
>  hw/i386/acpi-build.c        |  10 ++-
>  hw/acpi/Makefile.objs       |   2 +-
>  12 files changed, 290 insertions(+), 25 deletions(-)
>  create mode 100644 include/hw/acpi/cst.h
>  create mode 100644 hw/acpi/cst.c
> 

Re: [Qemu-devel] [PATCH hack dontapply v2 0/7] Dynamic _CST generation

[Michael S. Tsirkin]

On Wed, Jul 25, 2018 at 02:32:11PM +0200, Igor Mammedov wrote:
> On Tue, 10 Jul 2018 03:01:30 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > Now that basic support for guest CPU PM is upstream, I started looking
> > for making it migrateable.  Since a VM can be migrated between different
> > hosts, PM info needs to change each time with move the VM to a different
> > host.
> Considering notification is async, so there will be a window when
> guest will be using old Cstates on new host. What will happen if
> machine is migrated to host that doesn't support a Cstate
> that was used on source when guest was started?

My testing shows mwait with a wrong hint works, presumably it just uses
a lot of power.

> > This adds infrastructure - based on Load/Unload - to support exactly
> > that: QEMU generates AML (changing it on migration) and stores it in
> > reserved memory, OSPM loads _CST from there on demand.
> Cool and very tempting idea but I have 2 worries about this approach:
> 1. How well does it work with Windows based guests?
>    (I've tried something similar but generating new AML from AML itself
>     to get rid of our long if/else chains there to make up Notify target name.
>     I ditched it (unfortunately I don't recall why) )

After trying it, I can tell you why - it's a horrid mess of
unreadable code, with arbitrary restraints on package
length etc.

> 2. (probably biggest issue) Loading dynamically generated AML
>    basically would make all AML code ABI, so that static AML
>    in RAM of old QEMU version would match dynamic generated
>    one on target side with new QEMU (/me generalizing approach beyond _CST support).
>    I'd try to keep our AML version less as much as possible
>    and go this route only if there is no other way to do it.

That's a good point, thanks for bringing this up!

So it seems that we will need to define the ABI, yes. All AML code is
an over-statement though, there are specific entry points
we must maintain, right?

And that in the end isn't fundamentally different from the ABIs
mandated by the ACPI spec.

So I have these ideas to try to mitigate the issues:
- document the ABI (like we have in the ACPI spec)
- use a specific prefix for all external calls (like _ for ACPI spec ones)
- use a specific (different) prefix for all internal loaded calls (like
  [A-Z] for non-ACPI spec ones)

Thoughts?

> > This is a partially functional prototype.
> > 
> > What works:
> >     loading _CST dynamically and reporting it to OSPM
> > 
> > What doesn't:
> >     detecting host configuration and generating correct _CST package
> >     notifying guest about the change to trigger _CST re-evaluation
> >     disabling mwait/halt exists as appropriate
> > 
> > Michael S. Tsirkin (6):
> >   acpi: aml: add aml_register()
> >   acpi: generalize aml_package / aml_varpackage
> >   acpi: aml_load/aml_unload
> >   acpi: export acpi_checksum
> >   acpi: init header without linking
> >   acpi: aml generation for _CST
> >   pc: HACK: acpi: tie in _CST object to Processor
> > 
> >  include/hw/acpi/acpi.h      |   2 +
> >  include/hw/acpi/aml-build.h |  14 ++-
> >  include/hw/acpi/cst.h       |   8 ++
> >  include/hw/i386/pc.h        |   5 ++
> >  hw/acpi/aml-build.c         |  81 ++++++++++++++---
> >  hw/acpi/core.c              |   2 +-
> >  hw/acpi/cpu.c               |   5 ++
> >  hw/acpi/cpu_hotplug.c       |  11 +--
> >  hw/acpi/cst.c               | 173 ++++++++++++++++++++++++++++++++++++
> >  hw/arm/virt-acpi-build.c    |   2 +-
> >  hw/i386/acpi-build.c        |  10 ++-
> >  hw/acpi/Makefile.objs       |   2 +-
> >  12 files changed, 290 insertions(+), 25 deletions(-)
> >  create mode 100644 include/hw/acpi/cst.h
> >  create mode 100644 hw/acpi/cst.c
> > 

Re: [Qemu-devel] [PATCH hack dontapply v2 0/7] Dynamic _CST generation

[Igor Mammedov]

On Wed, 25 Jul 2018 15:44:37 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jul 25, 2018 at 02:32:11PM +0200, Igor Mammedov wrote:
> > On Tue, 10 Jul 2018 03:01:30 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > Now that basic support for guest CPU PM is upstream, I started looking
> > > for making it migrateable.  Since a VM can be migrated between different
> > > hosts, PM info needs to change each time with move the VM to a different
> > > host.  
> > Considering notification is async, so there will be a window when
> > guest will be using old Cstates on new host. What will happen if
> > machine is migrated to host that doesn't support a Cstate
> > that was used on source when guest was started?  
> 
> My testing shows mwait with a wrong hint works, presumably it just uses
> a lot of power.
> 
> > > This adds infrastructure - based on Load/Unload - to support exactly
> > > that: QEMU generates AML (changing it on migration) and stores it in
> > > reserved memory, OSPM loads _CST from there on demand.  
> > Cool and very tempting idea but I have 2 worries about this approach:
> > 1. How well does it work with Windows based guests?
> >    (I've tried something similar but generating new AML from AML itself
> >     to get rid of our long if/else chains there to make up Notify target name.
> >     I ditched it (unfortunately I don't recall why) )  
> 
> After trying it, I can tell you why - it's a horrid mess of
> unreadable code, with arbitrary restraints on package
> length etc.
in my case it was only 4 character NameString, but Windows probably
wasn't happy or just ignored it.
Considering recent development (TPM series) it might have been issue
with SYSTEM_MEMORY not working properly if used as byte buffer field.

Even if it's an unreadable mess it's still stable mess and very constrained
one within a single firmware blob that came from source.
Hence it's more preferable than split brain in this series.

But I don't think we even need dynamic AML for _CST usecase at all,
existing cpuhp interface should work just fine for it and should be
simpler as all infrastructure is already there.

> > 2. (probably biggest issue) Loading dynamically generated AML
> >    basically would make all AML code ABI, so that static AML
> >    in RAM of old QEMU version would match dynamic generated
> >    one on target side with new QEMU (/me generalizing approach beyond _CST support).
> >    I'd try to keep our AML version less as much as possible
> >    and go this route only if there is no other way to do it.  
> 
> That's a good point, thanks for bringing this up!
> 
> So it seems that we will need to define the ABI, yes. All AML code is
> an over-statement though, there are specific entry points
> we must maintain, right?
Well, I'm rather unsure what and where could break,
hence I'm afraid of a new beast and it probably won't be easy
to convince me that ABI would be able to keep things manageable
and stable.
Considering big amount of AML code that we already have
I'm not confident eye balling during review and testing the latest
firmware/qemu would be sufficient as we suddenly would have exploded
test matrix where firmware in use is a mixed from old/and new parts.

> And that in the end isn't fundamentally different from the ABIs
> mandated by the ACPI spec.
> 
> So I have these ideas to try to mitigate the issues:
> - document the ABI (like we have in the ACPI spec)
> - use a specific prefix for all external calls (like _ for ACPI spec ones)
> - use a specific (different) prefix for all internal loaded calls (like
>   [A-Z] for non-ACPI spec ones)
ACPI spec horrible mess and bad example (even committee isn't able to keep it consistent)
but it's necessary evil (compared to no spec at all).
Adding ABI requirement will complicate coding/reviewing for
already complex AML compared to current non ABI way.
Hence from maintainability POV we should stay away from this approach
if there is a viable alternative.

> 
> Thoughts?
> 
> > > This is a partially functional prototype.
> > > 
> > > What works:
> > >     loading _CST dynamically and reporting it to OSPM
> > > 
> > > What doesn't:
> > >     detecting host configuration and generating correct _CST package
> > >     notifying guest about the change to trigger _CST re-evaluation
> > >     disabling mwait/halt exists as appropriate
> > > 
> > > Michael S. Tsirkin (6):
> > >   acpi: aml: add aml_register()
> > >   acpi: generalize aml_package / aml_varpackage
> > >   acpi: aml_load/aml_unload
> > >   acpi: export acpi_checksum
> > >   acpi: init header without linking
> > >   acpi: aml generation for _CST
> > >   pc: HACK: acpi: tie in _CST object to Processor
> > > 
> > >  include/hw/acpi/acpi.h      |   2 +
> > >  include/hw/acpi/aml-build.h |  14 ++-
> > >  include/hw/acpi/cst.h       |   8 ++
> > >  include/hw/i386/pc.h        |   5 ++
> > >  hw/acpi/aml-build.c         |  81 ++++++++++++++---
> > >  hw/acpi/core.c              |   2 +-
> > >  hw/acpi/cpu.c               |   5 ++
> > >  hw/acpi/cpu_hotplug.c       |  11 +--
> > >  hw/acpi/cst.c               | 173 ++++++++++++++++++++++++++++++++++++
> > >  hw/arm/virt-acpi-build.c    |   2 +-
> > >  hw/i386/acpi-build.c        |  10 ++-
> > >  hw/acpi/Makefile.objs       |   2 +-
> > >  12 files changed, 290 insertions(+), 25 deletions(-)
> > >  create mode 100644 include/hw/acpi/cst.h
> > >  create mode 100644 hw/acpi/cst.c
> > >   
> 

Re: [Qemu-devel] [PATCH hack dontapply v2 0/7] Dynamic _CST generation

[Michael S. Tsirkin]

On Wed, Jul 25, 2018 at 05:53:35PM +0200, Igor Mammedov wrote:
> On Wed, 25 Jul 2018 15:44:37 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jul 25, 2018 at 02:32:11PM +0200, Igor Mammedov wrote:
> > > On Tue, 10 Jul 2018 03:01:30 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >   
> > > > Now that basic support for guest CPU PM is upstream, I started looking
> > > > for making it migrateable.  Since a VM can be migrated between different
> > > > hosts, PM info needs to change each time with move the VM to a different
> > > > host.  
> > > Considering notification is async, so there will be a window when
> > > guest will be using old Cstates on new host. What will happen if
> > > machine is migrated to host that doesn't support a Cstate
> > > that was used on source when guest was started?  
> > 
> > My testing shows mwait with a wrong hint works, presumably it just uses
> > a lot of power.
> > 
> > > > This adds infrastructure - based on Load/Unload - to support exactly
> > > > that: QEMU generates AML (changing it on migration) and stores it in
> > > > reserved memory, OSPM loads _CST from there on demand.  
> > > Cool and very tempting idea but I have 2 worries about this approach:
> > > 1. How well does it work with Windows based guests?
> > >    (I've tried something similar but generating new AML from AML itself
> > >     to get rid of our long if/else chains there to make up Notify target name.
> > >     I ditched it (unfortunately I don't recall why) )  
> > 
> > After trying it, I can tell you why - it's a horrid mess of
> > unreadable code, with arbitrary restraints on package
> > length etc.
> in my case it was only 4 character NameString, but Windows probably
> wasn't happy or just ignored it.
> Considering recent development (TPM series) it might have been issue
> with SYSTEM_MEMORY not working properly if used as byte buffer field.
> 
> Even if it's an unreadable mess it's still stable mess and very constrained
> one within a single firmware blob that came from source.

That's exectly the argument we had for keeping ACPI
generation in bios. It's just not an interface that scales.

> Hence it's more preferable than split brain in this series.
> 
> But I don't think we even need dynamic AML for _CST usecase at all,
> existing cpuhp interface should work just fine for it and should be
> simpler as all infrastructure is already there.

Not sure I get what you mean. Could you post a patch?

> > > 2. (probably biggest issue) Loading dynamically generated AML
> > >    basically would make all AML code ABI, so that static AML
> > >    in RAM of old QEMU version would match dynamic generated
> > >    one on target side with new QEMU (/me generalizing approach beyond _CST support).
> > >    I'd try to keep our AML version less as much as possible
> > >    and go this route only if there is no other way to do it.  
> > 
> > That's a good point, thanks for bringing this up!
> > 
> > So it seems that we will need to define the ABI, yes. All AML code is
> > an over-statement though, there are specific entry points
> > we must maintain, right?
> Well, I'm rather unsure what and where could break,
> hence I'm afraid of a new beast and it probably won't be easy
> to convince me that ABI would be able to keep things manageable
> and stable.
> Considering big amount of AML code that we already have
> I'm not confident eye balling during review and testing the latest
> firmware/qemu would be sufficient as we suddenly would have exploded
> test matrix where firmware in use is a mixed from old/and new parts.

Well there's one exported method so far and it relies on one container
device in static table set which does not sound too hard to keep stable.

Given how simple the dynamic table is, how about just checking it
with a unit test? It is literally a single return statement.
If it returns a valid package, that is all that we
care about. I can write some firmware to test that constraint.


> > And that in the end isn't fundamentally different from the ABIs
> > mandated by the ACPI spec.
> > 
> > So I have these ideas to try to mitigate the issues:
> > - document the ABI (like we have in the ACPI spec)
> > - use a specific prefix for all external calls (like _ for ACPI spec ones)
> > - use a specific (different) prefix for all internal loaded calls (like
> >   [A-Z] for non-ACPI spec ones)
> ACPI spec horrible mess and bad example (even committee isn't able to keep it consistent)
> but it's necessary evil (compared to no spec at all).
> Adding ABI requirement will complicate coding/reviewing for
> already complex AML compared to current non ABI way.
> Hence from maintainability POV we should stay away from this approach
> if there is a viable alternative.

I agree. Not that I see one.

> > 
> > Thoughts?
> > 
> > > > This is a partially functional prototype.
> > > > 
> > > > What works:
> > > >     loading _CST dynamically and reporting it to OSPM
> > > > 
> > > > What doesn't:
> > > >     detecting host configuration and generating correct _CST package
> > > >     notifying guest about the change to trigger _CST re-evaluation
> > > >     disabling mwait/halt exists as appropriate
> > > > 
> > > > Michael S. Tsirkin (6):
> > > >   acpi: aml: add aml_register()
> > > >   acpi: generalize aml_package / aml_varpackage
> > > >   acpi: aml_load/aml_unload
> > > >   acpi: export acpi_checksum
> > > >   acpi: init header without linking
> > > >   acpi: aml generation for _CST
> > > >   pc: HACK: acpi: tie in _CST object to Processor
> > > > 
> > > >  include/hw/acpi/acpi.h      |   2 +
> > > >  include/hw/acpi/aml-build.h |  14 ++-
> > > >  include/hw/acpi/cst.h       |   8 ++
> > > >  include/hw/i386/pc.h        |   5 ++
> > > >  hw/acpi/aml-build.c         |  81 ++++++++++++++---
> > > >  hw/acpi/core.c              |   2 +-
> > > >  hw/acpi/cpu.c               |   5 ++
> > > >  hw/acpi/cpu_hotplug.c       |  11 +--
> > > >  hw/acpi/cst.c               | 173 ++++++++++++++++++++++++++++++++++++
> > > >  hw/arm/virt-acpi-build.c    |   2 +-
> > > >  hw/i386/acpi-build.c        |  10 ++-
> > > >  hw/acpi/Makefile.objs       |   2 +-
> > > >  12 files changed, 290 insertions(+), 25 deletions(-)
> > > >  create mode 100644 include/hw/acpi/cst.h
> > > >  create mode 100644 hw/acpi/cst.c
> > > >   
> > 

Re: [Qemu-devel] [PATCH hack dontapply v2 0/7] Dynamic _CST generation

[Igor Mammedov]

On Thu, 26 Jul 2018 19:09:22 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jul 25, 2018 at 05:53:35PM +0200, Igor Mammedov wrote:
> > On Wed, 25 Jul 2018 15:44:37 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Wed, Jul 25, 2018 at 02:32:11PM +0200, Igor Mammedov wrote:  
> > > > On Tue, 10 Jul 2018 03:01:30 +0300
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >     
> > > > > Now that basic support for guest CPU PM is upstream, I started looking
> > > > > for making it migrateable.  Since a VM can be migrated between different
> > > > > hosts, PM info needs to change each time with move the VM to a different
> > > > > host.    
> > > > Considering notification is async, so there will be a window when
> > > > guest will be using old Cstates on new host. What will happen if
> > > > machine is migrated to host that doesn't support a Cstate
> > > > that was used on source when guest was started?    
> > > 
> > > My testing shows mwait with a wrong hint works, presumably it just uses
> > > a lot of power.
> > >   
> > > > > This adds infrastructure - based on Load/Unload - to support exactly
> > > > > that: QEMU generates AML (changing it on migration) and stores it in
> > > > > reserved memory, OSPM loads _CST from there on demand.    
> > > > Cool and very tempting idea but I have 2 worries about this approach:
> > > > 1. How well does it work with Windows based guests?
> > > >    (I've tried something similar but generating new AML from AML itself
> > > >     to get rid of our long if/else chains there to make up Notify target name.
> > > >     I ditched it (unfortunately I don't recall why) )    
> > > 
> > > After trying it, I can tell you why - it's a horrid mess of
> > > unreadable code, with arbitrary restraints on package
> > > length etc.  
> > in my case it was only 4 character NameString, but Windows probably
> > wasn't happy or just ignored it.
> > Considering recent development (TPM series) it might have been issue
> > with SYSTEM_MEMORY not working properly if used as byte buffer field.
> > 
> > Even if it's an unreadable mess it's still stable mess and very constrained
> > one within a single firmware blob that came from source.  
> 
> That's exectly the argument we had for keeping ACPI
> generation in bios. It's just not an interface that scales.
> 
> > Hence it's more preferable than split brain in this series.
> > 
> > But I don't think we even need dynamic AML for _CST usecase at all,
> > existing cpuhp interface should work just fine for it and should be
> > simpler as all infrastructure is already there.  
> 
> Not sure I get what you mean. Could you post a patch?
> 
> > > > 2. (probably biggest issue) Loading dynamically generated AML
> > > >    basically would make all AML code ABI, so that static AML
> > > >    in RAM of old QEMU version would match dynamic generated
> > > >    one on target side with new QEMU (/me generalizing approach beyond _CST support).
> > > >    I'd try to keep our AML version less as much as possible
> > > >    and go this route only if there is no other way to do it.    
> > > 
> > > That's a good point, thanks for bringing this up!
> > > 
> > > So it seems that we will need to define the ABI, yes. All AML code is
> > > an over-statement though, there are specific entry points
> > > we must maintain, right?  
> > Well, I'm rather unsure what and where could break,
> > hence I'm afraid of a new beast and it probably won't be easy
> > to convince me that ABI would be able to keep things manageable
> > and stable.
> > Considering big amount of AML code that we already have
> > I'm not confident eye balling during review and testing the latest
> > firmware/qemu would be sufficient as we suddenly would have exploded
> > test matrix where firmware in use is a mixed from old/and new parts.  
> 
> Well there's one exported method so far and it relies on one container
> device in static table set which does not sound too hard to keep stable.
> 
> Given how simple the dynamic table is, how about just checking it
> with a unit test? It is literally a single return statement.
> If it returns a valid package, that is all that we
> care about. I can write some firmware to test that constraint.
> 
> 
> > > And that in the end isn't fundamentally different from the ABIs
> > > mandated by the ACPI spec.
> > > 
> > > So I have these ideas to try to mitigate the issues:
> > > - document the ABI (like we have in the ACPI spec)
> > > - use a specific prefix for all external calls (like _ for ACPI spec ones)
> > > - use a specific (different) prefix for all internal loaded calls (like
> > >   [A-Z] for non-ACPI spec ones)  
> > ACPI spec horrible mess and bad example (even committee isn't able to keep it consistent)
> > but it's necessary evil (compared to no spec at all).
> > Adding ABI requirement will complicate coding/reviewing for
> > already complex AML compared to current non ABI way.
> > Hence from maintainability POV we should stay away from this approach
> > if there is a viable alternative.  
> 
> I agree. Not that I see one.

I'll try to come up with a patch to make use of cpuhp registers
to pass CST values and it's AML counterpart.
Then you'd need to just wire it up to whatever means you use
to get these values from host.

BTW:
I've noticed you have used SystemIO registers in CST
(my patch used FFH registers) using mmio defeats purpose
as it causes guest exit instead of mwait that we are targeting.

Is it just for demo purpose of RFC with new AML interface?

> 
> > > 
> > > Thoughts?
> > >   
> > > > > This is a partially functional prototype.
> > > > > 
> > > > > What works:
> > > > >     loading _CST dynamically and reporting it to OSPM
> > > > > 
> > > > > What doesn't:
> > > > >     detecting host configuration and generating correct _CST package
> > > > >     notifying guest about the change to trigger _CST re-evaluation
> > > > >     disabling mwait/halt exists as appropriate
> > > > > 
> > > > > Michael S. Tsirkin (6):
> > > > >   acpi: aml: add aml_register()
> > > > >   acpi: generalize aml_package / aml_varpackage
> > > > >   acpi: aml_load/aml_unload
> > > > >   acpi: export acpi_checksum
> > > > >   acpi: init header without linking
> > > > >   acpi: aml generation for _CST
> > > > >   pc: HACK: acpi: tie in _CST object to Processor
> > > > > 
> > > > >  include/hw/acpi/acpi.h      |   2 +
> > > > >  include/hw/acpi/aml-build.h |  14 ++-
> > > > >  include/hw/acpi/cst.h       |   8 ++
> > > > >  include/hw/i386/pc.h        |   5 ++
> > > > >  hw/acpi/aml-build.c         |  81 ++++++++++++++---
> > > > >  hw/acpi/core.c              |   2 +-
> > > > >  hw/acpi/cpu.c               |   5 ++
> > > > >  hw/acpi/cpu_hotplug.c       |  11 +--
> > > > >  hw/acpi/cst.c               | 173 ++++++++++++++++++++++++++++++++++++
> > > > >  hw/arm/virt-acpi-build.c    |   2 +-
> > > > >  hw/i386/acpi-build.c        |  10 ++-
> > > > >  hw/acpi/Makefile.objs       |   2 +-
> > > > >  12 files changed, 290 insertions(+), 25 deletions(-)
> > > > >  create mode 100644 include/hw/acpi/cst.h
> > > > >  create mode 100644 hw/acpi/cst.c
> > > > >     
> > >   

Re: [Qemu-devel] [PATCH hack dontapply v2 0/7] Dynamic _CST generation

[Michael S. Tsirkin]

On Thu, Aug 02, 2018 at 11:18:08AM +0200, Igor Mammedov wrote:
> On Thu, 26 Jul 2018 19:09:22 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jul 25, 2018 at 05:53:35PM +0200, Igor Mammedov wrote:
> > > On Wed, 25 Jul 2018 15:44:37 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >   
> > > > On Wed, Jul 25, 2018 at 02:32:11PM +0200, Igor Mammedov wrote:  
> > > > > On Tue, 10 Jul 2018 03:01:30 +0300
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > >     
> > > > > > Now that basic support for guest CPU PM is upstream, I started looking
> > > > > > for making it migrateable.  Since a VM can be migrated between different
> > > > > > hosts, PM info needs to change each time with move the VM to a different
> > > > > > host.    
> > > > > Considering notification is async, so there will be a window when
> > > > > guest will be using old Cstates on new host. What will happen if
> > > > > machine is migrated to host that doesn't support a Cstate
> > > > > that was used on source when guest was started?    
> > > > 
> > > > My testing shows mwait with a wrong hint works, presumably it just uses
> > > > a lot of power.
> > > >   
> > > > > > This adds infrastructure - based on Load/Unload - to support exactly
> > > > > > that: QEMU generates AML (changing it on migration) and stores it in
> > > > > > reserved memory, OSPM loads _CST from there on demand.    
> > > > > Cool and very tempting idea but I have 2 worries about this approach:
> > > > > 1. How well does it work with Windows based guests?
> > > > >    (I've tried something similar but generating new AML from AML itself
> > > > >     to get rid of our long if/else chains there to make up Notify target name.
> > > > >     I ditched it (unfortunately I don't recall why) )    
> > > > 
> > > > After trying it, I can tell you why - it's a horrid mess of
> > > > unreadable code, with arbitrary restraints on package
> > > > length etc.  
> > > in my case it was only 4 character NameString, but Windows probably
> > > wasn't happy or just ignored it.
> > > Considering recent development (TPM series) it might have been issue
> > > with SYSTEM_MEMORY not working properly if used as byte buffer field.
> > > 
> > > Even if it's an unreadable mess it's still stable mess and very constrained
> > > one within a single firmware blob that came from source.  
> > 
> > That's exectly the argument we had for keeping ACPI
> > generation in bios. It's just not an interface that scales.
> > 
> > > Hence it's more preferable than split brain in this series.
> > > 
> > > But I don't think we even need dynamic AML for _CST usecase at all,
> > > existing cpuhp interface should work just fine for it and should be
> > > simpler as all infrastructure is already there.  
> > 
> > Not sure I get what you mean. Could you post a patch?
> > 
> > > > > 2. (probably biggest issue) Loading dynamically generated AML
> > > > >    basically would make all AML code ABI, so that static AML
> > > > >    in RAM of old QEMU version would match dynamic generated
> > > > >    one on target side with new QEMU (/me generalizing approach beyond _CST support).
> > > > >    I'd try to keep our AML version less as much as possible
> > > > >    and go this route only if there is no other way to do it.    
> > > > 
> > > > That's a good point, thanks for bringing this up!
> > > > 
> > > > So it seems that we will need to define the ABI, yes. All AML code is
> > > > an over-statement though, there are specific entry points
> > > > we must maintain, right?  
> > > Well, I'm rather unsure what and where could break,
> > > hence I'm afraid of a new beast and it probably won't be easy
> > > to convince me that ABI would be able to keep things manageable
> > > and stable.
> > > Considering big amount of AML code that we already have
> > > I'm not confident eye balling during review and testing the latest
> > > firmware/qemu would be sufficient as we suddenly would have exploded
> > > test matrix where firmware in use is a mixed from old/and new parts.  
> > 
> > Well there's one exported method so far and it relies on one container
> > device in static table set which does not sound too hard to keep stable.
> > 
> > Given how simple the dynamic table is, how about just checking it
> > with a unit test? It is literally a single return statement.
> > If it returns a valid package, that is all that we
> > care about. I can write some firmware to test that constraint.
> > 
> > 
> > > > And that in the end isn't fundamentally different from the ABIs
> > > > mandated by the ACPI spec.
> > > > 
> > > > So I have these ideas to try to mitigate the issues:
> > > > - document the ABI (like we have in the ACPI spec)
> > > > - use a specific prefix for all external calls (like _ for ACPI spec ones)
> > > > - use a specific (different) prefix for all internal loaded calls (like
> > > >   [A-Z] for non-ACPI spec ones)  
> > > ACPI spec horrible mess and bad example (even committee isn't able to keep it consistent)
> > > but it's necessary evil (compared to no spec at all).
> > > Adding ABI requirement will complicate coding/reviewing for
> > > already complex AML compared to current non ABI way.
> > > Hence from maintainability POV we should stay away from this approach
> > > if there is a viable alternative.  
> > 
> > I agree. Not that I see one.
> 
> I'll try to come up with a patch to make use of cpuhp registers
> to pass CST values and it's AML counterpart.
> Then you'd need to just wire it up to whatever means you use
> to get these values from host.
> 
> BTW:
> I've noticed you have used SystemIO registers in CST
> (my patch used FFH registers) using mmio defeats purpose
> as it causes guest exit instead of mwait that we are targeting.
> 
> Is it just for demo purpose of RFC with new AML interface?

Just a demo. Actual mwait will need linux changes to make linux obey acpi
if it conflicts with cpuid.

> > 
> > > > 
> > > > Thoughts?
> > > >   
> > > > > > This is a partially functional prototype.
> > > > > > 
> > > > > > What works:
> > > > > >     loading _CST dynamically and reporting it to OSPM
> > > > > > 
> > > > > > What doesn't:
> > > > > >     detecting host configuration and generating correct _CST package
> > > > > >     notifying guest about the change to trigger _CST re-evaluation
> > > > > >     disabling mwait/halt exists as appropriate
> > > > > > 
> > > > > > Michael S. Tsirkin (6):
> > > > > >   acpi: aml: add aml_register()
> > > > > >   acpi: generalize aml_package / aml_varpackage
> > > > > >   acpi: aml_load/aml_unload
> > > > > >   acpi: export acpi_checksum
> > > > > >   acpi: init header without linking
> > > > > >   acpi: aml generation for _CST
> > > > > >   pc: HACK: acpi: tie in _CST object to Processor
> > > > > > 
> > > > > >  include/hw/acpi/acpi.h      |   2 +
> > > > > >  include/hw/acpi/aml-build.h |  14 ++-
> > > > > >  include/hw/acpi/cst.h       |   8 ++
> > > > > >  include/hw/i386/pc.h        |   5 ++
> > > > > >  hw/acpi/aml-build.c         |  81 ++++++++++++++---
> > > > > >  hw/acpi/core.c              |   2 +-
> > > > > >  hw/acpi/cpu.c               |   5 ++
> > > > > >  hw/acpi/cpu_hotplug.c       |  11 +--
> > > > > >  hw/acpi/cst.c               | 173 ++++++++++++++++++++++++++++++++++++
> > > > > >  hw/arm/virt-acpi-build.c    |   2 +-
> > > > > >  hw/i386/acpi-build.c        |  10 ++-
> > > > > >  hw/acpi/Makefile.objs       |   2 +-
> > > > > >  12 files changed, 290 insertions(+), 25 deletions(-)
> > > > > >  create mode 100644 include/hw/acpi/cst.h
> > > > > >  create mode 100644 hw/acpi/cst.c
> > > > > >     
> > > >   

Re: [Qemu-devel] [PATCH hack dontapply v2 0/7] Dynamic _CST generation

[Igor Mammedov]

On Thu, 2 Aug 2018 11:18:08 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Thu, 26 Jul 2018 19:09:22 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jul 25, 2018 at 05:53:35PM +0200, Igor Mammedov wrote:  
> > > On Wed, 25 Jul 2018 15:44:37 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >     
> > > > On Wed, Jul 25, 2018 at 02:32:11PM +0200, Igor Mammedov wrote:    
> > > > > On Tue, 10 Jul 2018 03:01:30 +0300
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > >       
> > > > > > Now that basic support for guest CPU PM is upstream, I started looking
> > > > > > for making it migrateable.  Since a VM can be migrated between different
> > > > > > hosts, PM info needs to change each time with move the VM to a different
> > > > > > host.      
> > > > > Considering notification is async, so there will be a window when
> > > > > guest will be using old Cstates on new host. What will happen if
> > > > > machine is migrated to host that doesn't support a Cstate
> > > > > that was used on source when guest was started?      
> > > > 
> > > > My testing shows mwait with a wrong hint works, presumably it just uses
> > > > a lot of power.
> > > >     
> > > > > > This adds infrastructure - based on Load/Unload - to support exactly
> > > > > > that: QEMU generates AML (changing it on migration) and stores it in
> > > > > > reserved memory, OSPM loads _CST from there on demand.      
> > > > > Cool and very tempting idea but I have 2 worries about this approach:
> > > > > 1. How well does it work with Windows based guests?
> > > > >    (I've tried something similar but generating new AML from AML itself
> > > > >     to get rid of our long if/else chains there to make up Notify target name.
> > > > >     I ditched it (unfortunately I don't recall why) )      
> > > > 
> > > > After trying it, I can tell you why - it's a horrid mess of
> > > > unreadable code, with arbitrary restraints on package
> > > > length etc.    
> > > in my case it was only 4 character NameString, but Windows probably
> > > wasn't happy or just ignored it.
> > > Considering recent development (TPM series) it might have been issue
> > > with SYSTEM_MEMORY not working properly if used as byte buffer field.
> > > 
> > > Even if it's an unreadable mess it's still stable mess and very constrained
> > > one within a single firmware blob that came from source.    
> > 
> > That's exectly the argument we had for keeping ACPI
> > generation in bios. It's just not an interface that scales.
> >   
> > > Hence it's more preferable than split brain in this series.
> > > 
> > > But I don't think we even need dynamic AML for _CST usecase at all,
> > > existing cpuhp interface should work just fine for it and should be
> > > simpler as all infrastructure is already there.    
> > 
> > Not sure I get what you mean. Could you post a patch?
> >   
> > > > > 2. (probably biggest issue) Loading dynamically generated AML
> > > > >    basically would make all AML code ABI, so that static AML
> > > > >    in RAM of old QEMU version would match dynamic generated
> > > > >    one on target side with new QEMU (/me generalizing approach beyond _CST support).
> > > > >    I'd try to keep our AML version less as much as possible
> > > > >    and go this route only if there is no other way to do it.      
> > > > 
> > > > That's a good point, thanks for bringing this up!
> > > > 
> > > > So it seems that we will need to define the ABI, yes. All AML code is
> > > > an over-statement though, there are specific entry points
> > > > we must maintain, right?    
> > > Well, I'm rather unsure what and where could break,
> > > hence I'm afraid of a new beast and it probably won't be easy
> > > to convince me that ABI would be able to keep things manageable
> > > and stable.
> > > Considering big amount of AML code that we already have
> > > I'm not confident eye balling during review and testing the latest
> > > firmware/qemu would be sufficient as we suddenly would have exploded
> > > test matrix where firmware in use is a mixed from old/and new parts.    
> > 
> > Well there's one exported method so far and it relies on one container
> > device in static table set which does not sound too hard to keep stable.
> > 
> > Given how simple the dynamic table is, how about just checking it
> > with a unit test? It is literally a single return statement.
> > If it returns a valid package, that is all that we
> > care about. I can write some firmware to test that constraint.
> > 
> >   
> > > > And that in the end isn't fundamentally different from the ABIs
> > > > mandated by the ACPI spec.
> > > > 
> > > > So I have these ideas to try to mitigate the issues:
> > > > - document the ABI (like we have in the ACPI spec)
> > > > - use a specific prefix for all external calls (like _ for ACPI spec ones)
> > > > - use a specific (different) prefix for all internal loaded calls (like
> > > >   [A-Z] for non-ACPI spec ones)    
> > > ACPI spec horrible mess and bad example (even committee isn't able to keep it consistent)
> > > but it's necessary evil (compared to no spec at all).
> > > Adding ABI requirement will complicate coding/reviewing for
> > > already complex AML compared to current non ABI way.
> > > Hence from maintainability POV we should stay away from this approach
> > > if there is a viable alternative.    
> > 
> > I agree. Not that I see one.  
> 
> I'll try to come up with a patch to make use of cpuhp registers
> to pass CST values and it's AML counterpart.
> Then you'd need to just wire it up to whatever means you use
> to get these values from host.
Posted a non dynamic variant
  [RFC PATCH 0/4] "pc: acpi: _CST support"
with uses conventional approach to compose _CST object
without opening AML to ABI issues

[...]