[Qemu-devel] [PATCH v3 0/8] NVDIMM ACPI: introduce the framework of QEMU emulated DSM

[Qemu-devel] [PATCH v3 0/8] NVDIMM ACPI: introduce the framework of QEMU emulated DSM

[Xiao Guangrong]

This patchset is against commit a407644079c86390 (net: set endianness on all
backend devices) on pci branch of Michael's git tree
and can be found at:
      https://github.com/xiaogr/qemu.git nvdimm-acpi-v3

Changelog in v3:
Changes addressing Michael's comment:
- rebase the patchset against current code

Changes addressing Igor's comment:
- rename the parameters of aml_create_field() to reflect the ACPI spec
- fix the issue that the @target operand can not be optional in
  aml_concatenate() that is also cleaned up by using build_opcode_2arg_dst()

Others:
- separate the test patches to the single set and will be posted on later 
  
These changes are based on Igor's comments:
- drop ssdt.rev2 support as the memory address allocated by BIOS/OVMF
  are always 32 bits
- support to test NVDIMM tables (NFIT and NVDIMM SSDT)
- add _CRS to report its operation region
- make AML APIs change be the separated patches

This is the second part of vNVDIMM implementation which implements the
BIOS patched dsm memory and introduces the framework that allows QEMU
to emulate DSM method

Thanks to Michael's idea, we do not reserve any memory for NVDIMM ACPI,
instead we let BIOS allocate the memory and patch the address to the
offset we want

IO port is still enabled as it plays as the way to notify QEMU and pass
the patched dsm memory address, so that IO port region, 0x0a18 - 0xa20,
is reserved and it is divided into two 32 bits ports and used to pass
the low 32 bits and high 32 bits of dsm memory address to QEMU

Thanks Igor's idea, this patchset also extends DSDT/SSDT to revision 2
to apply 64 bit operations, in order to keeping compatibility, old
version (<= 2.5) still uses revision 1. Since 64 bit operations breaks
old guests (such as windows XP), we should keep the 64 bits stuff in
the private place where common ACPI operation does not touch it

Xiao Guangrong (8):
  acpi: add aml_create_field()
  acpi: add aml_concatenate()
  acpi: allow using object as offset for OperationRegion
  nvdimm acpi: initialize the resource used by NVDIMM ACPI
  nvdimm acpi: introduce patched dsm memory
  nvdimm acpi: let qemu handle _DSM method
  nvdimm acpi: emulate dsm method
  nvdimm acpi: add _CRS

 hw/acpi/Makefile.objs       |   2 +-
 hw/acpi/aml-build.c         |  28 ++++-
 hw/acpi/nvdimm.c            | 255 +++++++++++++++++++++++++++++++++++++++++++-
 hw/i386/acpi-build.c        |  41 +++----
 hw/i386/pc.c                |   6 +-
 hw/i386/pc_piix.c           |   5 +
 hw/i386/pc_q35.c            |   8 +-
 include/hw/acpi/aml-build.h |   6 +-
 include/hw/i386/pc.h        |   4 +-
 include/hw/mem/nvdimm.h     |  36 ++++++-
 10 files changed, 351 insertions(+), 40 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 1/8] acpi: add aml_create_field()

[Xiao Guangrong]

It will be used by nvdimm acpi

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/aml-build.c         | 14 ++++++++++++++
 include/hw/acpi/aml-build.h |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 603068b..2cc3211 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -997,6 +997,20 @@ Aml *create_field_common(int opcode, Aml *srcbuf, Aml *index, const char *name)
     return var;
 }
 
+/* ACPI 1.0b: 16.2.5.2 Named Objects Encoding: DefCreateField */
+Aml *aml_create_field(Aml *srcbuf, Aml *bit_index, Aml *num_bits,
+                      const char *name)
+{
+    Aml *var = aml_alloc();
+    build_append_byte(var->buf, 0x5B); /* ExtOpPrefix */
+    build_append_byte(var->buf, 0x13); /* CreateFieldOp */
+    aml_append(var, srcbuf);
+    aml_append(var, bit_index);
+    aml_append(var, num_bits);
+    build_append_namestring(var->buf, "%s", name);
+    return var;
+}
+
 /* ACPI 1.0b: 16.2.5.2 Named Objects Encoding: DefCreateDWordField */
 Aml *aml_create_dword_field(Aml *srcbuf, Aml *index, const char *name)
 {
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index aa29d30..8ef10ad 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -346,6 +346,8 @@ Aml *aml_mutex(const char *name, uint8_t sync_level);
 Aml *aml_acquire(Aml *mutex, uint16_t timeout);
 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_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);
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 2/8] acpi: add aml_concatenate()

[Xiao Guangrong]

It will be used by nvdimm acpi

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/aml-build.c         | 8 ++++++++
 include/hw/acpi/aml-build.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 2cc3211..3362cc4 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1437,6 +1437,14 @@ Aml *aml_alias(const char *source_object, const char *alias_object)
     return var;
 }
 
+/* ACPI 1.0b: 16.2.5.4 Type 2 Opcodes Encoding: DefConcat */
+Aml *aml_concatenate(Aml *source1, Aml *source2, Aml *target)
+{
+    assert(target);
+    return build_opcode_2arg_dst(0x73 /* ConcatOp */, source1, source2,
+                                 target);
+}
+
 void
 build_header(GArray *linker, GArray *table_data,
              AcpiTableHeader *h, const char *sig, int len, uint8_t rev,
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 8ef10ad..735c34a 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -355,6 +355,7 @@ Aml *aml_touuid(const char *uuid);
 Aml *aml_unicode(const char *str);
 Aml *aml_derefof(Aml *arg);
 Aml *aml_sizeof(Aml *arg);
+Aml *aml_concatenate(Aml *source1, Aml *source2, Aml *target);
 
 void
 build_header(GArray *linker, GArray *table_data,
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 3/8] acpi: allow using object as offset for OperationRegion

[Xiao Guangrong]

Extend aml_operation_region() to use object as offset

Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/aml-build.c         |  4 ++--
 hw/i386/acpi-build.c        | 31 ++++++++++++++++---------------
 include/hw/acpi/aml-build.h |  2 +-
 3 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 3362cc4..c4662f0 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -942,14 +942,14 @@ Aml *aml_package(uint8_t num_elements)
 
 /* ACPI 1.0b: 16.2.5.2 Named Objects Encoding: DefOpRegion */
 Aml *aml_operation_region(const char *name, AmlRegionSpace rs,
-                          uint32_t offset, uint32_t len)
+                          Aml *offset, uint32_t len)
 {
     Aml *var = aml_alloc();
     build_append_byte(var->buf, 0x5B); /* ExtOpPrefix */
     build_append_byte(var->buf, 0x80); /* OpRegionOp */
     build_append_namestring(var->buf, "%s", name);
     build_append_byte(var->buf, rs);
-    build_append_int(var->buf, offset);
+    aml_append(var, offset);
     build_append_int(var->buf, len);
     return var;
 }
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 4554eb8..10399ec 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -993,7 +993,7 @@ static void build_processor_devices(Aml *sb_scope, unsigned acpi_cpus,
     aml_append(sb_scope, dev);
     /* declare CPU hotplug MMIO region and PRS field to access it */
     aml_append(sb_scope, aml_operation_region(
-        "PRST", AML_SYSTEM_IO, pm->cpu_hp_io_base, pm->cpu_hp_io_len));
+        "PRST", AML_SYSTEM_IO, aml_int(pm->cpu_hp_io_base), pm->cpu_hp_io_len));
     field = aml_field("PRST", AML_BYTE_ACC, AML_NOLOCK, AML_PRESERVE);
     aml_append(field, aml_named_field("PRS", 256));
     aml_append(sb_scope, field);
@@ -1078,7 +1078,7 @@ static void build_memory_devices(Aml *sb_scope, int nr_mem,
 
     aml_append(scope, aml_operation_region(
         MEMORY_HOTPLUG_IO_REGION, AML_SYSTEM_IO,
-        io_base, io_len)
+        aml_int(io_base), io_len)
     );
 
     field = aml_field(MEMORY_HOTPLUG_IO_REGION, AML_DWORD_ACC,
@@ -1192,7 +1192,8 @@ static void build_hpet_aml(Aml *table)
     aml_append(dev, aml_name_decl("_UID", zero));
 
     aml_append(dev,
-        aml_operation_region("HPTM", AML_SYSTEM_MEMORY, HPET_BASE, HPET_LEN));
+        aml_operation_region("HPTM", AML_SYSTEM_MEMORY, aml_int(HPET_BASE),
+                             HPET_LEN));
     field = aml_field("HPTM", AML_DWORD_ACC, AML_LOCK, AML_PRESERVE);
     aml_append(field, aml_named_field("VEND", 32));
     aml_append(field, aml_named_field("PRD", 32));
@@ -1430,7 +1431,7 @@ static void build_dbg_aml(Aml *table)
     Aml *idx = aml_local(2);
 
     aml_append(scope,
-       aml_operation_region("DBG", AML_SYSTEM_IO, 0x0402, 0x01));
+       aml_operation_region("DBG", AML_SYSTEM_IO, aml_int(0x0402), 0x01));
     field = aml_field("DBG", AML_BYTE_ACC, AML_NOLOCK, AML_PRESERVE);
     aml_append(field, aml_named_field("DBGB", 8));
     aml_append(scope, field);
@@ -1770,10 +1771,10 @@ static void build_q35_isa_bridge(Aml *table)
 
     /* ICH9 PCI to ISA irq remapping */
     aml_append(dev, aml_operation_region("PIRQ", AML_PCI_CONFIG,
-                                         0x60, 0x0C));
+                                         aml_int(0x60), 0x0C));
 
     aml_append(dev, aml_operation_region("LPCD", AML_PCI_CONFIG,
-                                         0x80, 0x02));
+                                         aml_int(0x80), 0x02));
     field = aml_field("LPCD", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE);
     aml_append(field, aml_named_field("COMA", 3));
     aml_append(field, aml_reserved_field(1));
@@ -1785,7 +1786,7 @@ static void build_q35_isa_bridge(Aml *table)
     aml_append(dev, field);
 
     aml_append(dev, aml_operation_region("LPCE", AML_PCI_CONFIG,
-                                         0x82, 0x02));
+                                         aml_int(0x82), 0x02));
     /* enable bits */
     field = aml_field("LPCE", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE);
     aml_append(field, aml_named_field("CAEN", 1));
@@ -1808,7 +1809,7 @@ static void build_piix4_pm(Aml *table)
     aml_append(dev, aml_name_decl("_ADR", aml_int(0x00010003)));
 
     aml_append(dev, aml_operation_region("P13C", AML_PCI_CONFIG,
-                                         0x00, 0xff));
+                                         aml_int(0x00), 0xff));
     aml_append(scope, dev);
     aml_append(table, scope);
 }
@@ -1825,7 +1826,7 @@ static void build_piix4_isa_bridge(Aml *table)
 
     /* PIIX PCI to ISA irq remapping */
     aml_append(dev, aml_operation_region("P40C", AML_PCI_CONFIG,
-                                         0x60, 0x04));
+                                         aml_int(0x60), 0x04));
     /* enable bits */
     field = aml_field("^PX13.P13C", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE);
     /* Offset(0x5f),, 7, */
@@ -1854,20 +1855,20 @@ static void build_piix4_pci_hotplug(Aml *table)
     scope =  aml_scope("_SB.PCI0");
 
     aml_append(scope,
-        aml_operation_region("PCST", AML_SYSTEM_IO, 0xae00, 0x08));
+        aml_operation_region("PCST", AML_SYSTEM_IO, aml_int(0xae00), 0x08));
     field = aml_field("PCST", AML_DWORD_ACC, AML_NOLOCK, AML_WRITE_AS_ZEROS);
     aml_append(field, aml_named_field("PCIU", 32));
     aml_append(field, aml_named_field("PCID", 32));
     aml_append(scope, field);
 
     aml_append(scope,
-        aml_operation_region("SEJ", AML_SYSTEM_IO, 0xae08, 0x04));
+        aml_operation_region("SEJ", AML_SYSTEM_IO, aml_int(0xae08), 0x04));
     field = aml_field("SEJ", AML_DWORD_ACC, AML_NOLOCK, AML_WRITE_AS_ZEROS);
     aml_append(field, aml_named_field("B0EJ", 32));
     aml_append(scope, field);
 
     aml_append(scope,
-        aml_operation_region("BNMR", AML_SYSTEM_IO, 0xae10, 0x04));
+        aml_operation_region("BNMR", AML_SYSTEM_IO, aml_int(0xae10), 0x04));
     field = aml_field("BNMR", AML_DWORD_ACC, AML_NOLOCK, AML_WRITE_AS_ZEROS);
     aml_append(field, aml_named_field("BNUM", 32));
     aml_append(scope, field);
@@ -1975,9 +1976,9 @@ build_dsdt(GArray *table_data, GArray *linker,
     } else {
         sb_scope = aml_scope("_SB");
         aml_append(sb_scope,
-            aml_operation_region("PCST", AML_SYSTEM_IO, 0xae00, 0x0c));
+            aml_operation_region("PCST", AML_SYSTEM_IO, aml_int(0xae00), 0x0c));
         aml_append(sb_scope,
-            aml_operation_region("PCSB", AML_SYSTEM_IO, 0xae0c, 0x01));
+            aml_operation_region("PCSB", AML_SYSTEM_IO, aml_int(0xae0c), 0x01));
         field = aml_field("PCSB", AML_ANY_ACC, AML_NOLOCK, AML_WRITE_AS_ZEROS);
         aml_append(field, aml_named_field("PCIB", 8));
         aml_append(sb_scope, field);
@@ -2223,7 +2224,7 @@ build_dsdt(GArray *table_data, GArray *linker,
         aml_append(dev, aml_name_decl("_CRS", crs));
 
         aml_append(dev, aml_operation_region("PEOR", AML_SYSTEM_IO,
-                                              misc->pvpanic_port, 1));
+                                              aml_int(misc->pvpanic_port), 1));
         field = aml_field("PEOR", AML_BYTE_ACC, AML_NOLOCK, AML_PRESERVE);
         aml_append(field, aml_named_field("PEPT", 8));
         aml_append(dev, field);
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 735c34a..07b2d48 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -287,7 +287,7 @@ Aml *aml_interrupt(AmlConsumerAndProducer con_and_pro,
 Aml *aml_io(AmlIODecode dec, uint16_t min_base, uint16_t max_base,
             uint8_t aln, uint8_t len);
 Aml *aml_operation_region(const char *name, AmlRegionSpace rs,
-                          uint32_t offset, uint32_t len);
+                          Aml *offset, uint32_t len);
 Aml *aml_irq_no_flags(uint8_t irq);
 Aml *aml_named_field(const char *name, unsigned length);
 Aml *aml_reserved_field(unsigned length);
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 4/8] nvdimm acpi: initialize the resource used by NVDIMM ACPI

[Xiao Guangrong]

32 bits IO port starting from 0x0a18 in guest is reserved for NVDIMM
ACPI emulation. The table, NVDIMM_DSM_MEM_FILE, will be patched into
NVDIMM ACPI binary code

OSPM uses this port to tell QEMU the final address of the DSM memory
and notify QEMU to emulate the DSM method

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/Makefile.objs   |  2 +-
 hw/acpi/nvdimm.c        | 35 +++++++++++++++++++++++++++++++++++
 hw/i386/acpi-build.c    | 10 +---------
 hw/i386/pc.c            |  6 +++---
 hw/i386/pc_piix.c       |  5 +++++
 hw/i386/pc_q35.c        |  8 +++++++-
 include/hw/i386/pc.h    |  4 +++-
 include/hw/mem/nvdimm.h | 28 +++++++++++++++++++++++++++-
 8 files changed, 82 insertions(+), 16 deletions(-)

diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
index f3ade9a..faee86c 100644
--- a/hw/acpi/Makefile.objs
+++ b/hw/acpi/Makefile.objs
@@ -2,7 +2,7 @@ common-obj-$(CONFIG_ACPI_X86) += core.o piix4.o pcihp.o
 common-obj-$(CONFIG_ACPI_X86_ICH) += ich9.o tco.o
 common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu_hotplug.o cpu_hotplug_acpi_table.o
 common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o memory_hotplug_acpi_table.o
-common-obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
+obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
 common-obj-$(CONFIG_ACPI) += acpi_interface.o
 common-obj-$(CONFIG_ACPI) += bios-linker-loader.o
 common-obj-$(CONFIG_ACPI) += aml-build.o
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 49ee68e..8568b20 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -29,6 +29,7 @@
 #include "qemu/osdep.h"
 #include "hw/acpi/acpi.h"
 #include "hw/acpi/aml-build.h"
+#include "hw/nvram/fw_cfg.h"
 #include "hw/mem/nvdimm.h"
 
 static int nvdimm_plugged_device_list(Object *obj, void *opaque)
@@ -370,6 +371,40 @@ static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets,
     g_array_free(structures, true);
 }
 
+static uint64_t
+nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
+{
+    return 0;
+}
+
+static void
+nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
+{
+}
+
+static const MemoryRegionOps nvdimm_dsm_ops = {
+    .read = nvdimm_dsm_read,
+    .write = nvdimm_dsm_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+};
+
+void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
+                            FWCfgState *fw_cfg, Object *owner)
+{
+    memory_region_init_io(&state->io_mr, owner, &nvdimm_dsm_ops, state,
+                          "nvdimm-acpi-io", NVDIMM_ACPI_IO_LEN);
+    memory_region_add_subregion(io, NVDIMM_ACPI_IO_BASE, &state->io_mr);
+
+    state->dsm_mem = g_array_new(false, true /* clear */, 1);
+    acpi_data_push(state->dsm_mem, TARGET_PAGE_SIZE);
+    fw_cfg_add_file(fw_cfg, NVDIMM_DSM_MEM_FILE, state->dsm_mem->data,
+                    state->dsm_mem->len);
+}
+
 #define NVDIMM_COMMON_DSM      "NCAL"
 
 static void nvdimm_build_common_dsm(Aml *dev)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 10399ec..90fc415 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -38,7 +38,6 @@
 #include "hw/loader.h"
 #include "hw/isa/isa.h"
 #include "hw/acpi/memory_hotplug.h"
-#include "hw/mem/nvdimm.h"
 #include "sysemu/tpm.h"
 #include "hw/acpi/tpm.h"
 #include "sysemu/tpm_backend.h"
@@ -2581,13 +2580,6 @@ static bool acpi_has_iommu(void)
     return intel_iommu && !ambiguous;
 }
 
-static bool acpi_has_nvdimm(void)
-{
-    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
-
-    return pcms->nvdimm;
-}
-
 static
 void acpi_build(AcpiBuildTables *tables)
 {
@@ -2672,7 +2664,7 @@ void acpi_build(AcpiBuildTables *tables)
         build_dmar_q35(tables_blob, tables->linker);
     }
 
-    if (acpi_has_nvdimm()) {
+    if (pcms->acpi_nvdimm_state.is_enabled) {
         nvdimm_build_acpi(table_offsets, tables_blob, tables->linker);
     }
 
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index ce185bb..8fadcc7 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1853,14 +1853,14 @@ static bool pc_machine_get_nvdimm(Object *obj, Error **errp)
 {
     PCMachineState *pcms = PC_MACHINE(obj);
 
-    return pcms->nvdimm;
+    return pcms->acpi_nvdimm_state.is_enabled;
 }
 
 static void pc_machine_set_nvdimm(Object *obj, bool value, Error **errp)
 {
     PCMachineState *pcms = PC_MACHINE(obj);
 
-    pcms->nvdimm = value;
+    pcms->acpi_nvdimm_state.is_enabled = value;
 }
 
 static void pc_machine_initfn(Object *obj)
@@ -1899,7 +1899,7 @@ static void pc_machine_initfn(Object *obj)
                                     &error_abort);
 
     /* nvdimm is disabled on default. */
-    pcms->nvdimm = false;
+    pcms->acpi_nvdimm_state.is_enabled = false;
     object_property_add_bool(obj, PC_MACHINE_NVDIMM, pc_machine_get_nvdimm,
                              pc_machine_set_nvdimm, &error_abort);
 }
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 6f8c2cd..6a69b23 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -274,6 +274,11 @@ static void pc_init1(MachineState *machine,
     if (pcmc->pci_enabled) {
         pc_pci_device_init(pci_bus);
     }
+
+    if (pcms->acpi_nvdimm_state.is_enabled) {
+        nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,
+                               pcms->fw_cfg, OBJECT(pcms));
+    }
 }
 
 /* Looking for a pc_compat_2_4() function? It doesn't exist.
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 208a224..fc2d335 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -61,6 +61,7 @@ static void pc_q35_init(MachineState *machine)
     PCIDevice *lpc;
     BusState *idebus[MAX_SATA_PORTS];
     ISADevice *rtc_state;
+    MemoryRegion *system_io = get_system_io();
     MemoryRegion *pci_memory;
     MemoryRegion *rom_memory;
     MemoryRegion *ram_memory;
@@ -166,7 +167,7 @@ static void pc_q35_init(MachineState *machine)
     q35_host->mch.ram_memory = ram_memory;
     q35_host->mch.pci_address_space = pci_memory;
     q35_host->mch.system_memory = get_system_memory();
-    q35_host->mch.address_space_io = get_system_io();
+    q35_host->mch.address_space_io = system_io;
     q35_host->mch.below_4g_mem_size = pcms->below_4g_mem_size;
     q35_host->mch.above_4g_mem_size = pcms->above_4g_mem_size;
     /* pci */
@@ -257,6 +258,11 @@ static void pc_q35_init(MachineState *machine)
     if (pcmc->pci_enabled) {
         pc_pci_device_init(host_bus);
     }
+
+    if (pcms->acpi_nvdimm_state.is_enabled) {
+        nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,
+                               pcms->fw_cfg, OBJECT(pcms));
+    }
 }
 
 /* Looking for a pc_compat_2_4() function? It doesn't exist.
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 8b3546e..3937f54 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -17,6 +17,7 @@
 #include "hw/boards.h"
 #include "hw/compat.h"
 #include "hw/mem/pc-dimm.h"
+#include "hw/mem/nvdimm.h"
 
 #define HPET_INTCAP "hpet-intcap"
 
@@ -57,7 +58,8 @@ struct PCMachineState {
     uint64_t max_ram_below_4g;
     OnOffAuto vmport;
     OnOffAuto smm;
-    bool nvdimm;
+
+    AcpiNVDIMMState acpi_nvdimm_state;
 
     /* RAM information (sizes, addresses, configuration): */
     ram_addr_t below_4g_mem_size, above_4g_mem_size;
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index 49183c1..634c60b 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -25,8 +25,34 @@
 
 #include "hw/mem/pc-dimm.h"
 
-#define TYPE_NVDIMM      "nvdimm"
+#define TYPE_NVDIMM             "nvdimm"
 
+#define NVDIMM_DSM_MEM_FILE     "etc/acpi/nvdimm-mem"
+
+/*
+ * 32 bits IO port starting from 0x0a18 in guest is reserved for
+ * NVDIMM ACPI emulation.
+ */
+#define NVDIMM_ACPI_IO_BASE     0x0a18
+#define NVDIMM_ACPI_IO_LEN      4
+
+/*
+ * AcpiNVDIMMState:
+ * @is_enabled: detect if NVDIMM support is enabled.
+ *
+ * @dsm_mem: the data of the fw_cfg file NVDIMM_DSM_MEM_FILE.
+ * @io_mr: the IO region used by OSPM to transfer control to QEMU.
+ */
+struct AcpiNVDIMMState {
+    bool is_enabled;
+
+    GArray *dsm_mem;
+    MemoryRegion io_mr;
+};
+typedef struct AcpiNVDIMMState AcpiNVDIMMState;
+
+void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
+                            FWCfgState *fw_cfg, Object *owner);
 void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
                        GArray *linker);
 #endif
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 5/8] nvdimm acpi: introduce patched dsm memory

[Xiao Guangrong]

The dsm memory is used to save the input parameters and store
the dsm result which is filled by QEMU.

The address of dsm memory is decided by bios and patched into
int32 object named "MEMA"

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 8568b20..bca36ae 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -29,6 +29,7 @@
 #include "qemu/osdep.h"
 #include "hw/acpi/acpi.h"
 #include "hw/acpi/aml-build.h"
+#include "hw/acpi/bios-linker-loader.h"
 #include "hw/nvram/fw_cfg.h"
 #include "hw/mem/nvdimm.h"
 
@@ -406,6 +407,7 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
 }
 
 #define NVDIMM_COMMON_DSM      "NCAL"
+#define NVDIMM_ACPI_MEM_ADDR   "MEMA"
 
 static void nvdimm_build_common_dsm(Aml *dev)
 {
@@ -470,7 +472,9 @@ static void nvdimm_build_nvdimm_devices(GSList *device_list, Aml *root_dev)
 static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
                               GArray *table_data, GArray *linker)
 {
-    Aml *ssdt, *sb_scope, *dev;
+    Aml *ssdt, *sb_scope, *dev, *mem_addr;
+    uint32_t zero_offset = 0;
+    int offset;
 
     acpi_add_table(table_offsets, table_data);
 
@@ -501,9 +505,37 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
 
     aml_append(sb_scope, dev);
 
+    /*
+     * leave it at the end of ssdt so that we can conveniently get the
+     * offset of int32 object which will be patched with the real address
+     * of the dsm memory by BIOS.
+     *
+     * 0x32000000 is the magic number to let aml_int() create int32 object.
+     * It will be zeroed later to make bios_linker_loader_add_pointer()
+     * happy.
+     */
+    mem_addr = aml_name_decl(NVDIMM_ACPI_MEM_ADDR, aml_int(0x32000000));
+
+    aml_append(sb_scope, mem_addr);
     aml_append(ssdt, sb_scope);
     /* copy AML table into ACPI tables blob and patch header there */
     g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
+
+    offset = table_data->len - 4;
+
+    /*
+     * zero the last 4 bytes, i.e, it is the offset of
+     * NVDIMM_ACPI_MEM_ADDR object.
+     */
+    g_array_remove_range(table_data, offset, 4);
+    g_array_append_vals(table_data, &zero_offset, 4);
+
+    bios_linker_loader_alloc(linker, NVDIMM_DSM_MEM_FILE, TARGET_PAGE_SIZE,
+                             false /* high memory */);
+    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
+                                   NVDIMM_DSM_MEM_FILE, table_data,
+                                   table_data->data + offset,
+                                   sizeof(uint32_t));
     build_header(linker, table_data,
         (void *)(table_data->data + table_data->len - ssdt->buf->len),
         "SSDT", ssdt->buf->len, 1, NULL, "NVDIMM");
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 6/8] nvdimm acpi: let qemu handle _DSM method

[Xiao Guangrong]

If dsm memory is successfully patched, we let qemu fully emulate
the dsm method

This patch saves _DSM input parameters into dsm memory, tell dsm
memory address to QEMU, then fetch the result from the dsm memory

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 112 insertions(+), 5 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index bca36ae..3d9b3ab 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -372,6 +372,24 @@ static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets,
     g_array_free(structures, true);
 }
 
+struct NvdimmDsmIn {
+    uint32_t handle;
+    uint32_t revision;
+    uint32_t function;
+   /* the remaining size in the page is used by arg3. */
+    union {
+        uint8_t arg3[0];
+    };
+} QEMU_PACKED;
+typedef struct NvdimmDsmIn NvdimmDsmIn;
+
+struct NvdimmDsmOut {
+    /* the size of buffer filled by QEMU. */
+    uint32_t len;
+    uint8_t data[0];
+} QEMU_PACKED;
+typedef struct NvdimmDsmOut NvdimmDsmOut;
+
 static uint64_t
 nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
 {
@@ -411,11 +429,18 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
 
 static void nvdimm_build_common_dsm(Aml *dev)
 {
-    Aml *method, *ifctx, *function;
+    Aml *method, *ifctx, *function, *dsm_mem, *unpatched, *result_size;
     uint8_t byte_list[1];
 
-    method = aml_method(NVDIMM_COMMON_DSM, 4, AML_NOTSERIALIZED);
+    method = aml_method(NVDIMM_COMMON_DSM, 4, AML_SERIALIZED);
     function = aml_arg(2);
+    dsm_mem = aml_name(NVDIMM_ACPI_MEM_ADDR);
+
+    /*
+     * do not support any method if DSM memory address has not been
+     * patched.
+     */
+    unpatched = aml_if(aml_equal(dsm_mem, aml_int(0x0)));
 
     /*
      * function 0 is called to inquire what functions are supported by
@@ -424,12 +449,36 @@ static void nvdimm_build_common_dsm(Aml *dev)
     ifctx = aml_if(aml_equal(function, aml_int(0)));
     byte_list[0] = 0 /* No function Supported */;
     aml_append(ifctx, aml_return(aml_buffer(1, byte_list)));
-    aml_append(method, ifctx);
+    aml_append(unpatched, ifctx);
 
     /* No function is supported yet. */
     byte_list[0] = 1 /* Not Supported */;
-    aml_append(method, aml_return(aml_buffer(1, byte_list)));
+    aml_append(unpatched, aml_return(aml_buffer(1, byte_list)));
+    aml_append(method, unpatched);
+
+    /*
+     * Currently no function is supported for both root device and NVDIMM
+     * devices, let's temporarily set handle to 0x0 at this time.
+     */
+    aml_append(method, aml_store(aml_int(0x0), aml_name("HDLE")));
+    aml_append(method, aml_store(aml_arg(1), aml_name("REVS")));
+    aml_append(method, aml_store(aml_arg(2), aml_name("FUNC")));
 
+    /*
+     * tell QEMU about the real address of DSM memory, then QEMU begins
+     * to emulate the method and fills the result to DSM memory.
+     */
+    aml_append(method, aml_store(dsm_mem, aml_name("NTFI")));
+
+    result_size = aml_local(1);
+    aml_append(method, aml_store(aml_name("RLEN"), result_size));
+    aml_append(method, aml_store(aml_shiftleft(result_size, aml_int(3)),
+                                 result_size));
+    aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0),
+                                        result_size, "OBUF"));
+    aml_append(method, aml_concatenate(aml_buffer(0, NULL), aml_name("OBUF"),
+                                       aml_arg(6)));
+    aml_append(method, aml_return(aml_arg(6)));
     aml_append(dev, method);
 }
 
@@ -472,7 +521,7 @@ static void nvdimm_build_nvdimm_devices(GSList *device_list, Aml *root_dev)
 static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
                               GArray *table_data, GArray *linker)
 {
-    Aml *ssdt, *sb_scope, *dev, *mem_addr;
+    Aml *ssdt, *sb_scope, *dev, *field, *mem_addr;
     uint32_t zero_offset = 0;
     int offset;
 
@@ -498,6 +547,64 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
      */
     aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012")));
 
+    /* map DSM memory and IO into ACPI namespace. */
+    aml_append(dev, aml_operation_region("NPIO", AML_SYSTEM_IO,
+               aml_int(NVDIMM_ACPI_IO_BASE), NVDIMM_ACPI_IO_LEN));
+    aml_append(dev, aml_operation_region("NRAM", AML_SYSTEM_MEMORY,
+               aml_name(NVDIMM_ACPI_MEM_ADDR), TARGET_PAGE_SIZE));
+
+    /*
+     * DSM notifier:
+     * NTFI: write the address of DSM memory and notify QEMU to emulate
+     *       the access.
+     *
+     * It is the IO port so that accessing them will cause VM-exit, the
+     * control will be transferred to QEMU.
+     */
+    field = aml_field("NPIO", AML_DWORD_ACC, AML_NOLOCK, AML_PRESERVE);
+    aml_append(field, aml_named_field("NTFI",
+               sizeof(uint32_t) * BITS_PER_BYTE));
+    aml_append(dev, field);
+
+    /*
+     * DSM input:
+     * @HDLE: store device's handle, it's zero if the _DSM call happens
+     *        on NVDIMM Root Device.
+     * @REVS: store the Arg1 of _DSM call.
+     * @FUNC: store the Arg2 of _DSM call.
+     * @ARG3: store the Arg3 of _DSM call.
+     *
+     * They are RAM mapping on host so that these accesses never cause
+     * VM-EXIT.
+     */
+    field = aml_field("NRAM", AML_DWORD_ACC, AML_NOLOCK, AML_PRESERVE);
+    aml_append(field, aml_named_field("HDLE",
+               sizeof(typeof_field(NvdimmDsmIn, handle)) * BITS_PER_BYTE));
+    aml_append(field, aml_named_field("REVS",
+               sizeof(typeof_field(NvdimmDsmIn, revision)) * BITS_PER_BYTE));
+    aml_append(field, aml_named_field("FUNC",
+               sizeof(typeof_field(NvdimmDsmIn, function)) * BITS_PER_BYTE));
+    aml_append(field, aml_named_field("ARG3",
+               (TARGET_PAGE_SIZE - offsetof(NvdimmDsmIn, arg3)) *
+                     BITS_PER_BYTE));
+    aml_append(dev, field);
+
+    /*
+     * DSM output:
+     * @RLEN: the size of the buffer filled by QEMU.
+     * @ODAT: the buffer QEMU uses to store the result.
+     *
+     * Since the page is reused by both input and out, the input data
+     * will be lost after storing new result into @ODAT.
+    */
+    field = aml_field("NRAM", AML_DWORD_ACC, AML_NOLOCK, AML_PRESERVE);
+    aml_append(field, aml_named_field("RLEN",
+               sizeof(typeof_field(NvdimmDsmOut, len)) * BITS_PER_BYTE));
+    aml_append(field, aml_named_field("ODAT",
+               (TARGET_PAGE_SIZE - offsetof(NvdimmDsmOut, data)) *
+                     BITS_PER_BYTE));
+    aml_append(dev, field);
+
     nvdimm_build_common_dsm(dev);
     nvdimm_build_device_dsm(dev);
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 7/8] nvdimm acpi: emulate dsm method

[Xiao Guangrong]

Emulate dsm method after IO VM-exit

Currently, we only introduce the framework and no function is actually
supported

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/aml-build.c         |  2 +-
 hw/acpi/nvdimm.c            | 44 ++++++++++++++++++++++++++++++++++++++++++++
 include/hw/acpi/aml-build.h |  1 +
 include/hw/mem/nvdimm.h     |  8 ++++++++
 4 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index c4662f0..3e24ea5 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -227,7 +227,7 @@ static void build_extop_package(GArray *package, uint8_t op)
     build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
 }
 
-static void build_append_int_noprefix(GArray *table, uint64_t value, int size)
+void build_append_int_noprefix(GArray *table, uint64_t value, int size)
 {
     int i;
 
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 3d9b3ab..d6f067b 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -393,12 +393,56 @@ typedef struct NvdimmDsmOut NvdimmDsmOut;
 static uint64_t
 nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
 {
+    fprintf(stderr, "BUG: we never read _DSM IO Port.\n");
     return 0;
 }
 
 static void
 nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
 {
+    NvdimmDsmIn *in;
+    GArray *out;
+    uint32_t buf_size;
+    hwaddr dsm_mem_addr = val;
+
+    nvdimm_debug("dsm memory address %#lx.\n", dsm_mem_addr);
+
+    /*
+     * The DSM memory is mapped to guest address space so an evil guest
+     * can change its content while we are doing DSM emulation. Avoid
+     * this by copying DSM memory to QEMU local memory.
+     */
+    in = g_malloc(TARGET_PAGE_SIZE);
+    cpu_physical_memory_read(dsm_mem_addr, in, TARGET_PAGE_SIZE);
+
+    le32_to_cpus(&in->revision);
+    le32_to_cpus(&in->function);
+    le32_to_cpus(&in->handle);
+
+    nvdimm_debug("Revision %#x Handler %#x Function %#x.\n", in->revision,
+                 in->handle, in->function);
+
+    out = g_array_new(false, true /* clear */, 1);
+
+    /*
+     * function 0 is called to inquire what functions are supported by
+     * OSPM
+     */
+    if (in->function == 0) {
+        build_append_int_noprefix(out, 0 /* No function Supported */,
+                                  sizeof(uint8_t));
+    } else {
+        /* No function is supported yet. */
+        build_append_int_noprefix(out, 1 /* Not Supported */,
+                                  sizeof(uint8_t));
+    }
+
+    buf_size = cpu_to_le32(out->len);
+    cpu_physical_memory_write(dsm_mem_addr, &buf_size, sizeof(buf_size));
+    cpu_physical_memory_write(dsm_mem_addr + sizeof(buf_size), out->data,
+                              out->len);
+    g_free(in);
+    g_array_free(out, true);
 }
 
 static const MemoryRegionOps nvdimm_dsm_ops = {
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 07b2d48..d828199 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -357,6 +357,7 @@ Aml *aml_derefof(Aml *arg);
 Aml *aml_sizeof(Aml *arg);
 Aml *aml_concatenate(Aml *source1, Aml *source2, Aml *target);
 
+void build_append_int_noprefix(GArray *table, uint64_t value, int size);
 void
 build_header(GArray *linker, GArray *table_data,
              AcpiTableHeader *h, const char *sig, int len, uint8_t rev,
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index 634c60b..aaa2608 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -25,6 +25,14 @@
 
 #include "hw/mem/pc-dimm.h"
 
+#define NVDIMM_DEBUG 0
+#define nvdimm_debug(fmt, ...)                                \
+    do {                                                      \
+        if (NVDIMM_DEBUG) {                                   \
+            fprintf(stderr, "nvdimm: " fmt, ## __VA_ARGS__);  \
+        }                                                     \
+    } while (0)
+
 #define TYPE_NVDIMM             "nvdimm"
 
 #define NVDIMM_DSM_MEM_FILE     "etc/acpi/nvdimm-mem"
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 8/8] nvdimm acpi: add _CRS

[Xiao Guangrong]

As Igor suggested that we can report the BIOS patched operation region
so that OSPM could see that particular range is in use and be able to
notice conflicts if it happens some day

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index d6f067b..a6fbbee 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -566,6 +566,7 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
                               GArray *table_data, GArray *linker)
 {
     Aml *ssdt, *sb_scope, *dev, *field, *mem_addr;
+    Aml *min_addr, *max_addr, *mr32, *method, *crs;
     uint32_t zero_offset = 0;
     int offset;
 
@@ -591,6 +592,32 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
      */
     aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012")));
 
+    /*
+     * report the dsm memory so that OSPM could see that particular range is
+     * in use and be able to notice conflicts if it happens some day.
+     */
+    method = aml_method("_CRS", 0, AML_SERIALIZED);
+    crs = aml_resource_template();
+    aml_append(crs, aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
+                                     AML_MAX_FIXED, AML_CACHEABLE,
+                                     AML_READ_WRITE,
+                                     0, 0x0, 0xFFFFFFFE, 0,
+                                     TARGET_PAGE_SIZE));
+    aml_append(method, aml_name_decl("MR32", crs));
+    mr32 = aml_name("MR32");
+    aml_append(method, aml_create_dword_field(mr32, aml_int(10), "MIN"));
+    aml_append(method, aml_create_dword_field(mr32, aml_int(14), "MAX"));
+
+    min_addr = aml_name("MIN");
+    max_addr = aml_name("MAX");
+
+    aml_append(method, aml_store(aml_name(NVDIMM_ACPI_MEM_ADDR), min_addr));
+    aml_append(method, aml_add(min_addr, aml_int(TARGET_PAGE_SIZE),
+                               max_addr));
+    aml_append(method, aml_decrement(max_addr));
+    aml_append(method, aml_return(mr32));
+    aml_append(dev, method);
+
     /* map DSM memory and IO into ACPI namespace. */
     aml_append(dev, aml_operation_region("NPIO", AML_SYSTEM_IO,
                aml_int(NVDIMM_ACPI_IO_BASE), NVDIMM_ACPI_IO_LEN));
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 2/8] acpi: add aml_concatenate()

[Igor Mammedov]

On Sun, 14 Feb 2016 16:50:59 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> It will be used by nvdimm acpi
> 
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/acpi/aml-build.c         | 8 ++++++++
>  include/hw/acpi/aml-build.h | 1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 2cc3211..3362cc4 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1437,6 +1437,14 @@ Aml *aml_alias(const char *source_object, const char *alias_object)
>      return var;
>  }
>  
> +/* ACPI 1.0b: 16.2.5.4 Type 2 Opcodes Encoding: DefConcat */
> +Aml *aml_concatenate(Aml *source1, Aml *source2, Aml *target)
> +{
> +    assert(target);
you don't need assert here

> +    return build_opcode_2arg_dst(0x73 /* ConcatOp */, source1, source2,
> +                                 target);
> +}
> +
>  void
>  build_header(GArray *linker, GArray *table_data,
>               AcpiTableHeader *h, const char *sig, int len, uint8_t rev,
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 8ef10ad..735c34a 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -355,6 +355,7 @@ Aml *aml_touuid(const char *uuid);
>  Aml *aml_unicode(const char *str);
>  Aml *aml_derefof(Aml *arg);
>  Aml *aml_sizeof(Aml *arg);
> +Aml *aml_concatenate(Aml *source1, Aml *source2, Aml *target);
>  
>  void
>  build_header(GArray *linker, GArray *table_data,

Re: [Qemu-devel] [PATCH v3 5/8] nvdimm acpi: introduce patched dsm memory

[Michael S. Tsirkin]

On Sun, Feb 14, 2016 at 04:51:02PM +0800, Xiao Guangrong wrote:
> The dsm memory is used to save the input parameters and store
> the dsm result which is filled by QEMU.
> 
> The address of dsm memory is decided by bios and patched into
> int32 object named "MEMA"
> 
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>


This is a bit too hacky for my taste. First, I would prefer an explicit API
to add a DWORD. Second, I would like to avoid offset math hacks
and make API returning offsets.

Pls see below for a suggestion.


> ---
>  hw/acpi/nvdimm.c | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 8568b20..bca36ae 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -29,6 +29,7 @@
>  #include "qemu/osdep.h"
>  #include "hw/acpi/acpi.h"
>  #include "hw/acpi/aml-build.h"
> +#include "hw/acpi/bios-linker-loader.h"
>  #include "hw/nvram/fw_cfg.h"
>  #include "hw/mem/nvdimm.h"
>  
> @@ -406,6 +407,7 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
>  }
>  
>  #define NVDIMM_COMMON_DSM      "NCAL"
> +#define NVDIMM_ACPI_MEM_ADDR   "MEMA"
>  
>  static void nvdimm_build_common_dsm(Aml *dev)
>  {
> @@ -470,7 +472,9 @@ static void nvdimm_build_nvdimm_devices(GSList *device_list, Aml *root_dev)
>  static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
>                                GArray *table_data, GArray *linker)
>  {
> -    Aml *ssdt, *sb_scope, *dev;
> +    Aml *ssdt, *sb_scope, *dev, *mem_addr;
> +    uint32_t zero_offset = 0;
> +    int offset;
>  
>      acpi_add_table(table_offsets, table_data);
>  
> @@ -501,9 +505,37 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
>  
>      aml_append(sb_scope, dev);
>  
> +    /*
> +     * leave it at the end of ssdt so that we can conveniently get the
> +     * offset of int32 object which will be patched with the real address
> +     * of the dsm memory by BIOS.
> +     *
> +     * 0x32000000 is the magic number to let aml_int() create int32 object.
> +     * It will be zeroed later to make bios_linker_loader_add_pointer()
> +     * happy.
> +     */
> +    mem_addr = aml_name_decl(NVDIMM_ACPI_MEM_ADDR, aml_int(0x32000000));
> +
> +    aml_append(sb_scope, mem_addr);
>      aml_append(ssdt, sb_scope);
>      /* copy AML table into ACPI tables blob and patch header there */
>      g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
> +
> +    offset = table_data->len - 4;
> +
> +    /*
> +     * zero the last 4 bytes, i.e, it is the offset of
> +     * NVDIMM_ACPI_MEM_ADDR object.
> +     */
> +    g_array_remove_range(table_data, offset, 4);
> +    g_array_append_vals(table_data, &zero_offset, 4);
> +
> +    bios_linker_loader_alloc(linker, NVDIMM_DSM_MEM_FILE, TARGET_PAGE_SIZE,
> +                             false /* high memory */);
> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> +                                   NVDIMM_DSM_MEM_FILE, table_data,
> +                                   table_data->data + offset,
> +                                   sizeof(uint32_t));
>      build_header(linker, table_data,
>          (void *)(table_data->data + table_data->len - ssdt->buf->len),
>          "SSDT", ssdt->buf->len, 1, NULL, "NVDIMM");
> -- 
> 1.8.3.1



--->

acpi: add build_append_named_dword, returning an offset in buffer

This is a very limited form of support for runtime patching -
similar in functionality to what we can do with ACPI_EXTRACT
macros in python, but implemented in C.

This is to allow ACPI code direct access to data tables -
which is exactly what DataTableRegion is there for, except
no known windows release so far implements DataTableRegion.

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

---

diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 1b632dc..f8998ea 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -286,4 +286,7 @@ void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre);
 void
 build_rsdt(GArray *table_data, GArray *linker, GArray *table_offsets);
 
+int
+build_append_named_dword(GArray *array, const char *name_format, ...);
+
 #endif
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 0d4b324..7f9fa65 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -262,6 +262,32 @@ static void build_append_int(GArray *table, uint64_t value)
     }
 }
 
+/* Build NAME(XXXX, 0x00000000) where 0x00000000 is encoded as a dword,
+ * and return the offset to 0x00000000 for runtime patching.
+ *
+ * Warning: runtime patching is best avoided. Only use this as
+ * a replacement for DataTableRegion (for guests that don't
+ * support it).
+ */
+int
+build_append_named_dword(GArray *array, const char *name_format, ...)
+{
+    int offset;
+    va_list ap;
+
+    va_start(ap, name_format);
+    build_append_namestringv(array, name_format, ap);
+    va_end(ap);
+
+    build_append_byte(array, 0x0C); /* DWordPrefix */
+
+    offset = array->len;
+    build_append_int_noprefix(array, 0x00000000, 4);
+    assert(array->len == offset + 4);
+
+    return offset;
+}
+
 static GPtrArray *alloc_list;
 
 static Aml *aml_alloc(void)

Re: [Qemu-devel] [PATCH v3 5/8] nvdimm acpi: introduce patched dsm memory

[Xiao Guangrong]

[-- Attachment #1: Type: text/plain, Size: 5429 bytes --]



On 02/29/2016 05:38 PM, Michael S. Tsirkin wrote:
> On Sun, Feb 14, 2016 at 04:51:02PM +0800, Xiao Guangrong wrote:
>> The dsm memory is used to save the input parameters and store
>> the dsm result which is filled by QEMU.
>>
>> The address of dsm memory is decided by bios and patched into
>> int32 object named "MEMA"
>>
>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>
>
> This is a bit too hacky for my taste. First, I would prefer an explicit API
> to add a DWORD. Second, I would like to avoid offset math hacks
> and make API returning offsets.
>
> Pls see below for a suggestion.
>
>
>> ---
>>   hw/acpi/nvdimm.c | 34 +++++++++++++++++++++++++++++++++-
>>   1 file changed, 33 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
>> index 8568b20..bca36ae 100644
>> --- a/hw/acpi/nvdimm.c
>> +++ b/hw/acpi/nvdimm.c
>> @@ -29,6 +29,7 @@
>>   #include "qemu/osdep.h"
>>   #include "hw/acpi/acpi.h"
>>   #include "hw/acpi/aml-build.h"
>> +#include "hw/acpi/bios-linker-loader.h"
>>   #include "hw/nvram/fw_cfg.h"
>>   #include "hw/mem/nvdimm.h"
>>
>> @@ -406,6 +407,7 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
>>   }
>>
>>   #define NVDIMM_COMMON_DSM      "NCAL"
>> +#define NVDIMM_ACPI_MEM_ADDR   "MEMA"
>>
>>   static void nvdimm_build_common_dsm(Aml *dev)
>>   {
>> @@ -470,7 +472,9 @@ static void nvdimm_build_nvdimm_devices(GSList *device_list, Aml *root_dev)
>>   static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
>>                                 GArray *table_data, GArray *linker)
>>   {
>> -    Aml *ssdt, *sb_scope, *dev;
>> +    Aml *ssdt, *sb_scope, *dev, *mem_addr;
>> +    uint32_t zero_offset = 0;
>> +    int offset;
>>
>>       acpi_add_table(table_offsets, table_data);
>>
>> @@ -501,9 +505,37 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
>>
>>       aml_append(sb_scope, dev);
>>
>> +    /*
>> +     * leave it at the end of ssdt so that we can conveniently get the
>> +     * offset of int32 object which will be patched with the real address
>> +     * of the dsm memory by BIOS.
>> +     *
>> +     * 0x32000000 is the magic number to let aml_int() create int32 object.
>> +     * It will be zeroed later to make bios_linker_loader_add_pointer()
>> +     * happy.
>> +     */
>> +    mem_addr = aml_name_decl(NVDIMM_ACPI_MEM_ADDR, aml_int(0x32000000));
>> +
>> +    aml_append(sb_scope, mem_addr);
>>       aml_append(ssdt, sb_scope);
>>       /* copy AML table into ACPI tables blob and patch header there */
>>       g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
>> +
>> +    offset = table_data->len - 4;
>> +
>> +    /*
>> +     * zero the last 4 bytes, i.e, it is the offset of
>> +     * NVDIMM_ACPI_MEM_ADDR object.
>> +     */
>> +    g_array_remove_range(table_data, offset, 4);
>> +    g_array_append_vals(table_data, &zero_offset, 4);
>> +
>> +    bios_linker_loader_alloc(linker, NVDIMM_DSM_MEM_FILE, TARGET_PAGE_SIZE,
>> +                             false /* high memory */);
>> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>> +                                   NVDIMM_DSM_MEM_FILE, table_data,
>> +                                   table_data->data + offset,
>> +                                   sizeof(uint32_t));
>>       build_header(linker, table_data,
>>           (void *)(table_data->data + table_data->len - ssdt->buf->len),
>>           "SSDT", ssdt->buf->len, 1, NULL, "NVDIMM");
>> --
>> 1.8.3.1
>
>
>
> --->
>
> acpi: add build_append_named_dword, returning an offset in buffer
>
> This is a very limited form of support for runtime patching -
> similar in functionality to what we can do with ACPI_EXTRACT
> macros in python, but implemented in C.
>
> This is to allow ACPI code direct access to data tables -
> which is exactly what DataTableRegion is there for, except
> no known windows release so far implements DataTableRegion.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ---
>
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 1b632dc..f8998ea 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -286,4 +286,7 @@ void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre);
>   void
>   build_rsdt(GArray *table_data, GArray *linker, GArray *table_offsets);
>
> +int
> +build_append_named_dword(GArray *array, const char *name_format, ...);
> +
>   #endif
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 0d4b324..7f9fa65 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -262,6 +262,32 @@ static void build_append_int(GArray *table, uint64_t value)
>       }
>   }
>
> +/* Build NAME(XXXX, 0x00000000) where 0x00000000 is encoded as a dword,
> + * and return the offset to 0x00000000 for runtime patching.
> + *
> + * Warning: runtime patching is best avoided. Only use this as
> + * a replacement for DataTableRegion (for guests that don't
> + * support it).
> + */
> +int
> +build_append_named_dword(GArray *array, const char *name_format, ...)
> +{
> +    int offset;
> +    va_list ap;
> +
> +    va_start(ap, name_format);
> +    build_append_namestringv(array, name_format, ap);
> +    va_end(ap);
> +

The NameOP was missed here...

The idea is great and i fixed and applied it on the top this patchset, the patch
is attached, would it be good to you?


[-- Attachment #2: 0001-acpi-add-build_append_named_dword-returning-an-offse.patch --]
[-- Type: text/x-patch, Size: 3865 bytes --]

>From 29a6803d244bbec807bd1df08aff4483ea776c9b Mon Sep 17 00:00:00 2001
From: Michael S. Tsirkin <mst@redhat.com>
Date: Tue, 1 Mar 2016 16:33:49 +0800
Subject: [PATCH] acpi: add build_append_named_dword, returning an offset in
 buffer

This is a very limited form of support for runtime patching -
similar in functionality to what we can do with ACPI_EXTRACT
macros in python, but implemented in C.

This is to allow ACPI code direct access to data tables -
which is exactly what DataTableRegion is there for, except
no known windows release so far implements DataTableRegion.

[ Xiao: fixed missed NameOp and applied it to NVDIMM ACPI. ]

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/aml-build.c |  1 +
 hw/acpi/nvdimm.c    | 33 +++++++--------------------------
 2 files changed, 8 insertions(+), 26 deletions(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index f40b93e..9d97ce8 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -271,6 +271,7 @@ build_append_named_dword(GArray *array, const char *name_format, ...)
     int offset;
     va_list ap;
 
+    build_append_byte(array, 0x08); /* NameOp */
     va_start(ap, name_format);
     build_append_namestringv(array, name_format, ap);
     va_end(ap);
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index a6fbbee..fbdff76 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -565,10 +565,9 @@ static void nvdimm_build_nvdimm_devices(GSList *device_list, Aml *root_dev)
 static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
                               GArray *table_data, GArray *linker)
 {
-    Aml *ssdt, *sb_scope, *dev, *field, *mem_addr;
+    Aml *ssdt, *sb_scope, *dev, *field;
     Aml *min_addr, *max_addr, *mr32, *method, *crs;
-    uint32_t zero_offset = 0;
-    int offset;
+    int offset, table_len;
 
     acpi_add_table(table_offsets, table_data);
 
@@ -682,31 +681,13 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
     nvdimm_build_nvdimm_devices(device_list, dev);
 
     aml_append(sb_scope, dev);
+    aml_append(ssdt, sb_scope);
 
-    /*
-     * leave it at the end of ssdt so that we can conveniently get the
-     * offset of int32 object which will be patched with the real address
-     * of the dsm memory by BIOS.
-     *
-     * 0x32000000 is the magic number to let aml_int() create int32 object.
-     * It will be zeroed later to make bios_linker_loader_add_pointer()
-     * happy.
-     */
-    mem_addr = aml_name_decl(NVDIMM_ACPI_MEM_ADDR, aml_int(0x32000000));
+    table_len = table_data->len;
 
-    aml_append(sb_scope, mem_addr);
-    aml_append(ssdt, sb_scope);
     /* copy AML table into ACPI tables blob and patch header there */
     g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
-
-    offset = table_data->len - 4;
-
-    /*
-     * zero the last 4 bytes, i.e, it is the offset of
-     * NVDIMM_ACPI_MEM_ADDR object.
-     */
-    g_array_remove_range(table_data, offset, 4);
-    g_array_append_vals(table_data, &zero_offset, 4);
+    offset = build_append_named_dword(table_data, NVDIMM_ACPI_MEM_ADDR);
 
     bios_linker_loader_alloc(linker, NVDIMM_DSM_MEM_FILE, TARGET_PAGE_SIZE,
                              false /* high memory */);
@@ -715,8 +696,8 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
                                    table_data->data + offset,
                                    sizeof(uint32_t));
     build_header(linker, table_data,
-        (void *)(table_data->data + table_data->len - ssdt->buf->len),
-        "SSDT", ssdt->buf->len, 1, NULL, "NVDIMM");
+        (void *)(table_data->data + table_len),
+        "SSDT", table_data->len - table_len, 1, NULL, "NVDIMM");
     free_aml_allocator();
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 5/8] nvdimm acpi: introduce patched dsm memory

[Xiao Guangrong]

[-- Attachment #1: Type: text/plain, Size: 742 bytes --]



On 02/29/2016 05:38 PM, Michael S. Tsirkin wrote:

> +/* Build NAME(XXXX, 0x00000000) where 0x00000000 is encoded as a dword,
> + * and return the offset to 0x00000000 for runtime patching.
> + *
> + * Warning: runtime patching is best avoided. Only use this as
> + * a replacement for DataTableRegion (for guests that don't
> + * support it).
> + */
> +int
> +build_append_named_dword(GArray *array, const char *name_format, ...)
> +{
> +    int offset;
> +    va_list ap;
> +
> +    va_start(ap, name_format);
> +    build_append_namestringv(array, name_format, ap);
> +    va_end(ap);

The NameOP was missed here...

The idea is great and i fixed and applied it on the top this patchset, the patch
is attached, would it be good to you?


[-- Attachment #2: 0001-acpi-add-build_append_named_dword-returning-an-offse.patch --]
[-- Type: text/x-patch, Size: 3865 bytes --]

>From 29a6803d244bbec807bd1df08aff4483ea776c9b Mon Sep 17 00:00:00 2001
From: Michael S. Tsirkin <mst@redhat.com>
Date: Tue, 1 Mar 2016 16:33:49 +0800
Subject: [PATCH] acpi: add build_append_named_dword, returning an offset in
 buffer

This is a very limited form of support for runtime patching -
similar in functionality to what we can do with ACPI_EXTRACT
macros in python, but implemented in C.

This is to allow ACPI code direct access to data tables -
which is exactly what DataTableRegion is there for, except
no known windows release so far implements DataTableRegion.

[ Xiao: fixed missed NameOp and applied it to NVDIMM ACPI. ]

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/aml-build.c |  1 +
 hw/acpi/nvdimm.c    | 33 +++++++--------------------------
 2 files changed, 8 insertions(+), 26 deletions(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index f40b93e..9d97ce8 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -271,6 +271,7 @@ build_append_named_dword(GArray *array, const char *name_format, ...)
     int offset;
     va_list ap;
 
+    build_append_byte(array, 0x08); /* NameOp */
     va_start(ap, name_format);
     build_append_namestringv(array, name_format, ap);
     va_end(ap);
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index a6fbbee..fbdff76 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -565,10 +565,9 @@ static void nvdimm_build_nvdimm_devices(GSList *device_list, Aml *root_dev)
 static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
                               GArray *table_data, GArray *linker)
 {
-    Aml *ssdt, *sb_scope, *dev, *field, *mem_addr;
+    Aml *ssdt, *sb_scope, *dev, *field;
     Aml *min_addr, *max_addr, *mr32, *method, *crs;
-    uint32_t zero_offset = 0;
-    int offset;
+    int offset, table_len;
 
     acpi_add_table(table_offsets, table_data);
 
@@ -682,31 +681,13 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
     nvdimm_build_nvdimm_devices(device_list, dev);
 
     aml_append(sb_scope, dev);
+    aml_append(ssdt, sb_scope);
 
-    /*
-     * leave it at the end of ssdt so that we can conveniently get the
-     * offset of int32 object which will be patched with the real address
-     * of the dsm memory by BIOS.
-     *
-     * 0x32000000 is the magic number to let aml_int() create int32 object.
-     * It will be zeroed later to make bios_linker_loader_add_pointer()
-     * happy.
-     */
-    mem_addr = aml_name_decl(NVDIMM_ACPI_MEM_ADDR, aml_int(0x32000000));
+    table_len = table_data->len;
 
-    aml_append(sb_scope, mem_addr);
-    aml_append(ssdt, sb_scope);
     /* copy AML table into ACPI tables blob and patch header there */
     g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
-
-    offset = table_data->len - 4;
-
-    /*
-     * zero the last 4 bytes, i.e, it is the offset of
-     * NVDIMM_ACPI_MEM_ADDR object.
-     */
-    g_array_remove_range(table_data, offset, 4);
-    g_array_append_vals(table_data, &zero_offset, 4);
+    offset = build_append_named_dword(table_data, NVDIMM_ACPI_MEM_ADDR);
 
     bios_linker_loader_alloc(linker, NVDIMM_DSM_MEM_FILE, TARGET_PAGE_SIZE,
                              false /* high memory */);
@@ -715,8 +696,8 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
                                    table_data->data + offset,
                                    sizeof(uint32_t));
     build_header(linker, table_data,
-        (void *)(table_data->data + table_data->len - ssdt->buf->len),
-        "SSDT", ssdt->buf->len, 1, NULL, "NVDIMM");
+        (void *)(table_data->data + table_len),
+        "SSDT", table_data->len - table_len, 1, NULL, "NVDIMM");
     free_aml_allocator();
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 5/8] nvdimm acpi: introduce patched dsm memory

[Michael S. Tsirkin]

On Tue, Mar 01, 2016 at 04:53:23PM +0800, Xiao Guangrong wrote:
> 
> 
> On 02/29/2016 05:38 PM, Michael S. Tsirkin wrote:
> 
> >+/* Build NAME(XXXX, 0x00000000) where 0x00000000 is encoded as a dword,
> >+ * and return the offset to 0x00000000 for runtime patching.
> >+ *
> >+ * Warning: runtime patching is best avoided. Only use this as
> >+ * a replacement for DataTableRegion (for guests that don't
> >+ * support it).
> >+ */
> >+int
> >+build_append_named_dword(GArray *array, const char *name_format, ...)
> >+{
> >+    int offset;
> >+    va_list ap;
> >+
> >+    va_start(ap, name_format);
> >+    build_append_namestringv(array, name_format, ap);
> >+    va_end(ap);
> 
> The NameOP was missed here...
> 
> The idea is great and i fixed and applied it on the top this patchset, the patch
> is attached, would it be good to you?
> 

OK but I can't review this patch on top of patch.
Please split this in aml-build and nvdimm changes,
then squash the am-build change with my patch and include it
as 5/8, then append yours squashed with the nvdimm.c changes.


> >From 29a6803d244bbec807bd1df08aff4483ea776c9b Mon Sep 17 00:00:00 2001
> From: Michael S. Tsirkin <mst@redhat.com>
> Date: Tue, 1 Mar 2016 16:33:49 +0800
> Subject: [PATCH] acpi: add build_append_named_dword, returning an offset in
>  buffer
> 
> This is a very limited form of support for runtime patching -
> similar in functionality to what we can do with ACPI_EXTRACT
> macros in python, but implemented in C.
> 
> This is to allow ACPI code direct access to data tables -
> which is exactly what DataTableRegion is there for, except
> no known windows release so far implements DataTableRegion.
> 
> [ Xiao: fixed missed NameOp and applied it to NVDIMM ACPI. ]
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  hw/acpi/aml-build.c |  1 +
>  hw/acpi/nvdimm.c    | 33 +++++++--------------------------
>  2 files changed, 8 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index f40b93e..9d97ce8 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -271,6 +271,7 @@ build_append_named_dword(GArray *array, const char *name_format, ...)
>      int offset;
>      va_list ap;
>  
> +    build_append_byte(array, 0x08); /* NameOp */
>      va_start(ap, name_format);
>      build_append_namestringv(array, name_format, ap);
>      va_end(ap);
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index a6fbbee..fbdff76 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -565,10 +565,9 @@ static void nvdimm_build_nvdimm_devices(GSList *device_list, Aml *root_dev)
>  static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
>                                GArray *table_data, GArray *linker)
>  {
> -    Aml *ssdt, *sb_scope, *dev, *field, *mem_addr;
> +    Aml *ssdt, *sb_scope, *dev, *field;
>      Aml *min_addr, *max_addr, *mr32, *method, *crs;
> -    uint32_t zero_offset = 0;
> -    int offset;
> +    int offset, table_len;
>  
>      acpi_add_table(table_offsets, table_data);
>  
> @@ -682,31 +681,13 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
>      nvdimm_build_nvdimm_devices(device_list, dev);
>  
>      aml_append(sb_scope, dev);
> +    aml_append(ssdt, sb_scope);
>  
> -    /*
> -     * leave it at the end of ssdt so that we can conveniently get the
> -     * offset of int32 object which will be patched with the real address
> -     * of the dsm memory by BIOS.
> -     *
> -     * 0x32000000 is the magic number to let aml_int() create int32 object.
> -     * It will be zeroed later to make bios_linker_loader_add_pointer()
> -     * happy.
> -     */
> -    mem_addr = aml_name_decl(NVDIMM_ACPI_MEM_ADDR, aml_int(0x32000000));
> +    table_len = table_data->len;


Rename it something that implies what it does, not it's value. Offset of
what is it?

For example
	nvdimm_ssdt = table_data->len;



>  
> -    aml_append(sb_scope, mem_addr);
> -    aml_append(ssdt, sb_scope);
>      /* copy AML table into ACPI tables blob and patch header there */
>      g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
> -
> -    offset = table_data->len - 4;
> -
> -    /*
> -     * zero the last 4 bytes, i.e, it is the offset of
> -     * NVDIMM_ACPI_MEM_ADDR object.
> -     */
> -    g_array_remove_range(table_data, offset, 4);
> -    g_array_append_vals(table_data, &zero_offset, 4);
> +    offset = build_append_named_dword(table_data, NVDIMM_ACPI_MEM_ADDR);

Here too, please give it a better name
	mem_addr_offset = ....; ?

>  
>      bios_linker_loader_alloc(linker, NVDIMM_DSM_MEM_FILE, TARGET_PAGE_SIZE,
>                               false /* high memory */);
> @@ -715,8 +696,8 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
>                                     table_data->data + offset,
>                                     sizeof(uint32_t));
>      build_header(linker, table_data,
> -        (void *)(table_data->data + table_data->len - ssdt->buf->len),
> -        "SSDT", ssdt->buf->len, 1, NULL, "NVDIMM");
> +        (void *)(table_data->data + table_len),
> +        "SSDT", table_data->len - table_len, 1, NULL, "NVDIMM");
>      free_aml_allocator();
>  }
>  
> -- 
> 1.8.3.1
> 

Re: [Qemu-devel] [PATCH v3 5/8] nvdimm acpi: introduce patched dsm memory

[Xiao Guangrong]

On 03/01/2016 05:08 PM, Michael S. Tsirkin wrote:
> On Tue, Mar 01, 2016 at 04:53:23PM +0800, Xiao Guangrong wrote:
>>
>>
>> On 02/29/2016 05:38 PM, Michael S. Tsirkin wrote:
>>
>>> +/* Build NAME(XXXX, 0x00000000) where 0x00000000 is encoded as a dword,
>>> + * and return the offset to 0x00000000 for runtime patching.
>>> + *
>>> + * Warning: runtime patching is best avoided. Only use this as
>>> + * a replacement for DataTableRegion (for guests that don't
>>> + * support it).
>>> + */
>>> +int
>>> +build_append_named_dword(GArray *array, const char *name_format, ...)
>>> +{
>>> +    int offset;
>>> +    va_list ap;
>>> +
>>> +    va_start(ap, name_format);
>>> +    build_append_namestringv(array, name_format, ap);
>>> +    va_end(ap);
>>
>> The NameOP was missed here...
>>
>> The idea is great and i fixed and applied it on the top this patchset, the patch
>> is attached, would it be good to you?
>>
>
> OK but I can't review this patch on top of patch.
> Please split this in aml-build and nvdimm changes,
> then squash the am-build change with my patch and include it
> as 5/8, then append yours squashed with the nvdimm.c changes.

Okay... will do.


> Rename it something that implies what it does, not it's value. Offset of
> what is it?
>
> For example
> 	nvdimm_ssdt = table_data->len;

Yep, good to me.

>
>
>
>>
>> -    aml_append(sb_scope, mem_addr);
>> -    aml_append(ssdt, sb_scope);
>>       /* copy AML table into ACPI tables blob and patch header there */
>>       g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
>> -
>> -    offset = table_data->len - 4;
>> -
>> -    /*
>> -     * zero the last 4 bytes, i.e, it is the offset of
>> -     * NVDIMM_ACPI_MEM_ADDR object.
>> -     */
>> -    g_array_remove_range(table_data, offset, 4);
>> -    g_array_append_vals(table_data, &zero_offset, 4);
>> +    offset = build_append_named_dword(table_data, NVDIMM_ACPI_MEM_ADDR);
>
> Here too, please give it a better name
> 	mem_addr_offset = ....; ?

Yup, it is better.