[Qemu-devel] [PATCH v4 0/9] NVDIMM ACPI: introduce the framework of QEMU emulated

[Qemu-devel] [PATCH v4 0/9] NVDIMM ACPI: introduce the framework of QEMU emulated

[Xiao Guangrong]

This patchset is against commit 2212ef27b342b98b220fe9 (fw-cfg: support
writeable blobs) on pci branch of Michael's git tree
and can be found at:
      https://github.com/xiaogr/qemu.git nvdimm-acpi-v4

Changelog in v4:
- drop the unnecessary assert() in aml_concatenate() based on Igor's
  suggestion
- introduce build_append_named_dword() and use it to simplify the code as
  Michael's suggestion

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

Michael S. Tsirkin (1):
  acpi: add build_append_named_dword, returning an offset in buffer

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         |  55 +++++++++-
 hw/acpi/nvdimm.c            | 243 ++++++++++++++++++++++++++++++++++++++++++--
 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 |   9 +-
 include/hw/i386/pc.h        |   4 +-
 include/hw/mem/nvdimm.h     |  36 ++++++-
 10 files changed, 366 insertions(+), 43 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/9] 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 6675535..45b7f0a 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 2/9] 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         | 7 +++++++
 include/hw/acpi/aml-build.h | 1 +
 2 files changed, 8 insertions(+)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 45b7f0a..bb0cf52 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1437,6 +1437,13 @@ 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)
+{
+    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 3/9] 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 bb0cf52..f26fa26 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 b654b0d..6d8d23b 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 4/9] 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 6d8d23b..f8ff89d 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"
@@ -2582,13 +2581,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)
 {
@@ -2673,7 +2665,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 0aeefd2..5194acd 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1854,14 +1854,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)
@@ -1900,7 +1900,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 46522c9..17915b0 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;
@@ -160,7 +161,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 */
@@ -251,6 +252,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));
+    }
 }
 
 #define DEFINE_Q35_MACHINE(suffix, name, compatfn, optionfn) \
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 5/9] acpi: add build_append_named_dword, returning an offset in buffer

[Xiao Guangrong]

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

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>
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/aml-build.c         | 28 ++++++++++++++++++++++++++++
 include/hw/acpi/aml-build.h |  3 +++
 2 files changed, 31 insertions(+)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index f26fa26..ab89ca6 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -258,6 +258,34 @@ 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;
+
+    build_append_byte(array, 0x08); /* NameOp */
+    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)
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 07b2d48..7404e2a 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -370,4 +370,7 @@ void
 build_rsdt(GArray *table_data, GArray *linker, GArray *table_offsets,
            const char *oem_id, const char *oem_table_id);
 
+int
+build_append_named_dword(GArray *array, const char *name_format, ...);
+
 #endif
-- 
1.8.3.1

[Qemu-devel] [PATCH 6/9] 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 | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 8568b20..90032e5 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)
 {
@@ -471,6 +473,7 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
                               GArray *table_data, GArray *linker)
 {
     Aml *ssdt, *sb_scope, *dev;
+    int mem_addr_offset, nvdimm_ssdt;
 
     acpi_add_table(table_offsets, table_data);
 
@@ -500,13 +503,24 @@ 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);
+
+    nvdimm_ssdt = table_data->len;
+
     /* copy AML table into ACPI tables blob and patch header there */
     g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
+    mem_addr_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 */);
+    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
+                                   NVDIMM_DSM_MEM_FILE, table_data,
+                                   table_data->data + mem_addr_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 + nvdimm_ssdt),
+        "SSDT", table_data->len - nvdimm_ssdt, 1, NULL, "NVDIMM");
     free_aml_allocator();
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 7/9] 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 90032e5..781f6c1 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;
+    Aml *ssdt, *sb_scope, *dev, *field;
     int mem_addr_offset, nvdimm_ssdt;
 
     acpi_add_table(table_offsets, table_data);
@@ -497,6 +546,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 8/9] 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 ab89ca6..da11bf8 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 781f6c1..e0b483a 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 7404e2a..b0826f0 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 9/9] 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 e0b483a..a6359cc 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;
+    Aml *min_addr, *max_addr, *mr32, *method, *crs;
     int mem_addr_offset, nvdimm_ssdt;
 
     acpi_add_table(table_offsets, table_data);
@@ -590,6 +591,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 v4 0/9] NVDIMM ACPI: introduce the framework of QEMU emulated

[Michael S. Tsirkin]

On Tue, Mar 01, 2016 at 06:56:02PM +0800, Xiao Guangrong wrote:
> This patchset is against commit 2212ef27b342b98b220fe9 (fw-cfg: support
> writeable blobs) on pci branch of Michael's git tree
> and can be found at:
>       https://github.com/xiaogr/qemu.git nvdimm-acpi-v4

BTW, in the future pls version all patches.
E.g. git format-patch -v4 will put PATCH v4 on all subjects.

> Changelog in v4:
> - drop the unnecessary assert() in aml_concatenate() based on Igor's
>   suggestion
> - introduce build_append_named_dword() and use it to simplify the code as
>   Michael's suggestion
> 
> 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
> 
> Michael S. Tsirkin (1):
>   acpi: add build_append_named_dword, returning an offset in buffer
> 
> 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         |  55 +++++++++-
>  hw/acpi/nvdimm.c            | 243 ++++++++++++++++++++++++++++++++++++++++++--
>  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 |   9 +-
>  include/hw/i386/pc.h        |   4 +-
>  include/hw/mem/nvdimm.h     |  36 ++++++-
>  10 files changed, 366 insertions(+), 43 deletions(-)
> 
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 8/9] nvdimm acpi: emulate dsm method

[Michael S. Tsirkin]

On Tue, Mar 01, 2016 at 06:56:10PM +0800, Xiao Guangrong wrote:
> 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 ab89ca6..da11bf8 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 781f6c1..e0b483a 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;
>  }

Can't guest trigger this?
If yes, don't put such code in production please:
this will fill up disk on the host.


>  
>  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));

is there a race here?
can guest read this before data is written?

> +    cpu_physical_memory_write(dsm_mem_addr + sizeof(buf_size), out->data,
> +                              out->len);

What is this doing?
Is this actually writing AML bytecode into guest memory?


> +    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 7404e2a..b0826f0 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

Re: [Qemu-devel] [PATCH 8/9] nvdimm acpi: emulate dsm method

[Michael S. Tsirkin]

On Tue, Mar 01, 2016 at 06:56:10PM +0800, Xiao Guangrong wrote:
> 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 ab89ca6..da11bf8 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 781f6c1..e0b483a 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);

BTW, how do we know buffer is big enough? Add assert here?

Also, you have a packed structure with the layout, correct?
Can't you use that instead of open-coding it?

> +    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 7404e2a..b0826f0 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

Re: [Qemu-devel] [PATCH v4 0/9] NVDIMM ACPI: introduce the framework of QEMU emulated

[Michael S. Tsirkin]

On Tue, Mar 01, 2016 at 06:56:02PM +0800, Xiao Guangrong wrote:
> This patchset is against commit 2212ef27b342b98b220fe9 (fw-cfg: support
> writeable blobs) on pci branch of Michael's git tree
> and can be found at:
>       https://github.com/xiaogr/qemu.git nvdimm-acpi-v4


Applied patch 1-3 and 5

> Changelog in v4:
> - drop the unnecessary assert() in aml_concatenate() based on Igor's
>   suggestion
> - introduce build_append_named_dword() and use it to simplify the code as
>   Michael's suggestion
> 
> 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
> 
> Michael S. Tsirkin (1):
>   acpi: add build_append_named_dword, returning an offset in buffer
> 
> 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         |  55 +++++++++-
>  hw/acpi/nvdimm.c            | 243 ++++++++++++++++++++++++++++++++++++++++++--
>  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 |   9 +-
>  include/hw/i386/pc.h        |   4 +-
>  include/hw/mem/nvdimm.h     |  36 ++++++-
>  10 files changed, 366 insertions(+), 43 deletions(-)
> 
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 8/9] nvdimm acpi: emulate dsm method

[Xiao Guangrong]

On 03/02/2016 01:09 AM, Michael S. Tsirkin wrote:

>
> Can't guest trigger this?
> If yes, don't put such code in production please:
> this will fill up disk on the host.
>

Okay, the evil guest can read the IO port freely. I will use nvdimm_debug() instead.

>
>>
>>   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));
>
> is there a race here?
> can guest read this before data is written?

I think no.

It is the SERIALIZED DSM so there is no race in guest. And the CPU has exited
from guest mode when we fill the buffer in the same CPU-context so the guest
can not read the buffer at this point also memory-barrier is not needed here.

>
>> +    cpu_physical_memory_write(dsm_mem_addr + sizeof(buf_size), out->data,
>> +                              out->len);
>
> What is this doing?
> Is this actually writing AML bytecode into guest memory?

The layout of result written into the buffer is like this:
struct NvdimmDsmOut {
     /* the size of buffer filled by QEMU. */
     uint32_t len;
     uint8_t data[0];
} QEMU_PACKED;
typedef struct NvdimmDsmOut NvdimmDsmOut;

So the first cpu_physical_memory_write() writes the @len and the second one you
pointed out writes the real payload.

Re: [Qemu-devel] [PATCH 8/9] nvdimm acpi: emulate dsm method

[Xiao Guangrong]

On 03/02/2016 01:12 AM, Michael S. Tsirkin wrote:
> On Tue, Mar 01, 2016 at 06:56:10PM +0800, Xiao Guangrong wrote:
>> 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 ab89ca6..da11bf8 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 781f6c1..e0b483a 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);
>
> BTW, how do we know buffer is big enough? Add assert here?

I planed to do it when we introduce the real handler of NVDIMM command, but yes,
it is better doing it in this patchset. Will follow it in the next version.

>
> Also, you have a packed structure with the layout, correct?
> Can't you use that instead of open-coding it?


Okay, how about do it like this:

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index a6359cc..2812f7a 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -401,7 +401,7 @@ static void
  nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
  {
      NvdimmDsmIn *in;
-    GArray *out;
+    NvdimmDsmOut *out;
      uint32_t buf_size;
      hwaddr dsm_mem_addr = val;

@@ -422,27 +422,33 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
      nvdimm_debug("Revision %#x Handler %#x Function %#x.\n", in->revision,
                   in->handle, in->function);

-    out = g_array_new(false, true /* clear */, 1);
+    out = g_malloc(TARGET_PAGE_SIZE);
+
+    out->len = sizeof(out);

      /*
       * 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));
+        /* No function Supported */
+        uint32_t cmd_list = cpu_to_le32(0);
+
+        out->len += sizeof(cmd_list);
      } else {
-        /* No function is supported yet. */
-        build_append_int_noprefix(out, 1 /* Not Supported */,
-                                  sizeof(uint8_t));
+        /* Not Supported */
+        uint32_t status =  cpu_to_le32(1);
+
+        out->len = sizeof(status);
      }

-    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);
+    buf_size = out->len;
+    assert(buf_size <= TARGET_PAGE_SIZE);
+
+    out->len = cpu_to_le32(out->len);
+    cpu_physical_memory_write(dsm_mem_addr, out, buf_size);
      g_free(in);
-    g_array_free(out, true);
+    g_free(out);
  }

  static const MemoryRegionOps nvdimm_dsm_ops = {

Re: [Qemu-devel] [PATCH v4 0/9] NVDIMM ACPI: introduce the framework of QEMU emulated

[Xiao Guangrong]

On 03/02/2016 12:36 AM, Michael S. Tsirkin wrote:
> On Tue, Mar 01, 2016 at 06:56:02PM +0800, Xiao Guangrong wrote:
>> This patchset is against commit 2212ef27b342b98b220fe9 (fw-cfg: support
>> writeable blobs) on pci branch of Michael's git tree
>> and can be found at:
>>        https://github.com/xiaogr/qemu.git nvdimm-acpi-v4
>
> BTW, in the future pls version all patches.
> E.g. git format-patch -v4 will put PATCH v4 on all subjects.

I always manually made the version info. Thank you for sharing the tip, it is very
useful to me.

Re: [Qemu-devel] [PATCH 8/9] nvdimm acpi: emulate dsm method

[Michael S. Tsirkin]

On Wed, Mar 02, 2016 at 11:30:10AM +0800, Xiao Guangrong wrote:
> 
> 
> On 03/02/2016 01:09 AM, Michael S. Tsirkin wrote:
> 
> >
> >Can't guest trigger this?
> >If yes, don't put such code in production please:
> >this will fill up disk on the host.
> >
> 
> Okay, the evil guest can read the IO port freely. I will use nvdimm_debug() instead.
> 
> >
> >>
> >>  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);

ugh. manual memory management :(

> >>+    cpu_physical_memory_read(dsm_mem_addr, in, TARGET_PAGE_SIZE);

is there a requirement address is aligned?
if not this might cross page and crash qemu.
better read just what you need.

> >>+
> >>+    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);

export build_alloc_array then, and reuse?

> >>+
> >>+    /*
> >>+     * 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));

What does this mean? Same comment here and below ...


> >>+    } 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));
> >
> >is there a race here?
> >can guest read this before data is written?
> 
> I think no.
> 
> It is the SERIALIZED DSM so there is no race in guest. And the CPU has exited
> from guest mode when we fill the buffer in the same CPU-context so the guest
> can not read the buffer at this point also memory-barrier is not needed here.
> 
> >
> >>+    cpu_physical_memory_write(dsm_mem_addr + sizeof(buf_size), out->data,
> >>+                              out->len);
> >
> >What is this doing?
> >Is this actually writing AML bytecode into guest memory?
> 
> The layout of result written into the buffer is like this:
> struct NvdimmDsmOut {
>     /* the size of buffer filled by QEMU. */
>     uint32_t len;
>     uint8_t data[0];
> } QEMU_PACKED;
> typedef struct NvdimmDsmOut NvdimmDsmOut;
> 
> So the first cpu_physical_memory_write() writes the @len and the second one you
> pointed out writes the real payload.


So either write a function that gets parameters and formats
buffer, or use a structure to do this.
Do not open-code formatting and don't mess with
offsets.

E.g.

struct NvdimmDsmFunc0Out {
     /* the size of buffer filled by QEMU. */
     uint32_t len;
     uint8_t supported;
} QEMU_PACKED;
typedef struct NvdimmDsmFunc0Out NvdimmDsmFunc0Out;


And now

NvdimmDsmFunc0Out func0 = { .len = cpu_to_le32(sizeof(func0)); suppported = func == 0; };

cpu_physical_memory_write(dsm_mem_addr, &func0, sizeof func0);


Or if you really insist on using GArray:

build_dsm_out_func0(int function...)
{
    uint32_t len;
    uint8_t result;

    len = sizeof result;
    if (function == 0) {
        result = 0 /* No function Supported */;
   } else {
        /* No function is supported yet. */
        result = 1 /* Not Supported */;
   }

    build_append_int_noprefix(out, len, sizeof len);
    build_append_int_noprefix(out, result, sizeof result);

    assert(out->len < PAGE_SIZE); - is this right?
    cpu_physical_memory_write(dsm_mem_addr, out->data,
                              out->len);
}


but I prefer the former ...

-- 
MST

Re: [Qemu-devel] [PATCH 8/9] nvdimm acpi: emulate dsm method

[Michael S. Tsirkin]

On Wed, Mar 02, 2016 at 12:00:42PM +0800, Xiao Guangrong wrote:
> 
> 
> On 03/02/2016 01:12 AM, Michael S. Tsirkin wrote:
> >On Tue, Mar 01, 2016 at 06:56:10PM +0800, Xiao Guangrong wrote:
> >>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 ab89ca6..da11bf8 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 781f6c1..e0b483a 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);
> >
> >BTW, how do we know buffer is big enough? Add assert here?
> 
> I planed to do it when we introduce the real handler of NVDIMM command, but yes,
> it is better doing it in this patchset. Will follow it in the next version.
> 
> >
> >Also, you have a packed structure with the layout, correct?
> >Can't you use that instead of open-coding it?
> 
> 
> Okay, how about do it like this:
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index a6359cc..2812f7a 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -401,7 +401,7 @@ static void
>  nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>  {
>      NvdimmDsmIn *in;
> -    GArray *out;
> +    NvdimmDsmOut *out;
>      uint32_t buf_size;
>      hwaddr dsm_mem_addr = val;
> 
> @@ -422,27 +422,33 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>      nvdimm_debug("Revision %#x Handler %#x Function %#x.\n", in->revision,
>                   in->handle, in->function);
> 
> -    out = g_array_new(false, true /* clear */, 1);
> +    out = g_malloc(TARGET_PAGE_SIZE);
> +
> +    out->len = sizeof(out);
> 
>      /*
>       * 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));
> +        /* No function Supported */
> +        uint32_t cmd_list = cpu_to_le32(0);
> +
> +        out->len += sizeof(cmd_list);
>      } else {
> -        /* No function is supported yet. */
> -        build_append_int_noprefix(out, 1 /* Not Supported */,
> -                                  sizeof(uint8_t));
> +        /* Not Supported */
> +        uint32_t status =  cpu_to_le32(1);
> +
> +        out->len = sizeof(status);
>      }
> 
> -    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);
> +    buf_size = out->len;
> +    assert(buf_size <= TARGET_PAGE_SIZE);
> +
> +    out->len = cpu_to_le32(out->len);
> +    cpu_physical_memory_write(dsm_mem_addr, out, buf_size);
>      g_free(in);
> -    g_array_free(out, true);
> +    g_free(out);
>  }
> 
>  static const MemoryRegionOps nvdimm_dsm_ops = {

Not really much better. I suggested a better version in
another thread.

Re: [Qemu-devel] [PATCH 8/9] nvdimm acpi: emulate dsm method

[Xiao Guangrong]

On 03/02/2016 02:36 PM, Michael S. Tsirkin wrote:
> On Wed, Mar 02, 2016 at 11:30:10AM +0800, Xiao Guangrong wrote:
>>
>>
>> On 03/02/2016 01:09 AM, Michael S. Tsirkin wrote:
>>
>>>
>>> Can't guest trigger this?
>>> If yes, don't put such code in production please:
>>> this will fill up disk on the host.
>>>
>>
>> Okay, the evil guest can read the IO port freely. I will use nvdimm_debug() instead.
>>
>>>
>>>>
>>>>   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);
>
> ugh. manual memory management :(
>

Hmm... Or use GArray? But it is :)

>>>> +    cpu_physical_memory_read(dsm_mem_addr, in, TARGET_PAGE_SIZE);
>
> is there a requirement address is aligned?
> if not this might cross page and crash qemu.
> better read just what you need.
>

Yes, this memory is allocated by BIOS and we asked it to align the memory
with PAGE_SIZE:

     bios_linker_loader_alloc(linker, NVDIMM_DSM_MEM_FILE, TARGET_PAGE_SIZE,
                              false /* high memory */);

>>>> +
>>>> +    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);
>
> export build_alloc_array then, and reuse?

It is good to me, but as your suggestions, this code will be removed.

>
>>>> +
>>>> +    /*
>>>> +     * 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));
>
> What does this mean? Same comment here and below ...

If its the function 0, we return 0 that indicates no command is supported yet.
Other wise, it is a command request from a evil guest regardless of the result
returned by function 0, we return the status code 1 to indicates this command
is not supported.

>
>
>>>> +    } 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));
>>>
>>> is there a race here?
>>> can guest read this before data is written?
>>
>> I think no.
>>
>> It is the SERIALIZED DSM so there is no race in guest. And the CPU has exited
>> from guest mode when we fill the buffer in the same CPU-context so the guest
>> can not read the buffer at this point also memory-barrier is not needed here.
>>
>>>
>>>> +    cpu_physical_memory_write(dsm_mem_addr + sizeof(buf_size), out->data,
>>>> +                              out->len);
>>>
>>> What is this doing?
>>> Is this actually writing AML bytecode into guest memory?
>>
>> The layout of result written into the buffer is like this:
>> struct NvdimmDsmOut {
>>      /* the size of buffer filled by QEMU. */
>>      uint32_t len;
>>      uint8_t data[0];
>> } QEMU_PACKED;
>> typedef struct NvdimmDsmOut NvdimmDsmOut;
>>
>> So the first cpu_physical_memory_write() writes the @len and the second one you
>> pointed out writes the real payload.
>
>
> So either write a function that gets parameters and formats
> buffer, or use a structure to do this.
> Do not open-code formatting and don't mess with
> offsets.
>
> E.g.
>
> struct NvdimmDsmFunc0Out {
>       /* the size of buffer filled by QEMU. */
>       uint32_t len;
>       uint8_t supported;
> } QEMU_PACKED;
> typedef struct NvdimmDsmFunc0Out NvdimmDsmFunc0Out;
>
>
> And now
>
> NvdimmDsmFunc0Out func0 = { .len = cpu_to_le32(sizeof(func0)); suppported = func == 0; };
>
> cpu_physical_memory_write(dsm_mem_addr, &func0, sizeof func0);
>
>
> Or if you really insist on using GArray:
>
> build_dsm_out_func0(int function...)
> {
>      uint32_t len;
>      uint8_t result;
>
>      len = sizeof result;
>      if (function == 0) {
>          result = 0 /* No function Supported */;
>     } else {
>          /* No function is supported yet. */
>          result = 1 /* Not Supported */;
>     }
>
>      build_append_int_noprefix(out, len, sizeof len);
>      build_append_int_noprefix(out, result, sizeof result);
>
>      assert(out->len < PAGE_SIZE); - is this right?
>      cpu_physical_memory_write(dsm_mem_addr, out->data,
>                                out->len);
> }
>
>
> but I prefer the former ...
>

Okay, i prefer the former too ;).

Thank you, Michael!

Re: [Qemu-devel] [PATCH 8/9] nvdimm acpi: emulate dsm method

[Michael S. Tsirkin]

On Wed, Mar 02, 2016 at 03:15:19PM +0800, Xiao Guangrong wrote:
> 
> 
> On 03/02/2016 02:36 PM, Michael S. Tsirkin wrote:
> >On Wed, Mar 02, 2016 at 11:30:10AM +0800, Xiao Guangrong wrote:
> >>
> >>
> >>On 03/02/2016 01:09 AM, Michael S. Tsirkin wrote:
> >>
> >>>
> >>>Can't guest trigger this?
> >>>If yes, don't put such code in production please:
> >>>this will fill up disk on the host.
> >>>
> >>
> >>Okay, the evil guest can read the IO port freely. I will use nvdimm_debug() instead.
> >>
> >>>
> >>>>
> >>>>  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);
> >
> >ugh. manual memory management :(
> >
> 
> Hmm... Or use GArray? But it is :)
> 
> >>>>+    cpu_physical_memory_read(dsm_mem_addr, in, TARGET_PAGE_SIZE);
> >
> >is there a requirement address is aligned?
> >if not this might cross page and crash qemu.
> >better read just what you need.
> >
> 
> Yes, this memory is allocated by BIOS and we asked it to align the memory
> with PAGE_SIZE:
> 
>     bios_linker_loader_alloc(linker, NVDIMM_DSM_MEM_FILE, TARGET_PAGE_SIZE,
>                              false /* high memory */);
> 
> >>>>+
> >>>>+    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);
> >
> >export build_alloc_array then, and reuse?
> 
> It is good to me, but as your suggestions, this code will be removed.
> 
> >
> >>>>+
> >>>>+    /*
> >>>>+     * 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));
> >
> >What does this mean? Same comment here and below ...
> 
> If its the function 0, we return 0 that indicates no command is supported yet.

first comment says no function supported.
clearly function 0 is supported, is it not?
how exactly does 0 indicate no command is supported?
is it a bitmask of supported commands?

> Other wise, it is a command request from a evil guest regardless of the result
> returned by function 0, we return the status code 1 to indicates this command
> is not supported.

is command same as function?

> 
> >
> >
> >>>>+    } 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));
> >>>
> >>>is there a race here?
> >>>can guest read this before data is written?
> >>
> >>I think no.
> >>
> >>It is the SERIALIZED DSM so there is no race in guest. And the CPU has exited
> >>from guest mode when we fill the buffer in the same CPU-context so the guest
> >>can not read the buffer at this point also memory-barrier is not needed here.
> >>
> >>>
> >>>>+    cpu_physical_memory_write(dsm_mem_addr + sizeof(buf_size), out->data,
> >>>>+                              out->len);
> >>>
> >>>What is this doing?
> >>>Is this actually writing AML bytecode into guest memory?
> >>
> >>The layout of result written into the buffer is like this:
> >>struct NvdimmDsmOut {
> >>     /* the size of buffer filled by QEMU. */
> >>     uint32_t len;
> >>     uint8_t data[0];
> >>} QEMU_PACKED;
> >>typedef struct NvdimmDsmOut NvdimmDsmOut;
> >>
> >>So the first cpu_physical_memory_write() writes the @len and the second one you
> >>pointed out writes the real payload.
> >
> >
> >So either write a function that gets parameters and formats
> >buffer, or use a structure to do this.
> >Do not open-code formatting and don't mess with
> >offsets.
> >
> >E.g.
> >
> >struct NvdimmDsmFunc0Out {
> >      /* the size of buffer filled by QEMU. */
> >      uint32_t len;
> >      uint8_t supported;
> >} QEMU_PACKED;
> >typedef struct NvdimmDsmFunc0Out NvdimmDsmFunc0Out;
> >
> >
> >And now
> >
> >NvdimmDsmFunc0Out func0 = { .len = cpu_to_le32(sizeof(func0)); suppported = func == 0; };
> >
> >cpu_physical_memory_write(dsm_mem_addr, &func0, sizeof func0);
> >
> >
> >Or if you really insist on using GArray:
> >
> >build_dsm_out_func0(int function...)
> >{
> >     uint32_t len;
> >     uint8_t result;
> >
> >     len = sizeof result;
> >     if (function == 0) {
> >         result = 0 /* No function Supported */;
> >    } else {
> >         /* No function is supported yet. */
> >         result = 1 /* Not Supported */;
> >    }
> >
> >     build_append_int_noprefix(out, len, sizeof len);
> >     build_append_int_noprefix(out, result, sizeof result);
> >
> >     assert(out->len < PAGE_SIZE); - is this right?
> >     cpu_physical_memory_write(dsm_mem_addr, out->data,
> >                               out->len);
> >}
> >
> >
> >but I prefer the former ...
> >
> 
> Okay, i prefer the former too ;).
> 
> Thank you, Michael!
> 

Re: [Qemu-devel] [PATCH 8/9] nvdimm acpi: emulate dsm method

[Xiao Guangrong]

On 03/02/2016 03:15 PM, Xiao Guangrong wrote:

>>>>> +    in = g_malloc(TARGET_PAGE_SIZE);
>>
>> ugh. manual memory management :(
>>
>
> Hmm... Or use GArray? But it is :)

Sorry, typo.

But it is the static size and we should read all memory out to get
the consistent data to avid guest changes it at anytime. It is no
much room to improve it i think...

Re: [Qemu-devel] [PATCH 8/9] nvdimm acpi: emulate dsm method

[Xiao Guangrong]

On 03/02/2016 03:20 PM, Michael S. Tsirkin wrote:
> On Wed, Mar 02, 2016 at 03:15:19PM +0800, Xiao Guangrong wrote:
>>
>>
>> On 03/02/2016 02:36 PM, Michael S. Tsirkin wrote:
>>> On Wed, Mar 02, 2016 at 11:30:10AM +0800, Xiao Guangrong wrote:
>>>>
>>>>
>>>> On 03/02/2016 01:09 AM, Michael S. Tsirkin wrote:
>>>>
>>>>>
>>>>> Can't guest trigger this?
>>>>> If yes, don't put such code in production please:
>>>>> this will fill up disk on the host.
>>>>>
>>>>
>>>> Okay, the evil guest can read the IO port freely. I will use nvdimm_debug() instead.
>>>>
>>>>>
>>>>>>
>>>>>>   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);
>>>
>>> ugh. manual memory management :(
>>>
>>
>> Hmm... Or use GArray? But it is :)
>>
>>>>>> +    cpu_physical_memory_read(dsm_mem_addr, in, TARGET_PAGE_SIZE);
>>>
>>> is there a requirement address is aligned?
>>> if not this might cross page and crash qemu.
>>> better read just what you need.
>>>
>>
>> Yes, this memory is allocated by BIOS and we asked it to align the memory
>> with PAGE_SIZE:
>>
>>      bios_linker_loader_alloc(linker, NVDIMM_DSM_MEM_FILE, TARGET_PAGE_SIZE,
>>                               false /* high memory */);
>>
>>>>>> +
>>>>>> +    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);
>>>
>>> export build_alloc_array then, and reuse?
>>
>> It is good to me, but as your suggestions, this code will be removed.
>>
>>>
>>>>>> +
>>>>>> +    /*
>>>>>> +     * 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));
>>>
>>> What does this mean? Same comment here and below ...
>>
>> If its the function 0, we return 0 that indicates no command is supported yet.
>
> first comment says no function supported.
> clearly function 0 is supported, is it not?

Yep, the comment is not clear here. It should be "No function Supported other
than function 0 "

Function 0 is the common function supported by all DSMs to inquire what functions are
supported by this DSM.

> how exactly does 0 indicate no command is supported?
> is it a bitmask of supported commands?

It is a bitmask. The spec said:

If Function Index is zero, the return is a buffer containing one bit for each function
index, starting with zero. Bit 0 indicates whether there is support for any functions other
than function 0 for the specified UUID and Revision ID. If set to zero, no functions are
supported (other than function zero) for the specified UUID and Revision ID.

>
>> Other wise, it is a command request from a evil guest regardless of the result
>> returned by function 0, we return the status code 1 to indicates this command
>> is not supported.
>
> is command same as function?

Yes.

Re: [Qemu-devel] [PATCH 8/9] nvdimm acpi: emulate dsm method

[Michael S. Tsirkin]

On Wed, Mar 02, 2016 at 03:29:33PM +0800, Xiao Guangrong wrote:
> 
> 
> On 03/02/2016 03:20 PM, Michael S. Tsirkin wrote:
> >On Wed, Mar 02, 2016 at 03:15:19PM +0800, Xiao Guangrong wrote:
> >>
> >>
> >>On 03/02/2016 02:36 PM, Michael S. Tsirkin wrote:
> >>>On Wed, Mar 02, 2016 at 11:30:10AM +0800, Xiao Guangrong wrote:
> >>>>
> >>>>
> >>>>On 03/02/2016 01:09 AM, Michael S. Tsirkin wrote:
> >>>>
> >>>>>
> >>>>>Can't guest trigger this?
> >>>>>If yes, don't put such code in production please:
> >>>>>this will fill up disk on the host.
> >>>>>
> >>>>
> >>>>Okay, the evil guest can read the IO port freely. I will use nvdimm_debug() instead.
> >>>>
> >>>>>
> >>>>>>
> >>>>>>  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);
> >>>
> >>>ugh. manual memory management :(
> >>>
> >>
> >>Hmm... Or use GArray? But it is :)
> >>
> >>>>>>+    cpu_physical_memory_read(dsm_mem_addr, in, TARGET_PAGE_SIZE);
> >>>
> >>>is there a requirement address is aligned?
> >>>if not this might cross page and crash qemu.
> >>>better read just what you need.
> >>>
> >>
> >>Yes, this memory is allocated by BIOS and we asked it to align the memory
> >>with PAGE_SIZE:
> >>
> >>     bios_linker_loader_alloc(linker, NVDIMM_DSM_MEM_FILE, TARGET_PAGE_SIZE,
> >>                              false /* high memory */);
> >>
> >>>>>>+
> >>>>>>+    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);
> >>>
> >>>export build_alloc_array then, and reuse?
> >>
> >>It is good to me, but as your suggestions, this code will be removed.
> >>
> >>>
> >>>>>>+
> >>>>>>+    /*
> >>>>>>+     * 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));
> >>>
> >>>What does this mean? Same comment here and below ...
> >>
> >>If its the function 0, we return 0 that indicates no command is supported yet.
> >
> >first comment says no function supported.
> >clearly function 0 is supported, is it not?
> 
> Yep, the comment is not clear here. It should be "No function Supported other
> than function 0 "
> 
> Function 0 is the common function supported by all DSMs to inquire what functions are
> supported by this DSM.
> 
> >how exactly does 0 indicate no command is supported?
> >is it a bitmask of supported commands?
> 
> It is a bitmask. The spec said:
> 
> If Function Index is zero, the return is a buffer containing one bit for each function
> index, starting with zero.

Why not start from 1?
So 0x1 - function 1 supported, 0x2 - function 2, 0x4 - function 3 etc.

> Bit 0 indicates whether there is support for any functions other
> than function 0 for the specified UUID and Revision ID. If set to zero, no functions are
> supported (other than function zero) for the specified UUID and Revision ID.

> >
> >>Other wise, it is a command request from a evil guest regardless of the result
> >>returned by function 0, we return the status code 1 to indicates this command
> >>is not supported.
> >
> >is command same as function?
> 
> Yes.

Re: [Qemu-devel] [PATCH 8/9] nvdimm acpi: emulate dsm method

[Xiao Guangrong]

On 03/02/2016 04:44 PM, Michael S. Tsirkin wrote:
> On Wed, Mar 02, 2016 at 03:29:33PM +0800, Xiao Guangrong wrote:
>>
>>
>> On 03/02/2016 03:20 PM, Michael S. Tsirkin wrote:
>>> On Wed, Mar 02, 2016 at 03:15:19PM +0800, Xiao Guangrong wrote:
>>>>
>>>>
>>>> On 03/02/2016 02:36 PM, Michael S. Tsirkin wrote:
>>>>> On Wed, Mar 02, 2016 at 11:30:10AM +0800, Xiao Guangrong wrote:
>>>>>>
>>>>>>
>>>>>> On 03/02/2016 01:09 AM, Michael S. Tsirkin wrote:
>>>>>>
>>>>>>>
>>>>>>> Can't guest trigger this?
>>>>>>> If yes, don't put such code in production please:
>>>>>>> this will fill up disk on the host.
>>>>>>>
>>>>>>
>>>>>> Okay, the evil guest can read the IO port freely. I will use nvdimm_debug() instead.
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>   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);
>>>>>
>>>>> ugh. manual memory management :(
>>>>>
>>>>
>>>> Hmm... Or use GArray? But it is :)
>>>>
>>>>>>>> +    cpu_physical_memory_read(dsm_mem_addr, in, TARGET_PAGE_SIZE);
>>>>>
>>>>> is there a requirement address is aligned?
>>>>> if not this might cross page and crash qemu.
>>>>> better read just what you need.
>>>>>
>>>>
>>>> Yes, this memory is allocated by BIOS and we asked it to align the memory
>>>> with PAGE_SIZE:
>>>>
>>>>      bios_linker_loader_alloc(linker, NVDIMM_DSM_MEM_FILE, TARGET_PAGE_SIZE,
>>>>                               false /* high memory */);
>>>>
>>>>>>>> +
>>>>>>>> +    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);
>>>>>
>>>>> export build_alloc_array then, and reuse?
>>>>
>>>> It is good to me, but as your suggestions, this code will be removed.
>>>>
>>>>>
>>>>>>>> +
>>>>>>>> +    /*
>>>>>>>> +     * 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));
>>>>>
>>>>> What does this mean? Same comment here and below ...
>>>>
>>>> If its the function 0, we return 0 that indicates no command is supported yet.
>>>
>>> first comment says no function supported.
>>> clearly function 0 is supported, is it not?
>>
>> Yep, the comment is not clear here. It should be "No function Supported other
>> than function 0 "
>>
>> Function 0 is the common function supported by all DSMs to inquire what functions are
>> supported by this DSM.
>>
>>> how exactly does 0 indicate no command is supported?
>>> is it a bitmask of supported commands?
>>
>> It is a bitmask. The spec said:
>>
>> If Function Index is zero, the return is a buffer containing one bit for each function
>> index, starting with zero.
>
> Why not start from 1?
> So 0x1 - function 1 supported, 0x2 - function 2, 0x4 - function 3 etc.

It is defined by the spec in ACPI 6.0 "9.14.1 _DSM (Device Specific Method)" on page 532:

If set to zero, no functions are supported (other than function zero) for the specified UUID and
Revision ID. If set to one, at least one additional function is supported.

I audaciously guess the ACPI guys just want to reserve 0 to quickly identify if any function is
valid other than walking the bits one by one. :) but who knows...