[Qemu-devel] [RFC PATCH 0/4] "pc: acpi: _CST support"

[Qemu-devel] [RFC PATCH 0/4] "pc: acpi: _CST support"

[Igor Mammedov]

It's an alternative approach to
 1) [PATCH hack dontapply v2 0/7] Dynamic _CST generation
which instead of dynamic AML loading uses static AML with
dynamic values.  It allows us to keep firmware blob static and
to avoid split firmware issue (1) in case of cross version migration.

ABI in this case is confined to cpu hotplug IO registers
(i.e. do it old school way, like we used to do so far).
This way we don't have to add yet another ABI to keep dynamic
AML code under control (1).

Tested  with: XPsp3 - ws2106 guests.

CC: "Michael S. Tsirkin" <mst@redhat.com>


Igor Mammedov (3):
  acpi: add aml_create_byte_field()
  pc: acpi: add _CST support
  acpi: add support for CST update notification

Michael S. Tsirkin (1):
  acpi: aml: add aml_register()

 include/hw/acpi/aml-build.h     |   6 ++
 include/hw/acpi/cpu.h           |  10 +++
 docs/specs/acpi_cpu_hotplug.txt |  21 +++++-
 hw/acpi/aml-build.c             |  28 +++++++
 hw/acpi/cpu.c                   | 158 +++++++++++++++++++++++++++++++++++++++-
 hw/acpi/piix4.c                 |   2 +
 hw/i386/acpi-build.c            |   5 +-
 tests/bios-tables-test.c        |   1 +
 8 files changed, 225 insertions(+), 6 deletions(-)

-- 
2.7.4

[Qemu-devel] [RFC PATCH 1/4] acpi: aml: add aml_register()

[Igor Mammedov]

From: "Michael S. Tsirkin" <mst@redhat.com>

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 6c36903..10c7946 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 1e43cd7..def62b3 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)
 {
-- 
2.7.4

[Qemu-devel] [RFC PATCH 2/4] acpi: add aml_create_byte_field()

[Igor Mammedov]

will be used by for packing _CST package in follow up patch

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/acpi/aml-build.h | 1 +
 hw/acpi/aml-build.c         | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 10c7946..8c8ca0b 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -371,6 +371,7 @@ Aml *aml_release(Aml *mutex);
 Aml *aml_alias(const char *source_object, const char *alias_object);
 Aml *aml_create_field(Aml *srcbuf, Aml *bit_index, Aml *num_bits,
                       const char *name);
+Aml *aml_create_byte_field(Aml *srcbuf, Aml *index, 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);
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index def62b3..977d929 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1094,6 +1094,13 @@ Aml *aml_create_field(Aml *srcbuf, Aml *bit_index, Aml *num_bits,
     return var;
 }
 
+/* ACPI 1.0b: 16.2.5.2 Named Objects Encoding: DefCreateByteField */
+Aml *aml_create_byte_field(Aml *srcbuf, Aml *index, const char *name)
+{
+    return create_field_common(0x8C /* CreateByteFieldOp */,
+                               srcbuf, index, name);
+}
+
 /* ACPI 1.0b: 16.2.5.2 Named Objects Encoding: DefCreateDWordField */
 Aml *aml_create_dword_field(Aml *srcbuf, Aml *index, const char *name)
 {
-- 
2.7.4

[Qemu-devel] [RFC PATCH 3/4] pc: acpi: add _CST support

[Igor Mammedov]

Reuse CPU hotplug IO registers for passing a CST entry
containing package for shalowest C1 using mwait and
read it out in guest with new CCST AML method.

The CState support is optional and could be turned on
with '-global PIIX4_PM.cstate=on' CLI option.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
for demo purposes it's wired only to piix4
TODO: q35 wiring

'tested' with rhel7 and XPsp3 - WS2016
 (i.e. it boots and all windows versions happy about AML qemu produces)
---
 include/hw/acpi/cpu.h           |   9 +++
 docs/specs/acpi_cpu_hotplug.txt |  10 ++-
 hw/acpi/cpu.c                   | 131 ++++++++++++++++++++++++++++++++++++++++
 hw/acpi/piix4.c                 |   2 +
 hw/i386/acpi-build.c            |   5 +-
 tests/bios-tables-test.c        |   1 +
 6 files changed, 156 insertions(+), 2 deletions(-)

diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
index 89ce172..eb79cbf 100644
--- a/include/hw/acpi/cpu.h
+++ b/include/hw/acpi/cpu.h
@@ -17,6 +17,12 @@
 #include "hw/acpi/aml-build.h"
 #include "hw/hotplug.h"
 
+typedef struct AcpiCState {
+    uint32_t current_cst_field;
+    uint32_t latency;
+    uint32_t power;
+} AcpiCState;
+
 typedef struct AcpiCpuStatus {
     struct CPUState *cpu;
     uint64_t arch_id;
@@ -24,6 +30,7 @@ typedef struct AcpiCpuStatus {
     bool is_removing;
     uint32_t ost_event;
     uint32_t ost_status;
+    AcpiCState cst;
 } AcpiCpuStatus;
 
 typedef struct CPUHotplugState {
@@ -32,6 +39,7 @@ typedef struct CPUHotplugState {
     uint8_t command;
     uint32_t dev_count;
     AcpiCpuStatus *devs;
+    bool enable_cstate;
 } CPUHotplugState;
 
 void acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
@@ -50,6 +58,7 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
 typedef struct CPUHotplugFeatures {
     bool apci_1_compatible;
     bool has_legacy_cphp;
+    bool cstate_enabled;
 } CPUHotplugFeatures;
 
 void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
index ee219c8..adfb026 100644
--- a/docs/specs/acpi_cpu_hotplug.txt
+++ b/docs/specs/acpi_cpu_hotplug.txt
@@ -47,6 +47,12 @@ read access:
           in case of error or unsupported command reads is 0xFFFFFFFF
           current 'Command field' value:
               0: returns PXM value corresponding to device
+              3: sequential reads return a sequence of DWORDs
+                   {
+                     AddressSpaceKeyword, RegisterBitWidth, RegisterBitOffset,
+                     RegisterAddress Lo, RegisterAddress Hi, AccessSize,
+                     C State type, Latency, Power,
+                   }
 
 write access:
     offset:
@@ -75,7 +81,9 @@ write access:
             1: following writes to 'Command data' register set OST event
                register in QEMU
             2: following writes to 'Command data' register set OST status
-               register in QEMU
+            3: following reads from 'Command data' register return Cx
+               state (command execution resets unread field counter to the 1st
+               field).
             other values: reserved
     [0x6-0x7] reserved
     [0x8] Command data: (DWORD access)
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 5ae595e..7ef04f9 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -16,6 +16,7 @@ enum {
     CPHP_GET_NEXT_CPU_WITH_EVENT_CMD = 0,
     CPHP_OST_EVENT_CMD = 1,
     CPHP_OST_STATUS_CMD = 2,
+    CPHP_READ_CST_CMD = 3,
     CPHP_CMD_MAX
 };
 
@@ -73,6 +74,41 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr, unsigned size)
         case CPHP_GET_NEXT_CPU_WITH_EVENT_CMD:
            val = cpu_st->selector;
            break;
+        case CPHP_READ_CST_CMD:
+            switch (cdev->cst.current_cst_field) {
+            case 0:
+                val = cpu_to_le32(AML_AS_FFH); /* AddressSpaceKeyword */
+                break;
+            case 1:  /* RegisterBitWidth */
+                val = cpu_to_le32(1); /* Vendor: Intel */
+                break;
+            case 2:  /* RegisterBitOffset */
+                val = cpu_to_le32(2); /* Class: Native C State Instruction */
+                break;
+            case 3:  /* RegisterAddress Lo */
+                val = cpu_to_le64(0); /* Arg0: mwait EAX hint */
+                break;
+            case 4:  /* RegisterAddress Hi */
+                val = cpu_to_le32(0); /* Reserved */
+                break;
+            case 5:  /* AccessSize */
+                val = cpu_to_le32(0); /* Arg1 */
+                break;
+            case 6:
+                val = cpu_to_le32(1); /* The C State type C1*/
+                break;
+            case 7:
+                val = cpu_to_le32(cdev->cst.latency);
+                break;
+            case 8:
+                val = cpu_to_le32(cdev->cst.power);
+                break;
+            default:
+                val = 0xFFFFFFFF;
+               break;
+            }
+            cdev->cst.current_cst_field++;
+            break;
         default:
            break;
         }
@@ -145,6 +181,9 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr, uint64_t data,
                     }
                     iter = iter + 1 < cpu_st->dev_count ? iter + 1 : 0;
                 } while (iter != cpu_st->selector);
+            } else if (cpu_st->command == CPHP_READ_CST_CMD) {
+                cdev = &cpu_st->devs[cpu_st->selector];
+                cdev->cst.current_cst_field = 0;
             }
         }
         break;
@@ -265,6 +304,36 @@ void acpi_cpu_unplug_cb(CPUHotplugState *cpu_st,
     cdev->cpu = NULL;
 }
 
+static const VMStateDescription vmstate_cstate_sts = {
+    .name = "CState",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields      = (VMStateField[]) {
+        VMSTATE_UINT32(cst.current_cst_field, AcpiCpuStatus),
+        VMSTATE_UINT32(cst.latency, AcpiCpuStatus),
+        VMSTATE_UINT32(cst.power, AcpiCpuStatus),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static bool vmstate_test_use_cst(void *opaque)
+{
+    CPUHotplugState *s = opaque;
+    return s->enable_cstate;
+}
+
+static const VMStateDescription vmstate_cstates = {
+    .name = "CPU hotplug state/CStates",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = vmstate_test_use_cst,
+    .fields      = (VMStateField[]) {
+        VMSTATE_STRUCT_VARRAY_POINTER_UINT32(devs, CPUHotplugState, dev_count,
+                                             vmstate_cstate_sts, AcpiCpuStatus),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 static const VMStateDescription vmstate_cpuhp_sts = {
     .name = "CPU hotplug device state",
     .version_id = 1,
@@ -290,6 +359,10 @@ const VMStateDescription vmstate_cpu_hotplug = {
         VMSTATE_STRUCT_VARRAY_POINTER_UINT32(devs, CPUHotplugState, dev_count,
                                              vmstate_cpuhp_sts, AcpiCpuStatus),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (const VMStateDescription * []) {
+       &vmstate_cstates,
+       NULL
     }
 };
 
@@ -301,6 +374,7 @@ const VMStateDescription vmstate_cpu_hotplug = {
 #define CPU_NOTIFY_METHOD "CTFY"
 #define CPU_EJECT_METHOD  "CEJ0"
 #define CPU_OST_METHOD    "COST"
+#define CPU_CST_METHOD    "CCST"
 
 #define CPU_ENABLED       "CPEN"
 #define CPU_SELECTOR      "CSEL"
@@ -501,6 +575,57 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
         }
         aml_append(cpus_dev, method);
 
+        if (opts.cstate_enabled) {
+            Aml *crs;
+            Aml *pkg = aml_local(0);
+            Aml *cst = aml_local(1);
+            Aml *cst_cmd = aml_int(CPHP_READ_CST_CMD);
+            Aml *uid = aml_arg(0);
+            Aml *nm = aml_name("CCRS");
+
+            method = aml_method(CPU_CST_METHOD, 1, AML_SERIALIZED);
+            /* Package to hold 1 CST entry */
+            aml_append(method, aml_store(aml_package(2), pkg));
+            aml_append(method, aml_store(aml_package(4), cst)); /* CST entry */
+
+            aml_append(method, aml_acquire(ctrl_lock, 0xFFFF));
+            aml_append(method, aml_store(uid, cpu_selector));
+            aml_append(method, aml_store(cst_cmd, cpu_cmd));
+
+            /* create register template to fill in */
+            crs = aml_resource_template();
+            aml_append(crs, aml_register(AML_AS_FFH, 0, 0, 0, 0));
+            aml_append(method, aml_name_decl("CCRS", crs));
+
+            /* fill in actual register values */
+            aml_append(method, aml_create_byte_field(nm, aml_int(3), "_ASI"));
+            aml_append(method, aml_store(cpu_data, aml_name("_ASI")));
+            aml_append(method, aml_create_byte_field(nm, aml_int(4), "_RBW"));
+            aml_append(method, aml_store(cpu_data, aml_name("_RBW")));
+            aml_append(method, aml_create_byte_field(nm, aml_int(5), "_RBO"));
+            aml_append(method, aml_store(cpu_data, aml_name("_RBO")));
+            aml_append(method, aml_create_dword_field(nm, aml_int(7), "LADR"));
+            aml_append(method, aml_store(cpu_data, aml_name("LADR")));
+            aml_append(method, aml_create_dword_field(nm, aml_int(11), "HADR"));
+            aml_append(method, aml_store(cpu_data, aml_name("HADR")));
+            aml_append(method, aml_create_byte_field(nm, aml_int(6), "_ASZ"));
+            aml_append(method, aml_store(cpu_data, aml_name("_ASZ")));
+
+            /* pack CST entry */
+            aml_append(method, aml_store(crs, aml_index(cst, zero)));
+            aml_append(method, aml_store(cpu_data, aml_index(cst, one)));
+            aml_append(method, aml_store(cpu_data, aml_index(cst, aml_int(2))));
+            aml_append(method, aml_store(cpu_data, aml_index(cst, aml_int(3))));
+            aml_append(method, aml_release(ctrl_lock));
+
+            /* prepare _CST descriptor with 1 CST entry */
+            aml_append(method, aml_store(one, aml_index(pkg, zero)));
+            aml_append(method, aml_store(cst, aml_index(pkg, one)));
+
+            aml_append(method, aml_return(pkg));
+            aml_append(cpus_dev, method);
+        }
+
         /* build Processor object for each processor */
         for (i = 0; i < arch_ids->len; i++) {
             Aml *dev;
@@ -520,6 +645,12 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
             aml_append(method, aml_return(aml_call1(CPU_STS_METHOD, uid)));
             aml_append(dev, method);
 
+            if (opts.cstate_enabled) {
+                method = aml_method("_CST", 0, AML_SERIALIZED);
+                aml_append(method, aml_return(aml_call1(CPU_CST_METHOD, uid)));
+                aml_append(dev, method);
+            }
+
             /* build _MAT object */
             assert(adevc && adevc->madt_cpu);
             adevc->madt_cpu(adev, i, arch_ids, madt_buf);
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 6404af5..6d3df17 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -677,6 +677,8 @@ static Property piix4_pm_properties[] = {
                      use_acpi_pci_hotplug, true),
     DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
                      acpi_memory_hotplug.is_enabled, true),
+    DEFINE_PROP_BOOL("cstate", PIIX4PMState,
+                     cpuhp_state.enable_cstate, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index e1ee8ae..dd695bd 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -100,6 +100,7 @@ typedef struct AcpiPmInfo {
     uint16_t cpu_hp_io_base;
     uint16_t pcihp_io_base;
     uint16_t pcihp_io_len;
+    bool cstate_enabled;
 } AcpiPmInfo;
 
 typedef struct AcpiMiscInfo {
@@ -218,6 +219,7 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
     pm->pcihp_bridge_en =
         object_property_get_bool(obj, "acpi-pci-hotplug-with-bridge-support",
                                  NULL);
+    pm->cstate_enabled = object_property_get_bool(obj, "cstate", NULL);
 }
 
 static void acpi_get_misc_info(AcpiMiscInfo *info)
@@ -1840,7 +1842,8 @@ 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,
+            .cstate_enabled = pm->cstate_enabled
         };
         build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
                        "\\_SB.PCI0", "\\_GPE._E02");
diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 4e24930..3c1687e 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -716,6 +716,7 @@ static void test_acpi_piix4_tcg_cphp(void)
     data.machine = MACHINE_PC;
     data.variant = ".cphp";
     test_acpi_one("-smp 2,cores=3,sockets=2,maxcpus=6"
+                  " -global PIIX4_PM.cstate=on"
                   " -numa node -numa node"
                   " -numa dist,src=0,dst=1,val=21",
                   &data);
-- 
2.7.4

Re: [Qemu-devel] [RFC PATCH 3/4] pc: acpi: add _CST support

[Michael S. Tsirkin]

On Wed, Aug 08, 2018 at 05:15:48PM +0200, Igor Mammedov wrote:
> Reuse CPU hotplug IO registers for passing a CST entry
> containing package for shalowest C1 using mwait and
> read it out in guest with new CCST AML method.

I don't see how 1 entry is enough. We need to describe full _CST package so
that guest can do reasonable power management on the CPU.


> The CState support is optional and could be turned on
> with '-global PIIX4_PM.cstate=on' CLI option.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> for demo purposes it's wired only to piix4
> TODO: q35 wiring
> 
> 'tested' with rhel7 and XPsp3 - WS2016
>  (i.e. it boots and all windows versions happy about AML qemu produces)
> ---
>  include/hw/acpi/cpu.h           |   9 +++
>  docs/specs/acpi_cpu_hotplug.txt |  10 ++-
>  hw/acpi/cpu.c                   | 131 ++++++++++++++++++++++++++++++++++++++++
>  hw/acpi/piix4.c                 |   2 +
>  hw/i386/acpi-build.c            |   5 +-
>  tests/bios-tables-test.c        |   1 +
>  6 files changed, 156 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
> index 89ce172..eb79cbf 100644
> --- a/include/hw/acpi/cpu.h
> +++ b/include/hw/acpi/cpu.h
> @@ -17,6 +17,12 @@
>  #include "hw/acpi/aml-build.h"
>  #include "hw/hotplug.h"
>  
> +typedef struct AcpiCState {
> +    uint32_t current_cst_field;
> +    uint32_t latency;
> +    uint32_t power;
> +} AcpiCState;
> +
>  typedef struct AcpiCpuStatus {
>      struct CPUState *cpu;
>      uint64_t arch_id;
> @@ -24,6 +30,7 @@ typedef struct AcpiCpuStatus {
>      bool is_removing;
>      uint32_t ost_event;
>      uint32_t ost_status;
> +    AcpiCState cst;
>  } AcpiCpuStatus;
>  
>  typedef struct CPUHotplugState {
> @@ -32,6 +39,7 @@ typedef struct CPUHotplugState {
>      uint8_t command;
>      uint32_t dev_count;
>      AcpiCpuStatus *devs;
> +    bool enable_cstate;
>  } CPUHotplugState;
>  
>  void acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
> @@ -50,6 +58,7 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
>  typedef struct CPUHotplugFeatures {
>      bool apci_1_compatible;
>      bool has_legacy_cphp;
> +    bool cstate_enabled;
>  } CPUHotplugFeatures;
>  
>  void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
> index ee219c8..adfb026 100644
> --- a/docs/specs/acpi_cpu_hotplug.txt
> +++ b/docs/specs/acpi_cpu_hotplug.txt
> @@ -47,6 +47,12 @@ read access:
>            in case of error or unsupported command reads is 0xFFFFFFFF
>            current 'Command field' value:
>                0: returns PXM value corresponding to device
> +              3: sequential reads return a sequence of DWORDs
> +                   {
> +                     AddressSpaceKeyword, RegisterBitWidth, RegisterBitOffset,
> +                     RegisterAddress Lo, RegisterAddress Hi, AccessSize,
> +                     C State type, Latency, Power,
> +                   }
>  
>  write access:
>      offset:
> @@ -75,7 +81,9 @@ write access:
>              1: following writes to 'Command data' register set OST event
>                 register in QEMU
>              2: following writes to 'Command data' register set OST status
> -               register in QEMU
> +            3: following reads from 'Command data' register return Cx
> +               state (command execution resets unread field counter to the 1st
> +               field).
>              other values: reserved
>      [0x6-0x7] reserved
>      [0x8] Command data: (DWORD access)
> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> index 5ae595e..7ef04f9 100644
> --- a/hw/acpi/cpu.c
> +++ b/hw/acpi/cpu.c
> @@ -16,6 +16,7 @@ enum {
>      CPHP_GET_NEXT_CPU_WITH_EVENT_CMD = 0,
>      CPHP_OST_EVENT_CMD = 1,
>      CPHP_OST_STATUS_CMD = 2,
> +    CPHP_READ_CST_CMD = 3,
>      CPHP_CMD_MAX
>  };
>  
> @@ -73,6 +74,41 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr, unsigned size)
>          case CPHP_GET_NEXT_CPU_WITH_EVENT_CMD:
>             val = cpu_st->selector;
>             break;
> +        case CPHP_READ_CST_CMD:
> +            switch (cdev->cst.current_cst_field) {
> +            case 0:
> +                val = cpu_to_le32(AML_AS_FFH); /* AddressSpaceKeyword */
> +                break;
> +            case 1:  /* RegisterBitWidth */
> +                val = cpu_to_le32(1); /* Vendor: Intel */
> +                break;
> +            case 2:  /* RegisterBitOffset */
> +                val = cpu_to_le32(2); /* Class: Native C State Instruction */
> +                break;
> +            case 3:  /* RegisterAddress Lo */
> +                val = cpu_to_le64(0); /* Arg0: mwait EAX hint */
> +                break;
> +            case 4:  /* RegisterAddress Hi */
> +                val = cpu_to_le32(0); /* Reserved */
> +                break;
> +            case 5:  /* AccessSize */
> +                val = cpu_to_le32(0); /* Arg1 */
> +                break;
> +            case 6:
> +                val = cpu_to_le32(1); /* The C State type C1*/
> +                break;
> +            case 7:
> +                val = cpu_to_le32(cdev->cst.latency);
> +                break;
> +            case 8:
> +                val = cpu_to_le32(cdev->cst.power);
> +                break;
> +            default:
> +                val = 0xFFFFFFFF;
> +               break;
> +            }
> +            cdev->cst.current_cst_field++;
> +            break;
>          default:
>             break;
>          }
> @@ -145,6 +181,9 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr, uint64_t data,
>                      }
>                      iter = iter + 1 < cpu_st->dev_count ? iter + 1 : 0;
>                  } while (iter != cpu_st->selector);
> +            } else if (cpu_st->command == CPHP_READ_CST_CMD) {
> +                cdev = &cpu_st->devs[cpu_st->selector];
> +                cdev->cst.current_cst_field = 0;
>              }
>          }
>          break;
> @@ -265,6 +304,36 @@ void acpi_cpu_unplug_cb(CPUHotplugState *cpu_st,
>      cdev->cpu = NULL;
>  }
>  
> +static const VMStateDescription vmstate_cstate_sts = {
> +    .name = "CState",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields      = (VMStateField[]) {
> +        VMSTATE_UINT32(cst.current_cst_field, AcpiCpuStatus),
> +        VMSTATE_UINT32(cst.latency, AcpiCpuStatus),
> +        VMSTATE_UINT32(cst.power, AcpiCpuStatus),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static bool vmstate_test_use_cst(void *opaque)
> +{
> +    CPUHotplugState *s = opaque;
> +    return s->enable_cstate;
> +}
> +
> +static const VMStateDescription vmstate_cstates = {
> +    .name = "CPU hotplug state/CStates",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = vmstate_test_use_cst,
> +    .fields      = (VMStateField[]) {
> +        VMSTATE_STRUCT_VARRAY_POINTER_UINT32(devs, CPUHotplugState, dev_count,
> +                                             vmstate_cstate_sts, AcpiCpuStatus),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
>  static const VMStateDescription vmstate_cpuhp_sts = {
>      .name = "CPU hotplug device state",
>      .version_id = 1,
> @@ -290,6 +359,10 @@ const VMStateDescription vmstate_cpu_hotplug = {
>          VMSTATE_STRUCT_VARRAY_POINTER_UINT32(devs, CPUHotplugState, dev_count,
>                                               vmstate_cpuhp_sts, AcpiCpuStatus),
>          VMSTATE_END_OF_LIST()
> +    },
> +    .subsections = (const VMStateDescription * []) {
> +       &vmstate_cstates,
> +       NULL
>      }
>  };
>  
> @@ -301,6 +374,7 @@ const VMStateDescription vmstate_cpu_hotplug = {
>  #define CPU_NOTIFY_METHOD "CTFY"
>  #define CPU_EJECT_METHOD  "CEJ0"
>  #define CPU_OST_METHOD    "COST"
> +#define CPU_CST_METHOD    "CCST"
>  
>  #define CPU_ENABLED       "CPEN"
>  #define CPU_SELECTOR      "CSEL"
> @@ -501,6 +575,57 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
>          }
>          aml_append(cpus_dev, method);
>  
> +        if (opts.cstate_enabled) {
> +            Aml *crs;
> +            Aml *pkg = aml_local(0);
> +            Aml *cst = aml_local(1);
> +            Aml *cst_cmd = aml_int(CPHP_READ_CST_CMD);
> +            Aml *uid = aml_arg(0);
> +            Aml *nm = aml_name("CCRS");
> +
> +            method = aml_method(CPU_CST_METHOD, 1, AML_SERIALIZED);
> +            /* Package to hold 1 CST entry */
> +            aml_append(method, aml_store(aml_package(2), pkg));
> +            aml_append(method, aml_store(aml_package(4), cst)); /* CST entry */
> +
> +            aml_append(method, aml_acquire(ctrl_lock, 0xFFFF));
> +            aml_append(method, aml_store(uid, cpu_selector));
> +            aml_append(method, aml_store(cst_cmd, cpu_cmd));
> +
> +            /* create register template to fill in */
> +            crs = aml_resource_template();
> +            aml_append(crs, aml_register(AML_AS_FFH, 0, 0, 0, 0));
> +            aml_append(method, aml_name_decl("CCRS", crs));
> +
> +            /* fill in actual register values */
> +            aml_append(method, aml_create_byte_field(nm, aml_int(3), "_ASI"));
> +            aml_append(method, aml_store(cpu_data, aml_name("_ASI")));
> +            aml_append(method, aml_create_byte_field(nm, aml_int(4), "_RBW"));
> +            aml_append(method, aml_store(cpu_data, aml_name("_RBW")));
> +            aml_append(method, aml_create_byte_field(nm, aml_int(5), "_RBO"));
> +            aml_append(method, aml_store(cpu_data, aml_name("_RBO")));
> +            aml_append(method, aml_create_dword_field(nm, aml_int(7), "LADR"));
> +            aml_append(method, aml_store(cpu_data, aml_name("LADR")));
> +            aml_append(method, aml_create_dword_field(nm, aml_int(11), "HADR"));
> +            aml_append(method, aml_store(cpu_data, aml_name("HADR")));
> +            aml_append(method, aml_create_byte_field(nm, aml_int(6), "_ASZ"));
> +            aml_append(method, aml_store(cpu_data, aml_name("_ASZ")));
> +
> +            /* pack CST entry */
> +            aml_append(method, aml_store(crs, aml_index(cst, zero)));
> +            aml_append(method, aml_store(cpu_data, aml_index(cst, one)));
> +            aml_append(method, aml_store(cpu_data, aml_index(cst, aml_int(2))));
> +            aml_append(method, aml_store(cpu_data, aml_index(cst, aml_int(3))));
> +            aml_append(method, aml_release(ctrl_lock));
> +
> +            /* prepare _CST descriptor with 1 CST entry */
> +            aml_append(method, aml_store(one, aml_index(pkg, zero)));
> +            aml_append(method, aml_store(cst, aml_index(pkg, one)));
> +
> +            aml_append(method, aml_return(pkg));
> +            aml_append(cpus_dev, method);
> +        }
> +
>          /* build Processor object for each processor */
>          for (i = 0; i < arch_ids->len; i++) {
>              Aml *dev;
> @@ -520,6 +645,12 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
>              aml_append(method, aml_return(aml_call1(CPU_STS_METHOD, uid)));
>              aml_append(dev, method);
>  
> +            if (opts.cstate_enabled) {
> +                method = aml_method("_CST", 0, AML_SERIALIZED);
> +                aml_append(method, aml_return(aml_call1(CPU_CST_METHOD, uid)));
> +                aml_append(dev, method);
> +            }
> +
>              /* build _MAT object */
>              assert(adevc && adevc->madt_cpu);
>              adevc->madt_cpu(adev, i, arch_ids, madt_buf);
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 6404af5..6d3df17 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -677,6 +677,8 @@ static Property piix4_pm_properties[] = {
>                       use_acpi_pci_hotplug, true),
>      DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
>                       acpi_memory_hotplug.is_enabled, true),
> +    DEFINE_PROP_BOOL("cstate", PIIX4PMState,
> +                     cpuhp_state.enable_cstate, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index e1ee8ae..dd695bd 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -100,6 +100,7 @@ typedef struct AcpiPmInfo {
>      uint16_t cpu_hp_io_base;
>      uint16_t pcihp_io_base;
>      uint16_t pcihp_io_len;
> +    bool cstate_enabled;
>  } AcpiPmInfo;
>  
>  typedef struct AcpiMiscInfo {
> @@ -218,6 +219,7 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
>      pm->pcihp_bridge_en =
>          object_property_get_bool(obj, "acpi-pci-hotplug-with-bridge-support",
>                                   NULL);
> +    pm->cstate_enabled = object_property_get_bool(obj, "cstate", NULL);
>  }
>  
>  static void acpi_get_misc_info(AcpiMiscInfo *info)
> @@ -1840,7 +1842,8 @@ 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,
> +            .cstate_enabled = pm->cstate_enabled
>          };
>          build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
>                         "\\_SB.PCI0", "\\_GPE._E02");
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 4e24930..3c1687e 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -716,6 +716,7 @@ static void test_acpi_piix4_tcg_cphp(void)
>      data.machine = MACHINE_PC;
>      data.variant = ".cphp";
>      test_acpi_one("-smp 2,cores=3,sockets=2,maxcpus=6"
> +                  " -global PIIX4_PM.cstate=on"
>                    " -numa node -numa node"
>                    " -numa dist,src=0,dst=1,val=21",
>                    &data);
> -- 
> 2.7.4

Re: [Qemu-devel] [RFC PATCH 3/4] pc: acpi: add _CST support

[Igor Mammedov]

On Wed, 8 Aug 2018 23:17:48 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Aug 08, 2018 at 05:15:48PM +0200, Igor Mammedov wrote:
> > Reuse CPU hotplug IO registers for passing a CST entry
> > containing package for shalowest C1 using mwait and
> > read it out in guest with new CCST AML method.  
> 
> I don't see how 1 entry is enough. We need to describe full _CST package so
> that guest can do reasonable power management on the CPU.
I was assuming that CST are to be used for mwait/monitor idle combo
to help reduce latencies/vmexits on cpus wakeup from idle state, hence 1 entry.
It's trivial to add as much as needed entries though.
If you are fine with approach I can respin patch with arbitrary
number of CStates.
 
> > The CState support is optional and could be turned on
> > with '-global PIIX4_PM.cstate=on' CLI option.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
[...]

Re: [Qemu-devel] [RFC PATCH 3/4] pc: acpi: add _CST support

[Michael S. Tsirkin]

On Thu, Aug 09, 2018 at 12:00:11PM +0200, Igor Mammedov wrote:
> On Wed, 8 Aug 2018 23:17:48 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Aug 08, 2018 at 05:15:48PM +0200, Igor Mammedov wrote:
> > > Reuse CPU hotplug IO registers for passing a CST entry
> > > containing package for shalowest C1 using mwait and
> > > read it out in guest with new CCST AML method.  
> > 
> > I don't see how 1 entry is enough. We need to describe full _CST package so
> > that guest can do reasonable power management on the CPU.
> I was assuming that CST are to be used for mwait/monitor idle combo
> to help reduce latencies/vmexits on cpus wakeup from idle state, hence 1 entry.
> It's trivial to add as much as needed entries though.
> If you are fine with approach I can respin patch with arbitrary
> number of CStates.

You will find that the code then becomes very messy, and
dynamic size of the package is one of the things
that makes it messy.


> > > The CState support is optional and could be turned on
> > > with '-global PIIX4_PM.cstate=on' CLI option.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> [...]

Re: [Qemu-devel] [RFC PATCH 3/4] pc: acpi: add _CST support

[Igor Mammedov]

On Fri, 10 Aug 2018 02:01:00 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Aug 09, 2018 at 12:00:11PM +0200, Igor Mammedov wrote:
> > On Wed, 8 Aug 2018 23:17:48 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Wed, Aug 08, 2018 at 05:15:48PM +0200, Igor Mammedov wrote:  
> > > > Reuse CPU hotplug IO registers for passing a CST entry
> > > > containing package for shalowest C1 using mwait and
> > > > read it out in guest with new CCST AML method.    
> > > 
> > > I don't see how 1 entry is enough. We need to describe full _CST package so
> > > that guest can do reasonable power management on the CPU.  
> > I was assuming that CST are to be used for mwait/monitor idle combo
> > to help reduce latencies/vmexits on cpus wakeup from idle state, hence 1 entry.
> > It's trivial to add as much as needed entries though.
> > If you are fine with approach I can respin patch with arbitrary
> > number of CStates.  
> 
> You will find that the code then becomes very messy, and
> dynamic size of the package is one of the things
> that makes it messy.
it turned up like extra ~10LOC, for a loop based impl.
I'll post amended patch here as reply with full update series in my repo.


> 
> > > > The CState support is optional and could be turned on
> > > > with '-global PIIX4_PM.cstate=on' CLI option.
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---  
> > [...]  

[Qemu-devel] [PATCH 3/4] pc: acpi: add _CST support

[Igor Mammedov]

Reuse CPU hotplug IO registers for passing a CST entry
containing package for shalowest C1 using mwait and
read it out in guest with new CCST AML method.

The CState support is optional and could be turned on
with '-global PIIX4_PM.cstate-count=X' CLI option.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
V2:
  - support for multiple CStates (Michael)
    full updated series is at https://github.com/imammedo/qemu/commits/mwait2

for demo purposes it's wired only to piix4
TODO: q35 wiring

'tested' with rhel7 and XPsp3 - WS2016
 (i.e. it boots and all windows versions happy about AML qemu produces)
---
 include/hw/acpi/cpu.h           |   5 ++
 docs/specs/acpi_cpu_hotplug.txt |  12 +++-
 hw/acpi/cpu.c                   | 141 ++++++++++++++++++++++++++++++++++++++++
 hw/acpi/piix4.c                 |   2 +
 hw/i386/acpi-build.c            |   5 +-
 tests/bios-tables-test.c        |   1 +
 6 files changed, 164 insertions(+), 2 deletions(-)

diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
index 89ce172..379605c 100644
--- a/include/hw/acpi/cpu.h
+++ b/include/hw/acpi/cpu.h
@@ -24,6 +24,9 @@ typedef struct AcpiCpuStatus {
     bool is_removing;
     uint32_t ost_event;
     uint32_t ost_status;
+    uint32_t cst_idx;
+    uint32_t cst_count;
+    uint32_t *cst;
 } AcpiCpuStatus;
 
 typedef struct CPUHotplugState {
@@ -32,6 +35,7 @@ typedef struct CPUHotplugState {
     uint8_t command;
     uint32_t dev_count;
     AcpiCpuStatus *devs;
+    uint32_t cst_count;
 } CPUHotplugState;
 
 void acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
@@ -50,6 +54,7 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
 typedef struct CPUHotplugFeatures {
     bool apci_1_compatible;
     bool has_legacy_cphp;
+    uint32_t cst_count;
 } CPUHotplugFeatures;
 
 void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
index ee219c8..c80cdcd 100644
--- a/docs/specs/acpi_cpu_hotplug.txt
+++ b/docs/specs/acpi_cpu_hotplug.txt
@@ -47,6 +47,14 @@ read access:
           in case of error or unsupported command reads is 0xFFFFFFFF
           current 'Command field' value:
               0: returns PXM value corresponding to device
+              3: 1st read, returns number of entries and following sequential
+                 reads return a sequence of tuples. each tuple consists of
+                 DWORDs in following order:
+                   {
+                     AddressSpaceKeyword, RegisterBitWidth, RegisterBitOffset,
+                     RegisterAddress Lo, RegisterAddress Hi, AccessSize,
+                     C State type, Latency, Power,
+                   }
 
 write access:
     offset:
@@ -75,7 +83,9 @@ write access:
             1: following writes to 'Command data' register set OST event
                register in QEMU
             2: following writes to 'Command data' register set OST status
-               register in QEMU
+            3: following reads from 'Command data' register return Cx
+               state (command execution resets unread field counter to the 1st
+               field).
             other values: reserved
     [0x6-0x7] reserved
     [0x8] Command data: (DWORD access)
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 5ae595e..364187b 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -16,6 +16,7 @@ enum {
     CPHP_GET_NEXT_CPU_WITH_EVENT_CMD = 0,
     CPHP_OST_EVENT_CMD = 1,
     CPHP_OST_STATUS_CMD = 2,
+    CPHP_READ_CST_CMD = 3,
     CPHP_CMD_MAX
 };
 
@@ -73,6 +74,16 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr, unsigned size)
         case CPHP_GET_NEXT_CPU_WITH_EVENT_CMD:
            val = cpu_st->selector;
            break;
+        case CPHP_READ_CST_CMD:
+            val = 0xFFFFFFFF;
+            if (cdev->cst_idx == 0) {
+                val = cpu_to_le32(cpu_st->cst_count);
+                cdev->cst_idx++;
+            } else if (cdev->cst_idx <= (cpu_st->cst_count * 9)) {
+                val = cpu_to_le32(cdev->cst[cdev->cst_idx - 1]);
+                cdev->cst_idx++;
+            }
+            break;
         default:
            break;
         }
@@ -145,6 +156,9 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr, uint64_t data,
                     }
                     iter = iter + 1 < cpu_st->dev_count ? iter + 1 : 0;
                 } while (iter != cpu_st->selector);
+            } else if (cpu_st->command == CPHP_READ_CST_CMD) {
+                cdev = &cpu_st->devs[cpu_st->selector];
+                cdev->cst_idx = 0;
             }
         }
         break;
@@ -197,6 +211,33 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
     id_list = mc->possible_cpu_arch_ids(machine);
     state->dev_count = id_list->len;
     state->devs = g_new0(typeof(*state->devs), state->dev_count);
+    if (state->cst_count) {
+        for (i = 0; i < state->dev_count; i++) {
+            int j;
+            state->devs[i].cst_count = state->cst_count;
+            state->devs[i].cst = g_new0(typeof(*state->devs->cst),
+                                        state->cst_count * 9);
+
+            /* HACK: makeup fake CST values somehow */
+            for (j = 0; j < state->cst_count; j++) {
+                int z = 9 * j;
+                /* AddressSpaceKeyword */
+                state->devs[i].cst[z + 0] = AML_AS_FFH;
+                /* RegisterBitWidth: Vendor: Intel */
+                state->devs[i].cst[z + 1] = 1;
+                /* RegisterBitOffset: Class: Native C State Instruction  */
+                state->devs[i].cst[z + 2] = 2;
+                /* RegisterAddress Lo: Arg0: mwait EAX hint */
+                state->devs[i].cst[z + 3] = 0;
+                /* RegisterAddress Hi: Reserved */
+                state->devs[i].cst[z + 4] = 0;
+                state->devs[i].cst[z + 5] = 0; /* AccessSize: Arg1 */
+                state->devs[i].cst[z + 6] = j + 1; /* The C State type C(j+1) */
+                state->devs[i].cst[z + 7] = 10 * (j + 1); /* CST: latency */
+                state->devs[i].cst[z + 8] = 100 * (j + 1); /* CST: power */
+            }
+        }
+    }
     for (i = 0; i < id_list->len; i++) {
         state->devs[i].cpu =  CPU(id_list->cpus[i].cpu);
         state->devs[i].arch_id = id_list->cpus[i].arch_id;
@@ -265,6 +306,37 @@ void acpi_cpu_unplug_cb(CPUHotplugState *cpu_st,
     cdev->cpu = NULL;
 }
 
+static const VMStateDescription vmstate_cstate_sts = {
+    .name = "CState",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields      = (VMStateField[]) {
+        VMSTATE_UINT32(cst_count, AcpiCpuStatus),
+        VMSTATE_UINT32(cst_idx, AcpiCpuStatus),
+        VMSTATE_VARRAY_UINT32(cst, AcpiCpuStatus, cst_count, 0,
+                              vmstate_info_uint32, uint32_t),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static bool vmstate_test_use_cst(void *opaque)
+{
+    CPUHotplugState *s = opaque;
+    return s->cst_count;
+}
+
+static const VMStateDescription vmstate_cstates = {
+    .name = "CPU hotplug state/CStates",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = vmstate_test_use_cst,
+    .fields      = (VMStateField[]) {
+        VMSTATE_STRUCT_VARRAY_POINTER_UINT32(devs, CPUHotplugState, dev_count,
+                                             vmstate_cstate_sts, AcpiCpuStatus),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 static const VMStateDescription vmstate_cpuhp_sts = {
     .name = "CPU hotplug device state",
     .version_id = 1,
@@ -290,6 +362,10 @@ const VMStateDescription vmstate_cpu_hotplug = {
         VMSTATE_STRUCT_VARRAY_POINTER_UINT32(devs, CPUHotplugState, dev_count,
                                              vmstate_cpuhp_sts, AcpiCpuStatus),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (const VMStateDescription * []) {
+       &vmstate_cstates,
+       NULL
     }
 };
 
@@ -301,6 +377,7 @@ const VMStateDescription vmstate_cpu_hotplug = {
 #define CPU_NOTIFY_METHOD "CTFY"
 #define CPU_EJECT_METHOD  "CEJ0"
 #define CPU_OST_METHOD    "COST"
+#define CPU_CST_METHOD    "CCST"
 
 #define CPU_ENABLED       "CPEN"
 #define CPU_SELECTOR      "CSEL"
@@ -501,6 +578,64 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
         }
         aml_append(cpus_dev, method);
 
+        if (opts.cst_count) {
+            Aml *crs, *loop;
+            Aml *pkg = aml_local(0);
+            Aml *cst = aml_local(1);
+            Aml *count = aml_local(2);
+            Aml *idx = aml_local(3);
+            Aml *cst_cmd = aml_int(CPHP_READ_CST_CMD);
+            Aml *uid = aml_arg(0);
+            Aml *nm = aml_name("CCRS");
+
+            method = aml_method(CPU_CST_METHOD, 1, AML_SERIALIZED);
+            /* Package to hold opts.cst_count CST entries */
+            aml_append(method, aml_store(aml_package(opts.cst_count + 1), pkg));
+            aml_append(method, aml_store(aml_package(4), cst)); /* CST entry */
+            aml_append(method, aml_store(aml_int(opts.cst_count),
+                                         aml_index(pkg, zero)));
+
+            /* create register template to fill in */
+            crs = aml_resource_template();
+            aml_append(crs, aml_register(AML_AS_FFH, 0, 0, 0, 0));
+            aml_append(method, aml_name_decl("CCRS", crs));
+            aml_append(method, aml_create_byte_field(nm, aml_int(3), "_ASI"));
+            aml_append(method, aml_create_byte_field(nm, aml_int(4), "_RBW"));
+            aml_append(method, aml_create_byte_field(nm, aml_int(5), "_RBO"));
+            aml_append(method, aml_create_dword_field(nm, aml_int(7), "LADR"));
+            aml_append(method, aml_create_dword_field(nm, aml_int(11), "HADR"));
+            aml_append(method, aml_create_byte_field(nm, aml_int(6), "_ASZ"));
+
+            aml_append(method, aml_acquire(ctrl_lock, 0xFFFF));
+            aml_append(method, aml_store(uid, cpu_selector));
+            aml_append(method, aml_store(cst_cmd, cpu_cmd));
+
+            aml_append(method, aml_store(one, idx));
+            aml_append(method, aml_store(cpu_data, count));
+
+            loop = aml_while(aml_lgreater_equal(count, idx));
+            /* fill in actual register values */
+            aml_append(loop, aml_store(cpu_data, aml_name("_ASI")));
+            aml_append(loop, aml_store(cpu_data, aml_name("_RBW")));
+            aml_append(loop, aml_store(cpu_data, aml_name("_RBO")));
+            aml_append(loop, aml_store(cpu_data, aml_name("LADR")));
+            aml_append(loop, aml_store(cpu_data, aml_name("HADR")));
+            aml_append(loop, aml_store(cpu_data, aml_name("_ASZ")));
+            /* pack CST entry */
+            aml_append(loop, aml_store(crs, aml_index(cst, zero)));
+            aml_append(loop, aml_store(cpu_data, aml_index(cst, one)));
+            aml_append(loop, aml_store(cpu_data, aml_index(cst, aml_int(2))));
+            aml_append(loop, aml_store(cpu_data, aml_index(cst, aml_int(3))));
+            /* store entry into _CST descriptors package */
+            aml_append(loop, aml_store(cst, aml_index(pkg, idx)));
+            aml_append(loop, aml_add(idx, one, idx));
+            aml_append(method, loop);
+            aml_append(method, aml_release(ctrl_lock));
+
+            aml_append(method, aml_return(pkg));
+            aml_append(cpus_dev, method);
+        }
+
         /* build Processor object for each processor */
         for (i = 0; i < arch_ids->len; i++) {
             Aml *dev;
@@ -520,6 +655,12 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
             aml_append(method, aml_return(aml_call1(CPU_STS_METHOD, uid)));
             aml_append(dev, method);
 
+            if (opts.cst_count) {
+                method = aml_method("_CST", 0, AML_SERIALIZED);
+                aml_append(method, aml_return(aml_call1(CPU_CST_METHOD, uid)));
+                aml_append(dev, method);
+            }
+
             /* build _MAT object */
             assert(adevc && adevc->madt_cpu);
             adevc->madt_cpu(adev, i, arch_ids, madt_buf);
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 6404af5..8858afc 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -677,6 +677,8 @@ static Property piix4_pm_properties[] = {
                      use_acpi_pci_hotplug, true),
     DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
                      acpi_memory_hotplug.is_enabled, true),
+    DEFINE_PROP_UINT32("cstate-count", PIIX4PMState,
+                      cpuhp_state.cst_count, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index e1ee8ae..490429f 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -100,6 +100,7 @@ typedef struct AcpiPmInfo {
     uint16_t cpu_hp_io_base;
     uint16_t pcihp_io_base;
     uint16_t pcihp_io_len;
+    uint32_t cst_count;
 } AcpiPmInfo;
 
 typedef struct AcpiMiscInfo {
@@ -218,6 +219,7 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
     pm->pcihp_bridge_en =
         object_property_get_bool(obj, "acpi-pci-hotplug-with-bridge-support",
                                  NULL);
+    pm->cst_count = object_property_get_uint(obj, "cstate-count", NULL);
 }
 
 static void acpi_get_misc_info(AcpiMiscInfo *info)
@@ -1840,7 +1842,8 @@ 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,
+            .cst_count = pm->cst_count
         };
         build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
                        "\\_SB.PCI0", "\\_GPE._E02");
diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 4e24930..d3a62e5 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -716,6 +716,7 @@ static void test_acpi_piix4_tcg_cphp(void)
     data.machine = MACHINE_PC;
     data.variant = ".cphp";
     test_acpi_one("-smp 2,cores=3,sockets=2,maxcpus=6"
+                  " -global PIIX4_PM.cstate-count=2"
                   " -numa node -numa node"
                   " -numa dist,src=0,dst=1,val=21",
                   &data);
-- 
2.7.4

[Qemu-devel] [RFC PATCH 4/4] acpi: add support for CST update notification

[Igor Mammedov]

Reuse cpu hotplug inotification interface to notify guest about CST change.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/acpi/cpu.h           |  1 +
 docs/specs/acpi_cpu_hotplug.txt | 11 ++++++++---
 hw/acpi/cpu.c                   | 27 ++++++++++++++++++++++++++-
 3 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
index eb79cbf..51d7fc6 100644
--- a/include/hw/acpi/cpu.h
+++ b/include/hw/acpi/cpu.h
@@ -28,6 +28,7 @@ typedef struct AcpiCpuStatus {
     uint64_t arch_id;
     bool is_inserting;
     bool is_removing;
+    bool is_cst_update;
     uint32_t ost_event;
     uint32_t ost_status;
     AcpiCState cst;
diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
index adfb026..41ba236 100644
--- a/docs/specs/acpi_cpu_hotplug.txt
+++ b/docs/specs/acpi_cpu_hotplug.txt
@@ -41,7 +41,10 @@ read access:
               It's valid only when bit 0 is set.
            2: Device remove event, used to distinguish device for which
               no device eject request to OSPM was issued.
-           3-7: reserved and should be ignored by OSPM
+           3: reserved and should be ignored by OSPM
+           4: Device CST event, used to distinguish device for which
+              device eject request to OSPM was issued
+           5-7: reserved and should be ignored by OSPM
     [0x5-0x7] reserved
     [0x8] Command data: (DWORD access)
           in case of error or unsupported command reads is 0xFFFFFFFF
@@ -70,14 +73,16 @@ write access:
                selected CPU device
             3: if set to 1 initiates device eject, set by OSPM when it
                triggers CPU device removal and calls _EJ0 method
+            4: if set to 1 clears CST update event, set by OSPM
+               after it has emitted CST update notification
             4-7: reserved, OSPM must clear them before writing to register
     [0x5] Command field: (1 byte access)
           value:
-            0: selects a CPU device with inserting/removing events and
+            0: selects a CPU device with inserting/removing/cst events and
                following reads from 'Command data' register return
                selected CPU (CPU selector value). If no CPU with events
                found, the current CPU selector doesn't change and
-               corresponding insert/remove event flags are not set.
+               corresponding insert/remove/cst event flags are not set.
             1: following writes to 'Command data' register set OST event
                register in QEMU
             2: following writes to 'Command data' register set OST status
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 7ef04f9..c4a9fac 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -67,6 +67,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_update ? 16 : 0;
         trace_cpuhp_acpi_read_flags(cpu_st->selector, val);
         break;
     case ACPI_CPU_CMD_DATA_OFFSET_RW:
@@ -162,6 +163,8 @@ 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 & 16) { /* clear CST update event */
+            cdev->is_cst_update = false;
         }
         break;
     case ACPI_CPU_CMD_OFFSET_WR:
@@ -312,6 +315,7 @@ static const VMStateDescription vmstate_cstate_sts = {
         VMSTATE_UINT32(cst.current_cst_field, AcpiCpuStatus),
         VMSTATE_UINT32(cst.latency, AcpiCpuStatus),
         VMSTATE_UINT32(cst.power, AcpiCpuStatus),
+        VMSTATE_BOOL(is_cst_update, AcpiCpuStatus),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -383,6 +387,7 @@ const VMStateDescription vmstate_cpu_hotplug = {
 #define CPU_INSERT_EVENT  "CINS"
 #define CPU_REMOVE_EVENT  "CRMV"
 #define CPU_EJECT_EVENT   "CEJ0"
+#define CPU_CST_EVENT     "CSTU"
 
 void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
                     hwaddr io_base,
@@ -435,7 +440,13 @@ 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));
+        if (opts.cstate_enabled) {
+            /* (read) 1 if has a CST event. (write) 1 to clear event */
+            aml_append(field, aml_named_field(CPU_CST_EVENT, 1));
+            aml_append(field, aml_reserved_field(3));
+        } else {
+            aml_append(field, aml_reserved_field(4));
+        }
         aml_append(field, aml_named_field(CPU_COMMAND, 8));
         aml_append(cpu_ctrl_dev, field);
 
@@ -470,6 +481,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_CST_EVENT);
 
         aml_append(cpus_dev, aml_name_decl("_HID", aml_string("ACPI0010")));
         aml_append(cpus_dev, aml_name_decl("_CID", aml_eisaid("PNP0A05")));
@@ -524,6 +536,7 @@ 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_upd = aml_int(0x81);
             Aml *next_cpu_cmd = aml_int(CPHP_GET_NEXT_CPU_WITH_EVENT_CMD);
 
             aml_append(method, aml_acquire(ctrl_lock, 0xFFFF));
@@ -553,6 +566,18 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
                  }
                  aml_append(else_ctx, ifctx);
                  aml_append(while_ctx, else_ctx);
+                 if (opts.cstate_enabled == true) {
+                    else_ctx = aml_else();
+                    ifctx = aml_if(aml_equal(cst_evt, one));
+                    {
+                        aml_append(ifctx,
+                            aml_call2(CPU_NOTIFY_METHOD, cpu_data, cst_upd));
+                        aml_append(ifctx, aml_store(one, cst_evt));
+                        aml_append(ifctx, aml_store(one, has_event));
+                    }
+                    aml_append(else_ctx, ifctx);
+                    aml_append(while_ctx, else_ctx);
+                 }
             }
             aml_append(method, while_ctx);
             aml_append(method, aml_release(ctrl_lock));
-- 
2.7.4

Re: [Qemu-devel] [RFC PATCH 0/4] "pc: acpi: _CST support"

[Michael S. Tsirkin]

On Wed, Aug 08, 2018 at 05:15:45PM +0200, Igor Mammedov wrote:
> It's an alternative approach to
>  1) [PATCH hack dontapply v2 0/7] Dynamic _CST generation
> which instead of dynamic AML loading uses static AML with
> dynamic values.  It allows us to keep firmware blob static and
> to avoid split firmware issue (1) in case of cross version migration.

I think there's a misunderstanding. That patch only declares a couple of
states but that is just for debugging/demonstration purposes.  A typical
real CPU has more states (e.g. some intel CPUs have ~10 levels).


> ABI in this case is confined to cpu hotplug IO registers
> (i.e. do it old school way, like we used to do so far).
> This way we don't have to add yet another ABI to keep dynamic
> AML code under control (1).
> 
> Tested  with: XPsp3 - ws2106 guests.
> 
> CC: "Michael S. Tsirkin" <mst@redhat.com>
> 
> 
> Igor Mammedov (3):
>   acpi: add aml_create_byte_field()
>   pc: acpi: add _CST support
>   acpi: add support for CST update notification
> 
> Michael S. Tsirkin (1):
>   acpi: aml: add aml_register()
> 
>  include/hw/acpi/aml-build.h     |   6 ++
>  include/hw/acpi/cpu.h           |  10 +++
>  docs/specs/acpi_cpu_hotplug.txt |  21 +++++-
>  hw/acpi/aml-build.c             |  28 +++++++
>  hw/acpi/cpu.c                   | 158 +++++++++++++++++++++++++++++++++++++++-
>  hw/acpi/piix4.c                 |   2 +
>  hw/i386/acpi-build.c            |   5 +-
>  tests/bios-tables-test.c        |   1 +
>  8 files changed, 225 insertions(+), 6 deletions(-)
> 
> -- 
> 2.7.4

Re: [Qemu-devel] [RFC PATCH 0/4] "pc: acpi: _CST support"

[Igor Mammedov]

On Wed, 8 Aug 2018 23:20:25 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Aug 08, 2018 at 05:15:45PM +0200, Igor Mammedov wrote:
> > It's an alternative approach to
> >  1) [PATCH hack dontapply v2 0/7] Dynamic _CST generation
> > which instead of dynamic AML loading uses static AML with
> > dynamic values.  It allows us to keep firmware blob static and
> > to avoid split firmware issue (1) in case of cross version migration.  
> 
> I think there's a misunderstanding. That patch only declares a couple of
> states but that is just for debugging/demonstration purposes.  A typical
> real CPU has more states (e.g. some intel CPUs have ~10 levels).
yep, baremetal has more CStates, but that depends on what we are trying to
achieve here. If goal is to use mwait/monitor instead o halt when idle
to reduce wakeup latencies then 1 entry in _CST is sufficient.
If target is reduce power consumption than we would need more entries.
In the later case _CST is probably not sufficient as it provides only
power/latency pair while intel_idle also provides target residence.
So if power mgmt target is considered, we need to use new _LPI
which means guest will need support for it as well, not sure if x86
kernel supports it nor aware of such support in Windows (hopefully it 
would just ignore unknown method)

> 
> > ABI in this case is confined to cpu hotplug IO registers
> > (i.e. do it old school way, like we used to do so far).
> > This way we don't have to add yet another ABI to keep dynamic
> > AML code under control (1).
> > 
> > Tested  with: XPsp3 - ws2106 guests.
> > 
> > CC: "Michael S. Tsirkin" <mst@redhat.com>
> > 
> > 
> > Igor Mammedov (3):
> >   acpi: add aml_create_byte_field()
> >   pc: acpi: add _CST support
> >   acpi: add support for CST update notification
> > 
> > Michael S. Tsirkin (1):
> >   acpi: aml: add aml_register()
> > 
> >  include/hw/acpi/aml-build.h     |   6 ++
> >  include/hw/acpi/cpu.h           |  10 +++
> >  docs/specs/acpi_cpu_hotplug.txt |  21 +++++-
> >  hw/acpi/aml-build.c             |  28 +++++++
> >  hw/acpi/cpu.c                   | 158 +++++++++++++++++++++++++++++++++++++++-
> >  hw/acpi/piix4.c                 |   2 +
> >  hw/i386/acpi-build.c            |   5 +-
> >  tests/bios-tables-test.c        |   1 +
> >  8 files changed, 225 insertions(+), 6 deletions(-)
> > 
> > -- 
> > 2.7.4  

Re: [Qemu-devel] [RFC PATCH 0/4] "pc: acpi: _CST support"

[Michael S. Tsirkin]

On Thu, Aug 09, 2018 at 12:13:24PM +0200, Igor Mammedov wrote:
> On Wed, 8 Aug 2018 23:20:25 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Aug 08, 2018 at 05:15:45PM +0200, Igor Mammedov wrote:
> > > It's an alternative approach to
> > >  1) [PATCH hack dontapply v2 0/7] Dynamic _CST generation
> > > which instead of dynamic AML loading uses static AML with
> > > dynamic values.  It allows us to keep firmware blob static and
> > > to avoid split firmware issue (1) in case of cross version migration.  
> > 
> > I think there's a misunderstanding. That patch only declares a couple of
> > states but that is just for debugging/demonstration purposes.  A typical
> > real CPU has more states (e.g. some intel CPUs have ~10 levels).
> yep, baremetal has more CStates, but that depends on what we are trying to
> achieve here.
> If goal is to use mwait/monitor instead o halt when idle
> to reduce wakeup latencies then 1 entry in _CST is sufficient.

1 entry is not sufficient in my testing. My testing shows
we want to stay in guest for states with latency up to ~200 usec
to get measureable latency gains.


> If target is reduce power consumption than we would need more entries.
> In the later case _CST is probably not sufficient as it provides only
> power/latency pair while intel_idle also provides target residence.
> So if power mgmt target is considered, we need to use new _LPI
> which means guest will need support for it as well, not sure if x86
> kernel supports it nor aware of such support in Windows (hopefully it 
> would just ignore unknown method)

I think we want to address both goals.

Linux on x86 does support _LPI and I agree it's a good idea to be able
to pass that in. Will take a look. But intel_idle seems to just set
target = latency * 2 so I am not sure it's all that important.

> > 
> > > ABI in this case is confined to cpu hotplug IO registers
> > > (i.e. do it old school way, like we used to do so far).
> > > This way we don't have to add yet another ABI to keep dynamic
> > > AML code under control (1).
> > > 
> > > Tested  with: XPsp3 - ws2106 guests.
> > > 
> > > CC: "Michael S. Tsirkin" <mst@redhat.com>
> > > 
> > > 
> > > Igor Mammedov (3):
> > >   acpi: add aml_create_byte_field()
> > >   pc: acpi: add _CST support
> > >   acpi: add support for CST update notification
> > > 
> > > Michael S. Tsirkin (1):
> > >   acpi: aml: add aml_register()
> > > 
> > >  include/hw/acpi/aml-build.h     |   6 ++
> > >  include/hw/acpi/cpu.h           |  10 +++
> > >  docs/specs/acpi_cpu_hotplug.txt |  21 +++++-
> > >  hw/acpi/aml-build.c             |  28 +++++++
> > >  hw/acpi/cpu.c                   | 158 +++++++++++++++++++++++++++++++++++++++-
> > >  hw/acpi/piix4.c                 |   2 +
> > >  hw/i386/acpi-build.c            |   5 +-
> > >  tests/bios-tables-test.c        |   1 +
> > >  8 files changed, 225 insertions(+), 6 deletions(-)
> > > 
> > > -- 
> > > 2.7.4  

Re: [Qemu-devel] [RFC PATCH 0/4] "pc: acpi: _CST support"

[Igor Mammedov]

On Fri, 10 Aug 2018 02:09:58 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Aug 09, 2018 at 12:13:24PM +0200, Igor Mammedov wrote:
> > On Wed, 8 Aug 2018 23:20:25 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Wed, Aug 08, 2018 at 05:15:45PM +0200, Igor Mammedov wrote:  
> > > > It's an alternative approach to
> > > >  1) [PATCH hack dontapply v2 0/7] Dynamic _CST generation
> > > > which instead of dynamic AML loading uses static AML with
> > > > dynamic values.  It allows us to keep firmware blob static and
> > > > to avoid split firmware issue (1) in case of cross version migration.    
> > > 
> > > I think there's a misunderstanding. That patch only declares a couple of
> > > states but that is just for debugging/demonstration purposes.  A typical
> > > real CPU has more states (e.g. some intel CPUs have ~10 levels).  
> > yep, baremetal has more CStates, but that depends on what we are trying to
> > achieve here.
> > If goal is to use mwait/monitor instead o halt when idle
> > to reduce wakeup latencies then 1 entry in _CST is sufficient.  
> 
> 1 entry is not sufficient in my testing. My testing shows
> we want to stay in guest for states with latency up to ~200 usec
> to get measureable latency gains.
Ok, I'll add support for more CStates

> 
> > If target is reduce power consumption than we would need more entries.
> > In the later case _CST is probably not sufficient as it provides only
> > power/latency pair while intel_idle also provides target residence.
> > So if power mgmt target is considered, we need to use new _LPI
> > which means guest will need support for it as well, not sure if x86
> > kernel supports it nor aware of such support in Windows (hopefully it 
> > would just ignore unknown method)  
> 
> I think we want to address both goals.
> 
> Linux on x86 does support _LPI and I agree it's a good idea to be able
> to pass that in. Will take a look. But intel_idle seems to just set
> target = latency * 2 so I am not sure it's all that important.
it's fairly recent, but if we plan in advance we can support it
as well.

> 
> > >   
> > > > ABI in this case is confined to cpu hotplug IO registers
> > > > (i.e. do it old school way, like we used to do so far).
> > > > This way we don't have to add yet another ABI to keep dynamic
> > > > AML code under control (1).
> > > > 
> > > > Tested  with: XPsp3 - ws2106 guests.
> > > > 
> > > > CC: "Michael S. Tsirkin" <mst@redhat.com>
> > > > 
> > > > 
> > > > Igor Mammedov (3):
> > > >   acpi: add aml_create_byte_field()
> > > >   pc: acpi: add _CST support
> > > >   acpi: add support for CST update notification
> > > > 
> > > > Michael S. Tsirkin (1):
> > > >   acpi: aml: add aml_register()
> > > > 
> > > >  include/hw/acpi/aml-build.h     |   6 ++
> > > >  include/hw/acpi/cpu.h           |  10 +++
> > > >  docs/specs/acpi_cpu_hotplug.txt |  21 +++++-
> > > >  hw/acpi/aml-build.c             |  28 +++++++
> > > >  hw/acpi/cpu.c                   | 158 +++++++++++++++++++++++++++++++++++++++-
> > > >  hw/acpi/piix4.c                 |   2 +
> > > >  hw/i386/acpi-build.c            |   5 +-
> > > >  tests/bios-tables-test.c        |   1 +
> > > >  8 files changed, 225 insertions(+), 6 deletions(-)
> > > > 
> > > > -- 
> > > > 2.7.4