QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/5] ARM virt: Add NVDIMM support
@ 2019-10-04 15:52 Shameer Kolothum
  2019-10-04 15:52 ` [PATCH 1/5] hw/arm: Align ACPI blob len to PAGE size Shameer Kolothum
                   ` (6 more replies)
  0 siblings, 7 replies; 15+ messages in thread
From: Shameer Kolothum @ 2019-10-04 15:52 UTC (permalink / raw)
  To: qemu-devel, qemu-arm, eric.auger, imammedo
  Cc: peter.maydell, lersek, linuxarm, xuwei5, shannon.zhaosl

This series adds NVDIMM support to arm/virt platform.
This has a dependency on [0] and make use of the GED
device for NVDIMM hotplug events. The series reuses
some of the patches posted by Eric in his earlier
attempt here[1].

Patch 1/5 is a fix to the Guest reboot issue on NVDIMM
hot add case described here[2].

I have done basic sanity testing of NVDIMM deviecs with
both ACPI and DT Guest boot. Further testing is always
welcome.

Please let me know your feedback.

Thanks,
Shameer

[0] https://patchwork.kernel.org/cover/11150345/
[1] https://patchwork.kernel.org/cover/10830777/
[2] https://patchwork.kernel.org/patch/11154757/

Eric Auger (1):
  hw/arm/boot: Expose the pmem nodes in the DT

Kwangwoo Lee (2):
  nvdimm: Use configurable ACPI IO base and size
  hw/arm/virt: Add nvdimm hot-plug infrastructure

Shameer Kolothum (2):
  hw/arm: Align ACPI blob len to PAGE size
  hw/arm/virt: Add nvdimm hotplug support

 docs/specs/acpi_hw_reduced_hotplug.rst |  1 +
 hw/acpi/generic_event_device.c         | 13 ++++++++
 hw/acpi/nvdimm.c                       | 32 ++++++++++++------
 hw/arm/Kconfig                         |  1 +
 hw/arm/boot.c                          | 45 ++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c               | 20 ++++++++++++
 hw/arm/virt.c                          | 42 ++++++++++++++++++++----
 hw/i386/acpi-build.c                   |  6 ++++
 hw/i386/acpi-build.h                   |  3 ++
 hw/i386/pc_piix.c                      |  2 ++
 hw/i386/pc_q35.c                       |  2 ++
 hw/mem/Kconfig                         |  2 +-
 include/hw/acpi/generic_event_device.h |  1 +
 include/hw/arm/virt.h                  |  1 +
 include/hw/mem/nvdimm.h                |  3 ++
 15 files changed, 157 insertions(+), 17 deletions(-)

-- 
2.17.1




^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/5] hw/arm: Align ACPI blob len to PAGE size
  2019-10-04 15:52 [PATCH 0/5] ARM virt: Add NVDIMM support Shameer Kolothum
@ 2019-10-04 15:52 ` Shameer Kolothum
  2019-11-08 16:17   ` Igor Mammedov
  2019-10-04 15:52 ` [PATCH 2/5] nvdimm: Use configurable ACPI IO base and size Shameer Kolothum
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 15+ messages in thread
From: Shameer Kolothum @ 2019-10-04 15:52 UTC (permalink / raw)
  To: qemu-devel, qemu-arm, eric.auger, imammedo
  Cc: peter.maydell, lersek, linuxarm, xuwei5, shannon.zhaosl

If ACPI blob length modifications happens after the initial
virt_acpi_build() call, and the changed blob length is within
the PAGE size boundary, then the revised size is not seen by
the firmware on Guest reboot. The is because in the
virt_acpi_build_update() -> acpi_ram_update() -> qemu_ram_resize()
path, qemu_ram_resize() uses ram_block size which is aligned
to PAGE size and the "resize callback" to update the size seen
by firmware is not getting invoked. Hence align ACPI blob sizes
to PAGE boundary.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
More details on this issue can be found here,
https://patchwork.kernel.org/patch/11154757/

---
 hw/arm/virt-acpi-build.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 4cd50175e0..074e0c858e 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -790,6 +790,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
     GArray *table_offsets;
     unsigned dsdt, xsdt;
     GArray *tables_blob = tables->table_data;
+    GArray *cmd_blob = tables->linker->cmd_blob;
     MachineState *ms = MACHINE(vms);
 
     table_offsets = g_array_new(false, true /* clear */,
@@ -854,6 +855,19 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
         build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
     }
 
+    /*
+     * Align the ACPI blob lengths to PAGE size so that on ACPI table
+     * regeneration, the length that firmware sees really gets updated
+     * through 'resize' callback in qemu_ram_resize() in the
+     * virt_acpi_build_update() -> acpi_ram_update() -> qemu_ram_resize()
+     * path.
+     */
+    g_array_set_size(tables_blob,
+                     TARGET_PAGE_ALIGN(acpi_data_len(tables_blob)));
+    g_array_set_size(tables->rsdp,
+                     TARGET_PAGE_ALIGN(acpi_data_len(tables->rsdp)));
+    g_array_set_size(cmd_blob,
+                     TARGET_PAGE_ALIGN(acpi_data_len(cmd_blob)));
     /* Cleanup memory that's no longer used. */
     g_array_free(table_offsets, true);
 }
-- 
2.17.1




^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 2/5] nvdimm: Use configurable ACPI IO base and size
  2019-10-04 15:52 [PATCH 0/5] ARM virt: Add NVDIMM support Shameer Kolothum
  2019-10-04 15:52 ` [PATCH 1/5] hw/arm: Align ACPI blob len to PAGE size Shameer Kolothum
@ 2019-10-04 15:52 ` Shameer Kolothum
  2019-11-11 14:24   ` Igor Mammedov
  2019-10-04 15:53 ` [PATCH 3/5] hw/arm/virt: Add nvdimm hot-plug infrastructure Shameer Kolothum
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 15+ messages in thread
From: Shameer Kolothum @ 2019-10-04 15:52 UTC (permalink / raw)
  To: qemu-devel, qemu-arm, eric.auger, imammedo
  Cc: peter.maydell, lersek, linuxarm, xuwei5, shannon.zhaosl

From: Kwangwoo Lee <kwangwoo.lee@sk.com>

This patch makes IO base and size configurable to create NPIO AML for
ACPI NFIT. Since a different architecture like AArch64 does not use
port-mapped IO, a configurable IO base is required to create correct
mapping of ACPI IO address and size.

Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com>
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/acpi/nvdimm.c        | 32 ++++++++++++++++++++++----------
 hw/i386/acpi-build.c    |  6 ++++++
 hw/i386/acpi-build.h    |  3 +++
 hw/i386/pc_piix.c       |  2 ++
 hw/i386/pc_q35.c        |  2 ++
 include/hw/mem/nvdimm.h |  3 +++
 6 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 9fdad6dc3f..f91eea3802 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -926,11 +926,13 @@ void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev)
 }
 
 void nvdimm_init_acpi_state(NVDIMMState *state, MemoryRegion *io,
+                            struct AcpiGenericAddress dsm_io,
                             FWCfgState *fw_cfg, Object *owner)
 {
+    state->dsm_io = dsm_io;
     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);
+                          "nvdimm-acpi-io", dsm_io.bit_width >> 3);
+    memory_region_add_subregion(io, dsm_io.address, &state->io_mr);
 
     state->dsm_mem = g_array_new(false, true /* clear */, 1);
     acpi_data_push(state->dsm_mem, sizeof(NvdimmDsmIn));
@@ -959,12 +961,14 @@ void nvdimm_init_acpi_state(NVDIMMState *state, MemoryRegion *io,
 
 #define NVDIMM_QEMU_RSVD_UUID   "648B9CF2-CDA1-4312-8AD9-49C4AF32BD62"
 
-static void nvdimm_build_common_dsm(Aml *dev)
+static void nvdimm_build_common_dsm(Aml *dev,
+                                    NVDIMMState *nvdimm_state)
 {
     Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *elsectx2;
     Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid;
     Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, *dsm_out_buf_size;
     uint8_t byte_list[1];
+    AmlRegionSpace rs;
 
     method = aml_method(NVDIMM_COMMON_DSM, 5, AML_SERIALIZED);
     uuid = aml_arg(0);
@@ -975,9 +979,16 @@ static void nvdimm_build_common_dsm(Aml *dev)
 
     aml_append(method, aml_store(aml_name(NVDIMM_ACPI_MEM_ADDR), dsm_mem));
 
+    if (nvdimm_state->dsm_io.space_id == AML_AS_SYSTEM_IO) {
+        rs = AML_SYSTEM_IO;
+    } else {
+        rs = AML_SYSTEM_MEMORY;
+    }
+
     /* map DSM memory and IO into ACPI namespace. */
-    aml_append(method, aml_operation_region(NVDIMM_DSM_IOPORT, AML_SYSTEM_IO,
-               aml_int(NVDIMM_ACPI_IO_BASE), NVDIMM_ACPI_IO_LEN));
+    aml_append(method, aml_operation_region(NVDIMM_DSM_IOPORT, rs,
+               aml_int(nvdimm_state->dsm_io.address),
+               nvdimm_state->dsm_io.bit_width >> 3));
     aml_append(method, aml_operation_region(NVDIMM_DSM_MEMORY,
                AML_SYSTEM_MEMORY, dsm_mem, sizeof(NvdimmDsmIn)));
 
@@ -992,7 +1003,7 @@ static void nvdimm_build_common_dsm(Aml *dev)
     field = aml_field(NVDIMM_DSM_IOPORT, AML_DWORD_ACC, AML_NOLOCK,
                       AML_PRESERVE);
     aml_append(field, aml_named_field(NVDIMM_DSM_NOTIFY,
-               NVDIMM_ACPI_IO_LEN * BITS_PER_BYTE));
+              (nvdimm_state->dsm_io.bit_width >> 3) * BITS_PER_BYTE));
     aml_append(method, field);
 
     /*
@@ -1260,7 +1271,8 @@ static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots)
 }
 
 static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
-                              BIOSLinker *linker, GArray *dsm_dma_area,
+                              BIOSLinker *linker,
+                              NVDIMMState *nvdimm_state,
                               uint32_t ram_slots)
 {
     Aml *ssdt, *sb_scope, *dev;
@@ -1288,7 +1300,7 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
      */
     aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012")));
 
-    nvdimm_build_common_dsm(dev);
+    nvdimm_build_common_dsm(dev, nvdimm_state);
 
     /* 0 is reserved for root device. */
     nvdimm_build_device_dsm(dev, 0);
@@ -1307,7 +1319,7 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
                                                NVDIMM_ACPI_MEM_ADDR);
 
     bios_linker_loader_alloc(linker,
-                             NVDIMM_DSM_MEM_FILE, dsm_dma_area,
+                             NVDIMM_DSM_MEM_FILE, nvdimm_state->dsm_mem,
                              sizeof(NvdimmDsmIn), false /* high memory */);
     bios_linker_loader_add_pointer(linker,
         ACPI_BUILD_TABLE_FILE, mem_addr_offset, sizeof(uint32_t),
@@ -1329,7 +1341,7 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
         return;
     }
 
-    nvdimm_build_ssdt(table_offsets, table_data, linker, state->dsm_mem,
+    nvdimm_build_ssdt(table_offsets, table_data, linker, state,
                       ram_slots);
 
     device_list = nvdimm_get_device_list();
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 1d077a7cb7..b5170912a8 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -126,6 +126,12 @@ typedef struct FwCfgTPMConfig {
 
 static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg);
 
+const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio = {
+    .space_id = AML_AS_SYSTEM_IO,
+    .address = NVDIMM_ACPI_IO_BASE,
+    .bit_width = NVDIMM_ACPI_IO_LEN << 3
+};
+
 static void init_common_fadt_data(MachineState *ms, Object *o,
                                   AcpiFadtData *data)
 {
diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
index 007332e51c..74df5fc612 100644
--- a/hw/i386/acpi-build.h
+++ b/hw/i386/acpi-build.h
@@ -1,6 +1,9 @@
 
 #ifndef HW_I386_ACPI_BUILD_H
 #define HW_I386_ACPI_BUILD_H
+#include "hw/acpi/acpi-defs.h"
+
+extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
 
 void acpi_setup(void);
 
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 6824b72124..78521cf017 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -58,6 +58,7 @@
 #include "migration/misc.h"
 #include "kvm_i386.h"
 #include "sysemu/numa.h"
+#include "hw/i386/acpi-build.h"
 
 #define MAX_IDE_BUS 2
 
@@ -303,6 +304,7 @@ else {
 
     if (machine->nvdimms_state->is_enabled) {
         nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
+                               x86_nvdimm_acpi_dsmio,
                                pcms->fw_cfg, OBJECT(pcms));
     }
 }
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 8fad20f314..d53ee8de84 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -53,6 +53,7 @@
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "sysemu/numa.h"
+#include "hw/i386/acpi-build.h"
 
 /* ICH9 AHCI has 6 ports */
 #define MAX_SATA_PORTS     6
@@ -330,6 +331,7 @@ static void pc_q35_init(MachineState *machine)
 
     if (machine->nvdimms_state->is_enabled) {
         nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
+                               x86_nvdimm_acpi_dsmio,
                                pcms->fw_cfg, OBJECT(pcms));
     }
 }
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index 523a9b3d4a..5fe440861e 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -25,6 +25,7 @@
 
 #include "hw/mem/pc-dimm.h"
 #include "hw/acpi/bios-linker-loader.h"
+#include "hw/acpi/aml-build.h"
 
 #define NVDIMM_DEBUG 0
 #define nvdimm_debug(fmt, ...)                                \
@@ -140,10 +141,12 @@ struct NVDIMMState {
      */
     int32_t persistence;
     char    *persistence_string;
+    struct AcpiGenericAddress dsm_io;
 };
 typedef struct NVDIMMState NVDIMMState;
 
 void nvdimm_init_acpi_state(NVDIMMState *state, MemoryRegion *io,
+                            struct AcpiGenericAddress dsm_io,
                             FWCfgState *fw_cfg, Object *owner);
 void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
                        BIOSLinker *linker, NVDIMMState *state,
-- 
2.17.1




^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 3/5] hw/arm/virt: Add nvdimm hot-plug infrastructure
  2019-10-04 15:52 [PATCH 0/5] ARM virt: Add NVDIMM support Shameer Kolothum
  2019-10-04 15:52 ` [PATCH 1/5] hw/arm: Align ACPI blob len to PAGE size Shameer Kolothum
  2019-10-04 15:52 ` [PATCH 2/5] nvdimm: Use configurable ACPI IO base and size Shameer Kolothum
@ 2019-10-04 15:53 ` Shameer Kolothum
  2019-11-11 14:38   ` Igor Mammedov
  2019-10-04 15:53 ` [PATCH 4/5] hw/arm/boot: Expose the pmem nodes in the DT Shameer Kolothum
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 15+ messages in thread
From: Shameer Kolothum @ 2019-10-04 15:53 UTC (permalink / raw)
  To: qemu-devel, qemu-arm, eric.auger, imammedo
  Cc: peter.maydell, lersek, linuxarm, xuwei5, shannon.zhaosl

From: Kwangwoo Lee <kwangwoo.lee@sk.com>

Pre-plug and plug handlers are prepared for NVDIMM support.
Please note nvdimm_support is not yet enabled.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/arm/Kconfig           |  1 +
 hw/arm/virt-acpi-build.c |  6 ++++++
 hw/arm/virt.c            | 22 +++++++++++++++++++++-
 hw/mem/Kconfig           |  2 +-
 include/hw/arm/virt.h    |  1 +
 5 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index c6e7782580..851dd81289 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -24,6 +24,7 @@ config ARM_VIRT
     select DIMM
     select ACPI_MEMORY_HOTPLUG
     select ACPI_HW_REDUCED
+    select ACPI_NVDIMM
 
 config CHEETAH
     bool
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 074e0c858e..4e63f5da48 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -44,6 +44,7 @@
 #include "hw/pci/pcie_host.h"
 #include "hw/pci/pci.h"
 #include "hw/arm/virt.h"
+#include "hw/mem/nvdimm.h"
 #include "sysemu/numa.h"
 #include "sysemu/reset.h"
 #include "kvm_arm.h"
@@ -835,6 +836,11 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
         }
     }
 
+    if (ms->nvdimms_state->is_enabled) {
+        nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
+                          ms->nvdimms_state, ms->ram_slots);
+    }
+
     if (its_class_name() && !vmc->no_its) {
         acpi_add_table(table_offsets, tables_blob);
         build_iort(tables_blob, tables->linker, vms);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index d4bedc2607..30bc8a7803 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -143,6 +143,7 @@ static const MemMapEntry base_memmap[] = {
     [VIRT_SMMU] =               { 0x09050000, 0x00020000 },
     [VIRT_PCDIMM_ACPI] =        { 0x09070000, MEMORY_HOTPLUG_IO_LEN },
     [VIRT_ACPI_GED] =           { 0x09080000, ACPI_GED_EVT_SEL_LEN },
+    [VIRT_NVDIMM_ACPI] =        { 0x09090000, NVDIMM_ACPI_IO_LEN},
     [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
     /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
     [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
@@ -1750,6 +1751,18 @@ static void machvirt_init(MachineState *machine)
 
     create_platform_bus(vms, pic);
 
+    if (machine->nvdimms_state->is_enabled) {
+        const struct AcpiGenericAddress arm_virt_nvdimm_acpi_dsmio = {
+            .space_id = AML_AS_SYSTEM_MEMORY,
+            .address = vms->memmap[VIRT_NVDIMM_ACPI].base,
+            .bit_width = NVDIMM_ACPI_IO_LEN << 3
+        };
+
+        nvdimm_init_acpi_state(machine->nvdimms_state, sysmem,
+                               arm_virt_nvdimm_acpi_dsmio,
+                               vms->fw_cfg, OBJECT(vms));
+    }
+
     vms->bootinfo.ram_size = machine->ram_size;
     vms->bootinfo.nb_cpus = smp_cpus;
     vms->bootinfo.board_id = -1;
@@ -1916,9 +1929,10 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                                  Error **errp)
 {
     VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
+    MachineState *ms = MACHINE(hotplug_dev);
     const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
 
-    if (is_nvdimm) {
+    if (is_nvdimm && (!ms->nvdimms_state->is_enabled)) {
         error_setg(errp, "nvdimm is not yet supported");
         return;
     }
@@ -1937,6 +1951,8 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
 {
     HotplugHandlerClass *hhc;
     VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
+    MachineState *ms = MACHINE(hotplug_dev);
+    bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
     Error *local_err = NULL;
 
     pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), &local_err);
@@ -1944,6 +1960,10 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
         goto out;
     }
 
+    if (is_nvdimm) {
+        nvdimm_plug(ms->nvdimms_state);
+    }
+
     hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
     hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, &error_abort);
 out:
diff --git a/hw/mem/Kconfig b/hw/mem/Kconfig
index 620fd4cb59..0d5f8f321a 100644
--- a/hw/mem/Kconfig
+++ b/hw/mem/Kconfig
@@ -8,4 +8,4 @@ config MEM_DEVICE
 config NVDIMM
     bool
     default y
-    depends on PC
+    depends on PC || ARM_VIRT
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 0b41083e9d..06d5e75611 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -79,6 +79,7 @@ enum {
     VIRT_SECURE_MEM,
     VIRT_PCDIMM_ACPI,
     VIRT_ACPI_GED,
+    VIRT_NVDIMM_ACPI,
     VIRT_LOWMEMMAP_LAST,
 };
 
-- 
2.17.1




^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 4/5] hw/arm/boot: Expose the pmem nodes in the DT
  2019-10-04 15:52 [PATCH 0/5] ARM virt: Add NVDIMM support Shameer Kolothum
                   ` (2 preceding siblings ...)
  2019-10-04 15:53 ` [PATCH 3/5] hw/arm/virt: Add nvdimm hot-plug infrastructure Shameer Kolothum
@ 2019-10-04 15:53 ` Shameer Kolothum
  2019-11-11 14:46   ` Igor Mammedov
  2019-10-04 15:53 ` [PATCH 5/5] hw/arm/virt: Add nvdimm hotplug support Shameer Kolothum
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 15+ messages in thread
From: Shameer Kolothum @ 2019-10-04 15:53 UTC (permalink / raw)
  To: qemu-devel, qemu-arm, eric.auger, imammedo
  Cc: peter.maydell, lersek, linuxarm, xuwei5, shannon.zhaosl

From: Eric Auger <eric.auger@redhat.com>

In case of NV-DIMM slots, let's add /pmem DT nodes.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/arm/boot.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index c264864c11..bd6d72b33e 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -20,6 +20,7 @@
 #include "hw/boards.h"
 #include "sysemu/reset.h"
 #include "hw/loader.h"
+#include "hw/mem/memory-device.h"
 #include "elf.h"
 #include "sysemu/device_tree.h"
 #include "qemu/config-file.h"
@@ -523,6 +524,44 @@ static void fdt_add_psci_node(void *fdt)
     qemu_fdt_setprop_cell(fdt, "/psci", "migrate", migrate_fn);
 }
 
+static int fdt_add_pmem_node(void *fdt, uint32_t acells, uint32_t scells)
+{
+    MemoryDeviceInfoList *info, *info_list = qmp_memory_device_list();
+    MemoryDeviceInfo *mi;
+    int ret;
+
+    for (info = info_list; info != NULL; info = info->next) {
+        mi = info->value;
+
+        if (mi->type == MEMORY_DEVICE_INFO_KIND_NVDIMM) {
+            PCDIMMDeviceInfo *di = mi->u.nvdimm.data;
+            char *nodename;
+
+            nodename = g_strdup_printf("/pmem@%" PRIx64, di->addr);
+            qemu_fdt_add_subnode(fdt, nodename);
+            qemu_fdt_setprop_string(fdt, nodename, "compatible", "pmem-region");
+            ret = qemu_fdt_setprop_sized_cells(fdt, nodename, "reg", acells,
+                                               di->addr, scells, di->size);
+            /* only set the NUMA ID if it is specified */
+            if (!ret && di->node >= 0) {
+                ret = qemu_fdt_setprop_cell(fdt, nodename, "numa-node-id",
+                                            di->node);
+            }
+
+            g_free(nodename);
+
+            if (ret < 0) {
+                fprintf(stderr, "couldn't add NVDIMM /memory@%"PRIx64" node\n",
+                        di->addr);
+                goto out;
+            }
+        }
+    }
+out:
+    qapi_free_MemoryDeviceInfoList(info_list);
+    return ret;
+}
+
 int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
                  hwaddr addr_limit, AddressSpace *as, MachineState *ms)
 {
@@ -622,6 +661,12 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
         }
     }
 
+    rc = fdt_add_pmem_node(fdt, acells, scells);
+    if (rc < 0) {
+        fprintf(stderr, "couldn't add pmem memory nodes\n");
+        goto fail;
+    }
+
     rc = fdt_path_offset(fdt, "/chosen");
     if (rc < 0) {
         qemu_fdt_add_subnode(fdt, "/chosen");
-- 
2.17.1




^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 5/5] hw/arm/virt: Add nvdimm hotplug support
  2019-10-04 15:52 [PATCH 0/5] ARM virt: Add NVDIMM support Shameer Kolothum
                   ` (3 preceding siblings ...)
  2019-10-04 15:53 ` [PATCH 4/5] hw/arm/boot: Expose the pmem nodes in the DT Shameer Kolothum
@ 2019-10-04 15:53 ` Shameer Kolothum
  2019-11-12 13:01   ` Igor Mammedov
  2019-10-18 16:39 ` [PATCH 0/5] ARM virt: Add NVDIMM support Auger Eric
  2019-11-12 14:39 ` Igor Mammedov
  6 siblings, 1 reply; 15+ messages in thread
From: Shameer Kolothum @ 2019-10-04 15:53 UTC (permalink / raw)
  To: qemu-devel, qemu-arm, eric.auger, imammedo
  Cc: peter.maydell, lersek, linuxarm, xuwei5, shannon.zhaosl

This adds support for nvdimm hotplug events through GED
and enables nvdimm for the arm/virt. Now Guests with DT boot
can have nvdimm cold plug and with ACPI both cold/hot plug.

Hot removal functionality is not yet supported.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 docs/specs/acpi_hw_reduced_hotplug.rst |  1 +
 hw/acpi/generic_event_device.c         | 13 +++++++++++++
 hw/arm/virt.c                          | 20 +++++++++++++++-----
 include/hw/acpi/generic_event_device.h |  1 +
 4 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/docs/specs/acpi_hw_reduced_hotplug.rst b/docs/specs/acpi_hw_reduced_hotplug.rst
index 911a98255b..e3abe975bf 100644
--- a/docs/specs/acpi_hw_reduced_hotplug.rst
+++ b/docs/specs/acpi_hw_reduced_hotplug.rst
@@ -63,6 +63,7 @@ GED IO interface (4 byte access)
     bits:
        0: Memory hotplug event
        1: System power down event
+       2: NVDIMM hotplug event
     2-31: Reserved
 
 **write_access:**
diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index 9cee90cc70..ad1b684304 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -16,6 +16,7 @@
 #include "hw/acpi/generic_event_device.h"
 #include "hw/irq.h"
 #include "hw/mem/pc-dimm.h"
+#include "hw/mem/nvdimm.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
 #include "qemu/error-report.h"
@@ -23,6 +24,7 @@
 static const uint32_t ged_supported_events[] = {
     ACPI_GED_MEM_HOTPLUG_EVT,
     ACPI_GED_PWR_DOWN_EVT,
+    ACPI_GED_NVDIMM_HOTPLUG_EVT,
 };
 
 /*
@@ -110,6 +112,11 @@ void build_ged_aml(Aml *table, const char *name, HotplugHandler *hotplug_dev,
                            aml_notify(aml_name(ACPI_POWER_BUTTON_DEVICE),
                                       aml_int(0x80)));
                 break;
+            case ACPI_GED_NVDIMM_HOTPLUG_EVT:
+                aml_append(if_ctx,
+                           aml_notify(aml_name("\\_SB.NVDR"),
+                                      aml_int(0x80)));
+                break;
             default:
                 /*
                  * Please make sure all the events in ged_supported_events[]
@@ -175,7 +182,11 @@ static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,
     AcpiGedState *s = ACPI_GED(hotplug_dev);
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
+            nvdimm_acpi_plug_cb(hotplug_dev, dev);
+        } else {
             acpi_memory_plug_cb(hotplug_dev, &s->memhp_state, dev, errp);
+        }
     } else {
         error_setg(errp, "virt: device plug request for unsupported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
@@ -192,6 +203,8 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
         sel = ACPI_GED_MEM_HOTPLUG_EVT;
     } else if (ev & ACPI_POWER_DOWN_STATUS) {
         sel = ACPI_GED_PWR_DOWN_EVT;
+    } else if (ev & ACPI_NVDIMM_HOTPLUG_STATUS) {
+        sel = ACPI_GED_NVDIMM_HOTPLUG_EVT;
     } else {
         /* Unknown event. Return without generating interrupt. */
         warn_report("GED: Unsupported event %d. No irq injected", ev);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 30bc8a7803..acdcba9638 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -543,6 +543,10 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms, qemu_irq *pic)
         event |= ACPI_GED_MEM_HOTPLUG_EVT;
     }
 
+    if (ms->nvdimms_state->is_enabled) {
+        event |= ACPI_GED_NVDIMM_HOTPLUG_EVT;
+    }
+
     dev = qdev_create(NULL, TYPE_ACPI_GED);
     qdev_prop_set_uint32(dev, "ged-event", event);
 
@@ -1938,9 +1942,12 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     }
 
     if (!vms->acpi_dev) {
-        error_setg(errp,
-                   "memory hotplug is not enabled: missing acpi-ged device");
-        return;
+        /* Allow nvdimm DT or cold plug */
+        if (!(is_nvdimm && !dev->hotplugged)) {
+            error_setg(errp,
+                       "memory hotplug is not enabled: missing acpi-ged device");
+            return;
+         }
     }
 
     pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL, errp);
@@ -1964,8 +1971,10 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
         nvdimm_plug(ms->nvdimms_state);
     }
 
-    hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
-    hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, &error_abort);
+    if (vms->acpi_dev) {
+        hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
+        hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, &error_abort);
+    }
 out:
     error_propagate(errp, local_err);
 }
@@ -2073,6 +2082,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
     hc->plug = virt_machine_device_plug_cb;
     hc->unplug_request = virt_machine_device_unplug_request_cb;
     mc->numa_mem_supported = true;
+    mc->nvdimm_supported = true;
     mc->auto_enable_numa_with_memhp = true;
 }
 
diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
index d157eac088..9eb86ca4fd 100644
--- a/include/hw/acpi/generic_event_device.h
+++ b/include/hw/acpi/generic_event_device.h
@@ -82,6 +82,7 @@
  */
 #define ACPI_GED_MEM_HOTPLUG_EVT   0x1
 #define ACPI_GED_PWR_DOWN_EVT      0x2
+#define ACPI_GED_NVDIMM_HOTPLUG_EVT 0x4
 
 typedef struct GEDState {
     MemoryRegion io;
-- 
2.17.1




^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/5] ARM virt: Add NVDIMM support
  2019-10-04 15:52 [PATCH 0/5] ARM virt: Add NVDIMM support Shameer Kolothum
                   ` (4 preceding siblings ...)
  2019-10-04 15:53 ` [PATCH 5/5] hw/arm/virt: Add nvdimm hotplug support Shameer Kolothum
@ 2019-10-18 16:39 ` Auger Eric
  2019-10-22 14:05   ` Shameerali Kolothum Thodi
  2019-11-12 14:39 ` Igor Mammedov
  6 siblings, 1 reply; 15+ messages in thread
From: Auger Eric @ 2019-10-18 16:39 UTC (permalink / raw)
  To: Shameer Kolothum, qemu-devel, qemu-arm, imammedo
  Cc: peter.maydell, lersek, linuxarm, xuwei5, shannon.zhaosl

Hi Shameer,

On 10/4/19 5:52 PM, Shameer Kolothum wrote:
> This series adds NVDIMM support to arm/virt platform.
> This has a dependency on [0] and make use of the GED
> device for NVDIMM hotplug events. The series reuses
> some of the patches posted by Eric in his earlier
> attempt here[1].
> 
> Patch 1/5 is a fix to the Guest reboot issue on NVDIMM
> hot add case described here[2].
> 
> I have done basic sanity testing of NVDIMM deviecs with
devcies
> both ACPI and DT Guest boot. Further testing is always
> welcome.
> 
> Please let me know your feedback.

I tested it on my side. Looks to work pretty well.

one question: I noticed that when a NVDIMM slot is hotplugged one get
the following trace on guest:

nfit ACPI0012:00: found a zero length table '0' parsing nfit
pmem0: detected capacity change from 0 to 1073741824

Have you experienced the 0 length trace?

Besides when we reset the system we find the namespaces again using
"ndctl list -u" so the original bug seems to be fixed.

Did you try to mount a DAX FS. I can mount but with DAX forced off.

sudo mkdir /mnt/mem0
mkfs.xfs -f -m reflink=0 /dev/pmem0
sudo mount -o dax /dev/pmem0 /mnt/mem0
[ 2610.051830] XFS (pmem0): DAX enabled. Warning: EXPERIMENTAL, use at
your own risk
[ 2610.178580] XFS (pmem0): DAX unsupported by block device. Turning off
DAX.
[ 2610.180871] XFS (pmem0): Mounting V5 Filesystem
[ 2610.189797] XFS (pmem0): Ending clean mount

I fail to remember if it was the case months ago. I am not sure if it is
an issue in my guest .config or if there is something not yet supported
on aarch64? Did you try on your side?

Also if you forget to put the ",nvdimm" to the machvirt options you get,
on hotplug:
{"error": {"class": "GenericError", "desc": "nvdimm is not yet supported"}}
which is not correct anymore ;-)

Thanks

Eric


> 
> Thanks,
> Shameer
> 
> [0] https://patchwork.kernel.org/cover/11150345/
> [1] https://patchwork.kernel.org/cover/10830777/
> [2] https://patchwork.kernel.org/patch/11154757/
> 
> Eric Auger (1):
>   hw/arm/boot: Expose the pmem nodes in the DT
> 
> Kwangwoo Lee (2):
>   nvdimm: Use configurable ACPI IO base and size
>   hw/arm/virt: Add nvdimm hot-plug infrastructure
> 
> Shameer Kolothum (2):
>   hw/arm: Align ACPI blob len to PAGE size
>   hw/arm/virt: Add nvdimm hotplug support
> 
>  docs/specs/acpi_hw_reduced_hotplug.rst |  1 +
>  hw/acpi/generic_event_device.c         | 13 ++++++++
>  hw/acpi/nvdimm.c                       | 32 ++++++++++++------
>  hw/arm/Kconfig                         |  1 +
>  hw/arm/boot.c                          | 45 ++++++++++++++++++++++++++
>  hw/arm/virt-acpi-build.c               | 20 ++++++++++++
>  hw/arm/virt.c                          | 42 ++++++++++++++++++++----
>  hw/i386/acpi-build.c                   |  6 ++++
>  hw/i386/acpi-build.h                   |  3 ++
>  hw/i386/pc_piix.c                      |  2 ++
>  hw/i386/pc_q35.c                       |  2 ++
>  hw/mem/Kconfig                         |  2 +-
>  include/hw/acpi/generic_event_device.h |  1 +
>  include/hw/arm/virt.h                  |  1 +
>  include/hw/mem/nvdimm.h                |  3 ++
>  15 files changed, 157 insertions(+), 17 deletions(-)
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH 0/5] ARM virt: Add NVDIMM support
  2019-10-18 16:39 ` [PATCH 0/5] ARM virt: Add NVDIMM support Auger Eric
@ 2019-10-22 14:05   ` Shameerali Kolothum Thodi
  0 siblings, 0 replies; 15+ messages in thread
From: Shameerali Kolothum Thodi @ 2019-10-22 14:05 UTC (permalink / raw)
  To: Auger Eric, qemu-devel, qemu-arm, imammedo
  Cc: peter.maydell, lersek, Linuxarm, xuwei \(O\), shannon.zhaosl

Hi Eric,

> -----Original Message-----
> From: Auger Eric [mailto:eric.auger@redhat.com]
> Sent: 18 October 2019 17:40
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> qemu-devel@nongnu.org; qemu-arm@nongnu.org; imammedo@redhat.com
> Cc: peter.maydell@linaro.org; shannon.zhaosl@gmail.com; xuwei (O)
> <xuwei5@huawei.com>; lersek@redhat.com; Linuxarm
> <linuxarm@huawei.com>
> Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support
> 
> Hi Shameer,
> 
> On 10/4/19 5:52 PM, Shameer Kolothum wrote:
> > This series adds NVDIMM support to arm/virt platform.
> > This has a dependency on [0] and make use of the GED
> > device for NVDIMM hotplug events. The series reuses
> > some of the patches posted by Eric in his earlier
> > attempt here[1].
> >
> > Patch 1/5 is a fix to the Guest reboot issue on NVDIMM
> > hot add case described here[2].
> >
> > I have done basic sanity testing of NVDIMM deviecs with
> devcies
> > both ACPI and DT Guest boot. Further testing is always
> > welcome.
> >
> > Please let me know your feedback.
> 
> I tested it on my side. Looks to work pretty well.

Thanks for giving this a spin.
 
> one question: I noticed that when a NVDIMM slot is hotplugged one get
> the following trace on guest:
> 
> nfit ACPI0012:00: found a zero length table '0' parsing nfit
> pmem0: detected capacity change from 0 to 1073741824
> 
> Have you experienced the 0 length trace?

I double checked and yes that trace is there. And I did a quick check with
x86 and it is not there. 

The reason looks like, ARM64 kernel receives an additional 8 bytes size when
the kernel evaluates the "_FIT" object. 

For the same test scenario, Qemu reports a FIT buffer size of 0xb8 and 

X86 Guest kernel,
[    1.601077] acpi_nfit_init: data 0xffff8a273dc12b18 sz 0xb8

ARM64 Guest,
[    0.933133] acpi_nfit_init: data 0xffff00003cbe6018 sz 0xc0

I am not sure how that size gets changed for ARM which results in
the above mentioned 0 length trace. I need to debug this further.

Please let me know if you have any pointers...
 
> Besides when we reset the system we find the namespaces again using
> "ndctl list -u" so the original bug seems to be fixed.
> 
> Did you try to mount a DAX FS. I can mount but with DAX forced off.
> sudo mkdir /mnt/mem0

Yes. I did try with DAX FS. But do we need to change the namespace mode to 
file system DAX mode?

I used the below command before attempting to mount with -o dax,

ndctl create-namespace -f -e namespace0.0 --mode=fsdax

And in order to do the above you might need the ZONE_DEVICE support
in the Kernel which in turn has dependency on hot remove. Hence I tried with
"arm64/mm: Enable memory hot remove" patches,

https://patchwork.kernel.org/cover/11185169/

> mkfs.xfs -f -m reflink=0 /dev/pmem0
> sudo mount -o dax /dev/pmem0 /mnt/mem0
> [ 2610.051830] XFS (pmem0): DAX enabled. Warning: EXPERIMENTAL, use at
> your own risk
> [ 2610.178580] XFS (pmem0): DAX unsupported by block device. Turning off
> DAX.
> [ 2610.180871] XFS (pmem0): Mounting V5 Filesystem
> [ 2610.189797] XFS (pmem0): Ending clean mount
> 
> I fail to remember if it was the case months ago. I am not sure if it is
> an issue in my guest .config or if there is something not yet supported
> on aarch64? Did you try on your side?
> 
> Also if you forget to put the ",nvdimm" to the machvirt options you get,
> on hotplug:
> {"error": {"class": "GenericError", "desc": "nvdimm is not yet supported"}}
> which is not correct anymore ;-)

Ok. I will check this.

Thanks,
Shameer
 
> Thanks
> 
> Eric
> 
> 
> >
> > Thanks,
> > Shameer
> >
> > [0] https://patchwork.kernel.org/cover/11150345/
> > [1] https://patchwork.kernel.org/cover/10830777/
> > [2] https://patchwork.kernel.org/patch/11154757/
> >
> > Eric Auger (1):
> >   hw/arm/boot: Expose the pmem nodes in the DT
> >
> > Kwangwoo Lee (2):
> >   nvdimm: Use configurable ACPI IO base and size
> >   hw/arm/virt: Add nvdimm hot-plug infrastructure
> >
> > Shameer Kolothum (2):
> >   hw/arm: Align ACPI blob len to PAGE size
> >   hw/arm/virt: Add nvdimm hotplug support
> >
> >  docs/specs/acpi_hw_reduced_hotplug.rst |  1 +
> >  hw/acpi/generic_event_device.c         | 13 ++++++++
> >  hw/acpi/nvdimm.c                       | 32 ++++++++++++------
> >  hw/arm/Kconfig                         |  1 +
> >  hw/arm/boot.c                          | 45
> ++++++++++++++++++++++++++
> >  hw/arm/virt-acpi-build.c               | 20 ++++++++++++
> >  hw/arm/virt.c                          | 42
> ++++++++++++++++++++----
> >  hw/i386/acpi-build.c                   |  6 ++++
> >  hw/i386/acpi-build.h                   |  3 ++
> >  hw/i386/pc_piix.c                      |  2 ++
> >  hw/i386/pc_q35.c                       |  2 ++
> >  hw/mem/Kconfig                         |  2 +-
> >  include/hw/acpi/generic_event_device.h |  1 +
> >  include/hw/arm/virt.h                  |  1 +
> >  include/hw/mem/nvdimm.h                |  3 ++
> >  15 files changed, 157 insertions(+), 17 deletions(-)
> >

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/5] hw/arm: Align ACPI blob len to PAGE size
  2019-10-04 15:52 ` [PATCH 1/5] hw/arm: Align ACPI blob len to PAGE size Shameer Kolothum
@ 2019-11-08 16:17   ` Igor Mammedov
  2019-11-11 12:47     ` Shameerali Kolothum Thodi
  0 siblings, 1 reply; 15+ messages in thread
From: Igor Mammedov @ 2019-11-08 16:17 UTC (permalink / raw)
  To: Shameer Kolothum
  Cc: peter.maydell, shannon.zhaosl, Michael S. Tsirkin, qemu-devel,
	xuwei5, linuxarm, eric.auger, qemu-arm, lersek

On Fri, 4 Oct 2019 16:52:58 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> If ACPI blob length modifications happens after the initial
> virt_acpi_build() call, and the changed blob length is within
> the PAGE size boundary, then the revised size is not seen by
> the firmware on Guest reboot. The is because in the
> virt_acpi_build_update() -> acpi_ram_update() -> qemu_ram_resize()
> path, qemu_ram_resize() uses ram_block size which is aligned
> to PAGE size and the "resize callback" to update the size seen
> by firmware is not getting invoked. Hence align ACPI blob sizes
> to PAGE boundary.
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> More details on this issue can be found here,
> https://patchwork.kernel.org/patch/11154757/
re-read it again and it seems to me that this patch is workaround
rather than a solution to the problem.
CCing Michael as an author this code.

on x86 we have crazy history of manually aligning acpi blobs, see code under comment

  /* We'll expose it all to Guest so we want to reduce

so used_length endups with over-sized value which includes table and padding
and it happens that ACPI_BUILD_TABLE_SIZE is much bigger than host page size
so if on reboot we happen to exceed ACPI_BUILD_TABLE_SIZE, the next padded table
size (used_length) would be  2 x ACPI_BUILD_TABLE_SIZE which doesn't trigger
  block->used_length == HOST_PAGE_ALIGN(newsize)
condition so fwcfg gets updated value.


> ---
>  hw/arm/virt-acpi-build.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 4cd50175e0..074e0c858e 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -790,6 +790,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>      GArray *table_offsets;
>      unsigned dsdt, xsdt;
>      GArray *tables_blob = tables->table_data;
> +    GArray *cmd_blob = tables->linker->cmd_blob;
>      MachineState *ms = MACHINE(vms);
>  
>      table_offsets = g_array_new(false, true /* clear */,
> @@ -854,6 +855,19 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>          build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
>      }
>  
> +    /*
> +     * Align the ACPI blob lengths to PAGE size so that on ACPI table
> +     * regeneration, the length that firmware sees really gets updated
> +     * through 'resize' callback in qemu_ram_resize() in the
> +     * virt_acpi_build_update() -> acpi_ram_update() -> qemu_ram_resize()
> +     * path.
> +     */
> +    g_array_set_size(tables_blob,
> +                     TARGET_PAGE_ALIGN(acpi_data_len(tables_blob)));
here it would depend on TARGET_PAGE_ALIGN vs HOST_PAGE_ALIGN relation
so depending on host it could flip it's behavior to opposite.

one thing we could do is dropping (block->used_length == newsize) condition
another is to use value of block->used_length for s->files->f[index].size.

Michael,
what's your take in this?

> +    g_array_set_size(tables->rsdp,
> +                     TARGET_PAGE_ALIGN(acpi_data_len(tables->rsdp)));
> +    g_array_set_size(cmd_blob,
> +                     TARGET_PAGE_ALIGN(acpi_data_len(cmd_blob)));
>      /* Cleanup memory that's no longer used. */
>      g_array_free(table_offsets, true);
>  }



^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH 1/5] hw/arm: Align ACPI blob len to PAGE size
  2019-11-08 16:17   ` Igor Mammedov
@ 2019-11-11 12:47     ` Shameerali Kolothum Thodi
  0 siblings, 0 replies; 15+ messages in thread
From: Shameerali Kolothum Thodi @ 2019-11-11 12:47 UTC (permalink / raw)
  To: Igor Mammedov
  Cc: peter.maydell, shannon.zhaosl, Michael S. Tsirkin, qemu-devel,
	Linuxarm, eric.auger, qemu-arm, xuwei \(O\),
	lersek

Hi Igor,

> -----Original Message-----
> From: Igor Mammedov [mailto:imammedo@redhat.com]
> Sent: 08 November 2019 16:18
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> eric.auger@redhat.com; peter.maydell@linaro.org;
> shannon.zhaosl@gmail.com; xuwei (O) <xuwei5@huawei.com>;
> lersek@redhat.com; Linuxarm <linuxarm@huawei.com>; Michael S. Tsirkin
> <mst@redhat.com>
> Subject: Re: [PATCH 1/5] hw/arm: Align ACPI blob len to PAGE size
> 
> On Fri, 4 Oct 2019 16:52:58 +0100
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> 
> > If ACPI blob length modifications happens after the initial
> > virt_acpi_build() call, and the changed blob length is within
> > the PAGE size boundary, then the revised size is not seen by
> > the firmware on Guest reboot. The is because in the
> > virt_acpi_build_update() -> acpi_ram_update() -> qemu_ram_resize()
> > path, qemu_ram_resize() uses ram_block size which is aligned
> > to PAGE size and the "resize callback" to update the size seen
> > by firmware is not getting invoked. Hence align ACPI blob sizes
> > to PAGE boundary.
> >
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > ---
> > More details on this issue can be found here,
> > https://patchwork.kernel.org/patch/11154757/
> re-read it again and it seems to me that this patch is workaround
> rather than a solution to the problem.

Thanks for taking a look at this. Yes, I was also not very sure about this approach
as the root cause of the issue is in qemu_ram_resize().

> CCing Michael as an author this code.
> on x86 we have crazy history of manually aligning acpi blobs, see code under
> comment
> 
>   /* We'll expose it all to Guest so we want to reduce
> 
> so used_length endups with over-sized value which includes table and padding
> and it happens that ACPI_BUILD_TABLE_SIZE is much bigger than host page
> size
> so if on reboot we happen to exceed ACPI_BUILD_TABLE_SIZE, the next padded
> table
> size (used_length) would be  2 x ACPI_BUILD_TABLE_SIZE which doesn't
> trigger
>   block->used_length == HOST_PAGE_ALIGN(newsize)
> condition so fwcfg gets updated value.

Yes, this is the reason why the issue is not visible on x86.
 
> 
> > ---
> >  hw/arm/virt-acpi-build.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 4cd50175e0..074e0c858e 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -790,6 +790,7 @@ void virt_acpi_build(VirtMachineState *vms,
> AcpiBuildTables *tables)
> >      GArray *table_offsets;
> >      unsigned dsdt, xsdt;
> >      GArray *tables_blob = tables->table_data;
> > +    GArray *cmd_blob = tables->linker->cmd_blob;
> >      MachineState *ms = MACHINE(vms);
> >
> >      table_offsets = g_array_new(false, true /* clear */,
> > @@ -854,6 +855,19 @@ void virt_acpi_build(VirtMachineState *vms,
> AcpiBuildTables *tables)
> >          build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
> >      }
> >
> > +    /*
> > +     * Align the ACPI blob lengths to PAGE size so that on ACPI table
> > +     * regeneration, the length that firmware sees really gets updated
> > +     * through 'resize' callback in qemu_ram_resize() in the
> > +     * virt_acpi_build_update() -> acpi_ram_update() ->
> qemu_ram_resize()
> > +     * path.
> > +     */
> > +    g_array_set_size(tables_blob,
> > +
> TARGET_PAGE_ALIGN(acpi_data_len(tables_blob)));
> here it would depend on TARGET_PAGE_ALIGN vs HOST_PAGE_ALIGN relation
> so depending on host it could flip it's behavior to opposite.

Ok.

> 
> one thing we could do is dropping (block->used_length == newsize) condition

I tried this before and strangely for some reason on reboot path,

virt_acpi_build_update() is called with build_state being NULL and no acpi_ram_update()
happens. Not sure what causes this behavior when we drop the above condition.

> another is to use value of block->used_length for s->files->f[index].size.

I just tried this by passing block->used_length to fw_cfg_add_file_callback() .
This could work for this case. But not sure there will be any corner cases
and also there isn't any easy way to access the mr->ram_balck->used_length from
hw/core/loader.c.

> 
> Michael,
> what's your take in this?

Thanks,
Shameer

> 
> > +    g_array_set_size(tables->rsdp,
> > +
> TARGET_PAGE_ALIGN(acpi_data_len(tables->rsdp)));
> > +    g_array_set_size(cmd_blob,
> > +                     TARGET_PAGE_ALIGN(acpi_data_len(cmd_blob)));
> >      /* Cleanup memory that's no longer used. */
> >      g_array_free(table_offsets, true);
> >  }



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/5] nvdimm: Use configurable ACPI IO base and size
  2019-10-04 15:52 ` [PATCH 2/5] nvdimm: Use configurable ACPI IO base and size Shameer Kolothum
@ 2019-11-11 14:24   ` Igor Mammedov
  0 siblings, 0 replies; 15+ messages in thread
From: Igor Mammedov @ 2019-11-11 14:24 UTC (permalink / raw)
  To: Shameer Kolothum
  Cc: peter.maydell, shannon.zhaosl, qemu-devel, xuwei5, linuxarm,
	eric.auger, qemu-arm, lersek

On Fri, 4 Oct 2019 16:52:59 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> From: Kwangwoo Lee <kwangwoo.lee@sk.com>
> 
> This patch makes IO base and size configurable to create NPIO AML for
> ACPI NFIT. Since a different architecture like AArch64 does not use
> port-mapped IO, a configurable IO base is required to create correct
> mapping of ACPI IO address and size.
> 
> Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  hw/acpi/nvdimm.c        | 32 ++++++++++++++++++++++----------
>  hw/i386/acpi-build.c    |  6 ++++++
>  hw/i386/acpi-build.h    |  3 +++
>  hw/i386/pc_piix.c       |  2 ++
>  hw/i386/pc_q35.c        |  2 ++
>  include/hw/mem/nvdimm.h |  3 +++
>  6 files changed, 38 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 9fdad6dc3f..f91eea3802 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -926,11 +926,13 @@ void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev)
>  }
>  
>  void nvdimm_init_acpi_state(NVDIMMState *state, MemoryRegion *io,
> +                            struct AcpiGenericAddress dsm_io,
>                              FWCfgState *fw_cfg, Object *owner)
>  {
> +    state->dsm_io = dsm_io;
>      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);
> +                          "nvdimm-acpi-io", dsm_io.bit_width >> 3);
> +    memory_region_add_subregion(io, dsm_io.address, &state->io_mr);
>  
>      state->dsm_mem = g_array_new(false, true /* clear */, 1);
>      acpi_data_push(state->dsm_mem, sizeof(NvdimmDsmIn));
> @@ -959,12 +961,14 @@ void nvdimm_init_acpi_state(NVDIMMState *state, MemoryRegion *io,
>  
>  #define NVDIMM_QEMU_RSVD_UUID   "648B9CF2-CDA1-4312-8AD9-49C4AF32BD62"
>  
> -static void nvdimm_build_common_dsm(Aml *dev)
> +static void nvdimm_build_common_dsm(Aml *dev,
> +                                    NVDIMMState *nvdimm_state)
>  {
>      Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *elsectx2;
>      Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid;
>      Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, *dsm_out_buf_size;
>      uint8_t byte_list[1];
> +    AmlRegionSpace rs;
>  
>      method = aml_method(NVDIMM_COMMON_DSM, 5, AML_SERIALIZED);
>      uuid = aml_arg(0);
> @@ -975,9 +979,16 @@ static void nvdimm_build_common_dsm(Aml *dev)
>  
>      aml_append(method, aml_store(aml_name(NVDIMM_ACPI_MEM_ADDR), dsm_mem));
>  
> +    if (nvdimm_state->dsm_io.space_id == AML_AS_SYSTEM_IO) {
> +        rs = AML_SYSTEM_IO;
> +    } else {
> +        rs = AML_SYSTEM_MEMORY;
> +    }
> +
>      /* map DSM memory and IO into ACPI namespace. */
> -    aml_append(method, aml_operation_region(NVDIMM_DSM_IOPORT, AML_SYSTEM_IO,
> -               aml_int(NVDIMM_ACPI_IO_BASE), NVDIMM_ACPI_IO_LEN));
> +    aml_append(method, aml_operation_region(NVDIMM_DSM_IOPORT, rs,
> +               aml_int(nvdimm_state->dsm_io.address),
> +               nvdimm_state->dsm_io.bit_width >> 3));
>      aml_append(method, aml_operation_region(NVDIMM_DSM_MEMORY,
>                 AML_SYSTEM_MEMORY, dsm_mem, sizeof(NvdimmDsmIn)));
>  
> @@ -992,7 +1003,7 @@ static void nvdimm_build_common_dsm(Aml *dev)
>      field = aml_field(NVDIMM_DSM_IOPORT, AML_DWORD_ACC, AML_NOLOCK,
>                        AML_PRESERVE);
>      aml_append(field, aml_named_field(NVDIMM_DSM_NOTIFY,
> -               NVDIMM_ACPI_IO_LEN * BITS_PER_BYTE));
> +              (nvdimm_state->dsm_io.bit_width >> 3) * BITS_PER_BYTE));
Why are you converting bits to bytes and then back to bits, here?


>      aml_append(method, field);
>  
>      /*
> @@ -1260,7 +1271,8 @@ static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots)
>  }
>  
>  static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
> -                              BIOSLinker *linker, GArray *dsm_dma_area,
> +                              BIOSLinker *linker,
> +                              NVDIMMState *nvdimm_state,
>                                uint32_t ram_slots)
>  {
>      Aml *ssdt, *sb_scope, *dev;
> @@ -1288,7 +1300,7 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
>       */
>      aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012")));
>  
> -    nvdimm_build_common_dsm(dev);
> +    nvdimm_build_common_dsm(dev, nvdimm_state);
>  
>      /* 0 is reserved for root device. */
>      nvdimm_build_device_dsm(dev, 0);
> @@ -1307,7 +1319,7 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
>                                                 NVDIMM_ACPI_MEM_ADDR);
>  
>      bios_linker_loader_alloc(linker,
> -                             NVDIMM_DSM_MEM_FILE, dsm_dma_area,
> +                             NVDIMM_DSM_MEM_FILE, nvdimm_state->dsm_mem,
>                               sizeof(NvdimmDsmIn), false /* high memory */);
>      bios_linker_loader_add_pointer(linker,
>          ACPI_BUILD_TABLE_FILE, mem_addr_offset, sizeof(uint32_t),
> @@ -1329,7 +1341,7 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
>          return;
>      }
>  
> -    nvdimm_build_ssdt(table_offsets, table_data, linker, state->dsm_mem,
> +    nvdimm_build_ssdt(table_offsets, table_data, linker, state,
>                        ram_slots);
>  
>      device_list = nvdimm_get_device_list();
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 1d077a7cb7..b5170912a8 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -126,6 +126,12 @@ typedef struct FwCfgTPMConfig {
>  
>  static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg);
>  
> +const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio = {
> +    .space_id = AML_AS_SYSTEM_IO,
> +    .address = NVDIMM_ACPI_IO_BASE,
> +    .bit_width = NVDIMM_ACPI_IO_LEN << 3
> +};
> +
>  static void init_common_fadt_data(MachineState *ms, Object *o,
>                                    AcpiFadtData *data)
>  {
> diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> index 007332e51c..74df5fc612 100644
> --- a/hw/i386/acpi-build.h
> +++ b/hw/i386/acpi-build.h
> @@ -1,6 +1,9 @@
>  
>  #ifndef HW_I386_ACPI_BUILD_H
>  #define HW_I386_ACPI_BUILD_H
> +#include "hw/acpi/acpi-defs.h"
> +
> +extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
>  
>  void acpi_setup(void);
>  
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 6824b72124..78521cf017 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -58,6 +58,7 @@
>  #include "migration/misc.h"
>  #include "kvm_i386.h"
>  #include "sysemu/numa.h"
> +#include "hw/i386/acpi-build.h"
>  
>  #define MAX_IDE_BUS 2
>  
> @@ -303,6 +304,7 @@ else {
>  
>      if (machine->nvdimms_state->is_enabled) {
>          nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
> +                               x86_nvdimm_acpi_dsmio,
>                                 pcms->fw_cfg, OBJECT(pcms));
>      }
>  }
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 8fad20f314..d53ee8de84 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -53,6 +53,7 @@
>  #include "qapi/error.h"
>  #include "qemu/error-report.h"
>  #include "sysemu/numa.h"
> +#include "hw/i386/acpi-build.h"
>  
>  /* ICH9 AHCI has 6 ports */
>  #define MAX_SATA_PORTS     6
> @@ -330,6 +331,7 @@ static void pc_q35_init(MachineState *machine)
>  
>      if (machine->nvdimms_state->is_enabled) {
>          nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
> +                               x86_nvdimm_acpi_dsmio,
>                                 pcms->fw_cfg, OBJECT(pcms));
>      }
>  }
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index 523a9b3d4a..5fe440861e 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -25,6 +25,7 @@
>  
>  #include "hw/mem/pc-dimm.h"
>  #include "hw/acpi/bios-linker-loader.h"
> +#include "hw/acpi/aml-build.h"
>  
>  #define NVDIMM_DEBUG 0
>  #define nvdimm_debug(fmt, ...)                                \
> @@ -140,10 +141,12 @@ struct NVDIMMState {
>       */
>      int32_t persistence;
>      char    *persistence_string;
> +    struct AcpiGenericAddress dsm_io;
>  };
>  typedef struct NVDIMMState NVDIMMState;
>  
>  void nvdimm_init_acpi_state(NVDIMMState *state, MemoryRegion *io,
> +                            struct AcpiGenericAddress dsm_io,
>                              FWCfgState *fw_cfg, Object *owner);
>  void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
>                         BIOSLinker *linker, NVDIMMState *state,



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/5] hw/arm/virt: Add nvdimm hot-plug infrastructure
  2019-10-04 15:53 ` [PATCH 3/5] hw/arm/virt: Add nvdimm hot-plug infrastructure Shameer Kolothum
@ 2019-11-11 14:38   ` Igor Mammedov
  0 siblings, 0 replies; 15+ messages in thread
From: Igor Mammedov @ 2019-11-11 14:38 UTC (permalink / raw)
  To: Shameer Kolothum
  Cc: peter.maydell, shannon.zhaosl, qemu-devel, xuwei5, linuxarm,
	eric.auger, qemu-arm, lersek

On Fri, 4 Oct 2019 16:53:00 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> From: Kwangwoo Lee <kwangwoo.lee@sk.com>
> 
> Pre-plug and plug handlers are prepared for NVDIMM support.

Prepare pre-plug and plug handlers for NVDIMM support.

> Please note nvdimm_support is not yet enabled.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  hw/arm/Kconfig           |  1 +
>  hw/arm/virt-acpi-build.c |  6 ++++++
>  hw/arm/virt.c            | 22 +++++++++++++++++++++-
>  hw/mem/Kconfig           |  2 +-
>  include/hw/arm/virt.h    |  1 +
>  5 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index c6e7782580..851dd81289 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -24,6 +24,7 @@ config ARM_VIRT
>      select DIMM
>      select ACPI_MEMORY_HOTPLUG
>      select ACPI_HW_REDUCED
> +    select ACPI_NVDIMM
>  
>  config CHEETAH
>      bool
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 074e0c858e..4e63f5da48 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -44,6 +44,7 @@
>  #include "hw/pci/pcie_host.h"
>  #include "hw/pci/pci.h"
>  #include "hw/arm/virt.h"
> +#include "hw/mem/nvdimm.h"
>  #include "sysemu/numa.h"
>  #include "sysemu/reset.h"
>  #include "kvm_arm.h"
> @@ -835,6 +836,11 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>          }
>      }
>  
> +    if (ms->nvdimms_state->is_enabled) {
> +        nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
> +                          ms->nvdimms_state, ms->ram_slots);
> +    }
> +
>      if (its_class_name() && !vmc->no_its) {
>          acpi_add_table(table_offsets, tables_blob);
>          build_iort(tables_blob, tables->linker, vms);
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index d4bedc2607..30bc8a7803 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -143,6 +143,7 @@ static const MemMapEntry base_memmap[] = {
>      [VIRT_SMMU] =               { 0x09050000, 0x00020000 },
>      [VIRT_PCDIMM_ACPI] =        { 0x09070000, MEMORY_HOTPLUG_IO_LEN },
>      [VIRT_ACPI_GED] =           { 0x09080000, ACPI_GED_EVT_SEL_LEN },
> +    [VIRT_NVDIMM_ACPI] =        { 0x09090000, NVDIMM_ACPI_IO_LEN},
>      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
>      [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
> @@ -1750,6 +1751,18 @@ static void machvirt_init(MachineState *machine)
>  
>      create_platform_bus(vms, pic);
>  
> +    if (machine->nvdimms_state->is_enabled) {
> +        const struct AcpiGenericAddress arm_virt_nvdimm_acpi_dsmio = {
> +            .space_id = AML_AS_SYSTEM_MEMORY,
> +            .address = vms->memmap[VIRT_NVDIMM_ACPI].base,
> +            .bit_width = NVDIMM_ACPI_IO_LEN << 3
> +        };
> +
> +        nvdimm_init_acpi_state(machine->nvdimms_state, sysmem,
> +                               arm_virt_nvdimm_acpi_dsmio,
> +                               vms->fw_cfg, OBJECT(vms));
> +    }
> +
>      vms->bootinfo.ram_size = machine->ram_size;
>      vms->bootinfo.nb_cpus = smp_cpus;
>      vms->bootinfo.board_id = -1;
> @@ -1916,9 +1929,10 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                                   Error **errp)
>  {
>      VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> +    MachineState *ms = MACHINE(hotplug_dev);
>      const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>  
> -    if (is_nvdimm) {
> +    if (is_nvdimm && (!ms->nvdimms_state->is_enabled)) {
shouldn't shouldn't this hunk go to 5/5 where functionality is eventually
enabled and use the same error message as in pc_memory_pre_plug()

>          error_setg(errp, "nvdimm is not yet supported");
>          return;
>      }
> @@ -1937,6 +1951,8 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
>  {
>      HotplugHandlerClass *hhc;
>      VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> +    MachineState *ms = MACHINE(hotplug_dev);
> +    bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>      Error *local_err = NULL;
>  
>      pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), &local_err);
> @@ -1944,6 +1960,10 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
>          goto out;
>      }
>  
> +    if (is_nvdimm) {
> +        nvdimm_plug(ms->nvdimms_state);
> +    }
> +
>      hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
>      hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, &error_abort);
>  out:
> diff --git a/hw/mem/Kconfig b/hw/mem/Kconfig
> index 620fd4cb59..0d5f8f321a 100644
> --- a/hw/mem/Kconfig
> +++ b/hw/mem/Kconfig
> @@ -8,4 +8,4 @@ config MEM_DEVICE
>  config NVDIMM
>      bool
>      default y
> -    depends on PC
> +    depends on PC || ARM_VIRT
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 0b41083e9d..06d5e75611 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -79,6 +79,7 @@ enum {
>      VIRT_SECURE_MEM,
>      VIRT_PCDIMM_ACPI,
>      VIRT_ACPI_GED,
> +    VIRT_NVDIMM_ACPI,
>      VIRT_LOWMEMMAP_LAST,
>  };
>  



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/5] hw/arm/boot: Expose the pmem nodes in the DT
  2019-10-04 15:53 ` [PATCH 4/5] hw/arm/boot: Expose the pmem nodes in the DT Shameer Kolothum
@ 2019-11-11 14:46   ` Igor Mammedov
  0 siblings, 0 replies; 15+ messages in thread
From: Igor Mammedov @ 2019-11-11 14:46 UTC (permalink / raw)
  To: Shameer Kolothum
  Cc: peter.maydell, shannon.zhaosl, qemu-devel, xuwei5, linuxarm,
	eric.auger, qemu-arm, lersek

On Fri, 4 Oct 2019 16:53:01 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> From: Eric Auger <eric.auger@redhat.com>
> 
> In case of NV-DIMM slots, let's add /pmem DT nodes
Why should we do it for NVDIMM but not for PC-DIMM?

> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  hw/arm/boot.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index c264864c11..bd6d72b33e 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -20,6 +20,7 @@
>  #include "hw/boards.h"
>  #include "sysemu/reset.h"
>  #include "hw/loader.h"
> +#include "hw/mem/memory-device.h"
>  #include "elf.h"
>  #include "sysemu/device_tree.h"
>  #include "qemu/config-file.h"
> @@ -523,6 +524,44 @@ static void fdt_add_psci_node(void *fdt)
>      qemu_fdt_setprop_cell(fdt, "/psci", "migrate", migrate_fn);
>  }
>  
> +static int fdt_add_pmem_node(void *fdt, uint32_t acells, uint32_t scells)
> +{
> +    MemoryDeviceInfoList *info, *info_list = qmp_memory_device_list();
> +    MemoryDeviceInfo *mi;
> +    int ret;
> +
> +    for (info = info_list; info != NULL; info = info->next) {
> +        mi = info->value;
> +
> +        if (mi->type == MEMORY_DEVICE_INFO_KIND_NVDIMM) {
> +            PCDIMMDeviceInfo *di = mi->u.nvdimm.data;
> +            char *nodename;
> +
> +            nodename = g_strdup_printf("/pmem@%" PRIx64, di->addr);
> +            qemu_fdt_add_subnode(fdt, nodename);
> +            qemu_fdt_setprop_string(fdt, nodename, "compatible", "pmem-region");
> +            ret = qemu_fdt_setprop_sized_cells(fdt, nodename, "reg", acells,
> +                                               di->addr, scells, di->size);
> +            /* only set the NUMA ID if it is specified */
> +            if (!ret && di->node >= 0) {
> +                ret = qemu_fdt_setprop_cell(fdt, nodename, "numa-node-id",
> +                                            di->node);
> +            }
> +
> +            g_free(nodename);
> +
> +            if (ret < 0) {
> +                fprintf(stderr, "couldn't add NVDIMM /memory@%"PRIx64" node\n",
shouldn't it be s:/memory:/pmem:

and maybe move error printing outside like it's done in fdt_add_memory_node()
to be consistent with current code.

> +                        di->addr);
> +                goto out;
> +            }
> +        }
> +    }
> +out:
> +    qapi_free_MemoryDeviceInfoList(info_list);
> +    return ret;
> +}
> +
>  int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>                   hwaddr addr_limit, AddressSpace *as, MachineState *ms)
>  {
> @@ -622,6 +661,12 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>          }
>      }
>  
> +    rc = fdt_add_pmem_node(fdt, acells, scells);
> +    if (rc < 0) {
> +        fprintf(stderr, "couldn't add pmem memory nodes\n");
> +        goto fail;
> +    }
> +
>      rc = fdt_path_offset(fdt, "/chosen");
>      if (rc < 0) {
>          qemu_fdt_add_subnode(fdt, "/chosen");



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/5] hw/arm/virt: Add nvdimm hotplug support
  2019-10-04 15:53 ` [PATCH 5/5] hw/arm/virt: Add nvdimm hotplug support Shameer Kolothum
@ 2019-11-12 13:01   ` Igor Mammedov
  0 siblings, 0 replies; 15+ messages in thread
From: Igor Mammedov @ 2019-11-12 13:01 UTC (permalink / raw)
  To: Shameer Kolothum
  Cc: peter.maydell, shannon.zhaosl, qemu-devel, xuwei5, linuxarm,
	eric.auger, qemu-arm, lersek

On Fri, 4 Oct 2019 16:53:02 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> This adds support for nvdimm hotplug events through GED
> and enables nvdimm for the arm/virt. Now Guests with DT boot
> can have nvdimm cold plug and with ACPI both cold/hot plug.
> 
> Hot removal functionality is not yet supported.
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

I'd move out DT cold-plug out of this patch is it's not related
to this patch at all

otherwise patch looks good, so 
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  docs/specs/acpi_hw_reduced_hotplug.rst |  1 +
>  hw/acpi/generic_event_device.c         | 13 +++++++++++++
>  hw/arm/virt.c                          | 20 +++++++++++++++-----
>  include/hw/acpi/generic_event_device.h |  1 +
>  4 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/docs/specs/acpi_hw_reduced_hotplug.rst b/docs/specs/acpi_hw_reduced_hotplug.rst
> index 911a98255b..e3abe975bf 100644
> --- a/docs/specs/acpi_hw_reduced_hotplug.rst
> +++ b/docs/specs/acpi_hw_reduced_hotplug.rst
> @@ -63,6 +63,7 @@ GED IO interface (4 byte access)
>      bits:
>         0: Memory hotplug event
>         1: System power down event
> +       2: NVDIMM hotplug event
>      2-31: Reserved
>  
>  **write_access:**
> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> index 9cee90cc70..ad1b684304 100644
> --- a/hw/acpi/generic_event_device.c
> +++ b/hw/acpi/generic_event_device.c
> @@ -16,6 +16,7 @@
>  #include "hw/acpi/generic_event_device.h"
>  #include "hw/irq.h"
>  #include "hw/mem/pc-dimm.h"
> +#include "hw/mem/nvdimm.h"
>  #include "hw/qdev-properties.h"
>  #include "migration/vmstate.h"
>  #include "qemu/error-report.h"
> @@ -23,6 +24,7 @@
>  static const uint32_t ged_supported_events[] = {
>      ACPI_GED_MEM_HOTPLUG_EVT,
>      ACPI_GED_PWR_DOWN_EVT,
> +    ACPI_GED_NVDIMM_HOTPLUG_EVT,
>  };
>  
>  /*
> @@ -110,6 +112,11 @@ void build_ged_aml(Aml *table, const char *name, HotplugHandler *hotplug_dev,
>                             aml_notify(aml_name(ACPI_POWER_BUTTON_DEVICE),
>                                        aml_int(0x80)));
>                  break;
> +            case ACPI_GED_NVDIMM_HOTPLUG_EVT:
> +                aml_append(if_ctx,
> +                           aml_notify(aml_name("\\_SB.NVDR"),
> +                                      aml_int(0x80)));
> +                break;
>              default:
>                  /*
>                   * Please make sure all the events in ged_supported_events[]
> @@ -175,7 +182,11 @@ static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,
>      AcpiGedState *s = ACPI_GED(hotplug_dev);
>  
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +        if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> +            nvdimm_acpi_plug_cb(hotplug_dev, dev);
> +        } else {
>              acpi_memory_plug_cb(hotplug_dev, &s->memhp_state, dev, errp);
> +        }
>      } else {
>          error_setg(errp, "virt: device plug request for unsupported device"
>                     " type: %s", object_get_typename(OBJECT(dev)));
> @@ -192,6 +203,8 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
>          sel = ACPI_GED_MEM_HOTPLUG_EVT;
>      } else if (ev & ACPI_POWER_DOWN_STATUS) {
>          sel = ACPI_GED_PWR_DOWN_EVT;
> +    } else if (ev & ACPI_NVDIMM_HOTPLUG_STATUS) {
> +        sel = ACPI_GED_NVDIMM_HOTPLUG_EVT;
>      } else {
>          /* Unknown event. Return without generating interrupt. */
>          warn_report("GED: Unsupported event %d. No irq injected", ev);
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 30bc8a7803..acdcba9638 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -543,6 +543,10 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms, qemu_irq *pic)
>          event |= ACPI_GED_MEM_HOTPLUG_EVT;
>      }
>  
> +    if (ms->nvdimms_state->is_enabled) {
> +        event |= ACPI_GED_NVDIMM_HOTPLUG_EVT;
> +    }
> +
>      dev = qdev_create(NULL, TYPE_ACPI_GED);
>      qdev_prop_set_uint32(dev, "ged-event", event);
>  
> @@ -1938,9 +1942,12 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      }
>  
>      if (!vms->acpi_dev) {
> -        error_setg(errp,
> -                   "memory hotplug is not enabled: missing acpi-ged device");
> -        return;
> +        /* Allow nvdimm DT or cold plug */
> +        if (!(is_nvdimm && !dev->hotplugged)) {
> +            error_setg(errp,
> +                       "memory hotplug is not enabled: missing acpi-ged device");
> +            return;
> +         }



>      }
>  
>      pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL, errp);
> @@ -1964,8 +1971,10 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
>          nvdimm_plug(ms->nvdimms_state);
>      }
>  
> -    hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
> -    hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, &error_abort);
> +    if (vms->acpi_dev) {
> +        hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
> +        hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, &error_abort);
> +    }
>  out:
>      error_propagate(errp, local_err);
>  }
> @@ -2073,6 +2082,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>      hc->plug = virt_machine_device_plug_cb;
>      hc->unplug_request = virt_machine_device_unplug_request_cb;
>      mc->numa_mem_supported = true;
> +    mc->nvdimm_supported = true;
>      mc->auto_enable_numa_with_memhp = true;
>  }
>  
> diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
> index d157eac088..9eb86ca4fd 100644
> --- a/include/hw/acpi/generic_event_device.h
> +++ b/include/hw/acpi/generic_event_device.h
> @@ -82,6 +82,7 @@
>   */
>  #define ACPI_GED_MEM_HOTPLUG_EVT   0x1
>  #define ACPI_GED_PWR_DOWN_EVT      0x2
> +#define ACPI_GED_NVDIMM_HOTPLUG_EVT 0x4
>  
>  typedef struct GEDState {
>      MemoryRegion io;



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/5] ARM virt: Add NVDIMM support
  2019-10-04 15:52 [PATCH 0/5] ARM virt: Add NVDIMM support Shameer Kolothum
                   ` (5 preceding siblings ...)
  2019-10-18 16:39 ` [PATCH 0/5] ARM virt: Add NVDIMM support Auger Eric
@ 2019-11-12 14:39 ` Igor Mammedov
  6 siblings, 0 replies; 15+ messages in thread
From: Igor Mammedov @ 2019-11-12 14:39 UTC (permalink / raw)
  To: Shameer Kolothum
  Cc: peter.maydell, shannon.zhaosl, qemu-devel, xuwei5, linuxarm,
	eric.auger, qemu-arm, lersek

On Fri, 4 Oct 2019 16:52:57 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> This series adds NVDIMM support to arm/virt platform.
> This has a dependency on [0] and make use of the GED
> device for NVDIMM hotplug events. The series reuses
> some of the patches posted by Eric in his earlier
> attempt here[1].
> 
> Patch 1/5 is a fix to the Guest reboot issue on NVDIMM
> hot add case described here[2].
> 
> I have done basic sanity testing of NVDIMM deviecs with
> both ACPI and DT Guest boot. Further testing is always
> welcome.
> 
> Please let me know your feedback.
one more thing,

It's possible to run bios-tables-test for virt/arm now,
pls add corresponding test case
it might be easier to just extend test_acpi_virt_tcg_memhp() with nvdimms

> 
> Thanks,
> Shameer
> 
> [0] https://patchwork.kernel.org/cover/11150345/
> [1] https://patchwork.kernel.org/cover/10830777/
> [2] https://patchwork.kernel.org/patch/11154757/
> 
> Eric Auger (1):
>   hw/arm/boot: Expose the pmem nodes in the DT
> 
> Kwangwoo Lee (2):
>   nvdimm: Use configurable ACPI IO base and size
>   hw/arm/virt: Add nvdimm hot-plug infrastructure
> 
> Shameer Kolothum (2):
>   hw/arm: Align ACPI blob len to PAGE size
>   hw/arm/virt: Add nvdimm hotplug support
> 
>  docs/specs/acpi_hw_reduced_hotplug.rst |  1 +
>  hw/acpi/generic_event_device.c         | 13 ++++++++
>  hw/acpi/nvdimm.c                       | 32 ++++++++++++------
>  hw/arm/Kconfig                         |  1 +
>  hw/arm/boot.c                          | 45 ++++++++++++++++++++++++++
>  hw/arm/virt-acpi-build.c               | 20 ++++++++++++
>  hw/arm/virt.c                          | 42 ++++++++++++++++++++----
>  hw/i386/acpi-build.c                   |  6 ++++
>  hw/i386/acpi-build.h                   |  3 ++
>  hw/i386/pc_piix.c                      |  2 ++
>  hw/i386/pc_q35.c                       |  2 ++
>  hw/mem/Kconfig                         |  2 +-
>  include/hw/acpi/generic_event_device.h |  1 +
>  include/hw/arm/virt.h                  |  1 +
>  include/hw/mem/nvdimm.h                |  3 ++
>  15 files changed, 157 insertions(+), 17 deletions(-)
> 



^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, back to index

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-04 15:52 [PATCH 0/5] ARM virt: Add NVDIMM support Shameer Kolothum
2019-10-04 15:52 ` [PATCH 1/5] hw/arm: Align ACPI blob len to PAGE size Shameer Kolothum
2019-11-08 16:17   ` Igor Mammedov
2019-11-11 12:47     ` Shameerali Kolothum Thodi
2019-10-04 15:52 ` [PATCH 2/5] nvdimm: Use configurable ACPI IO base and size Shameer Kolothum
2019-11-11 14:24   ` Igor Mammedov
2019-10-04 15:53 ` [PATCH 3/5] hw/arm/virt: Add nvdimm hot-plug infrastructure Shameer Kolothum
2019-11-11 14:38   ` Igor Mammedov
2019-10-04 15:53 ` [PATCH 4/5] hw/arm/boot: Expose the pmem nodes in the DT Shameer Kolothum
2019-11-11 14:46   ` Igor Mammedov
2019-10-04 15:53 ` [PATCH 5/5] hw/arm/virt: Add nvdimm hotplug support Shameer Kolothum
2019-11-12 13:01   ` Igor Mammedov
2019-10-18 16:39 ` [PATCH 0/5] ARM virt: Add NVDIMM support Auger Eric
2019-10-22 14:05   ` Shameerali Kolothum Thodi
2019-11-12 14:39 ` Igor Mammedov

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git