* [Qemu-devel] [RFC PATCH 1/3] hw/arm/virt: add hotplug memory support
2016-07-20 0:49 [Qemu-devel] [RFC PATCH 0/3] add nvdimm support on AArch64 virt platform Kwangwoo Lee
@ 2016-07-20 0:49 ` Kwangwoo Lee
2016-07-29 18:10 ` Peter Maydell
2016-07-20 0:49 ` [Qemu-devel] [RFC PATCH 2/3] nvdimm: use configurable ACPI IO base and size Kwangwoo Lee
` (2 subsequent siblings)
3 siblings, 1 reply; 22+ messages in thread
From: Kwangwoo Lee @ 2016-07-20 0:49 UTC (permalink / raw)
To: Xiao Guangrong, Peter Maydell, Michael S. Tsirkin, Igor Mammedov,
Paolo Bonzini, Richard Henderson, Eduardo Habkost, Shannon Zhao,
Shannon Zhao, qemu-devel, qemu-arm
Cc: Kwangwoo Lee, Woosuk Chung, Hyunchul Kim
Add hotplug memory feature on aarch64 virt platfom. This patch is
required to emulate a DIMM type memory like NVDIMM.
Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com>
---
default-configs/aarch64-softmmu.mak | 1 +
hw/arm/virt.c | 110 ++++++++++++++++++++++++++++++++++++
include/hw/arm/virt.h | 3 +
3 files changed, 114 insertions(+)
diff --git a/default-configs/aarch64-softmmu.mak b/default-configs/aarch64-softmmu.mak
index 2449483..5790cd2 100644
--- a/default-configs/aarch64-softmmu.mak
+++ b/default-configs/aarch64-softmmu.mak
@@ -7,3 +7,4 @@ CONFIG_AUX=y
CONFIG_DDC=y
CONFIG_DPCD=y
CONFIG_XLNX_ZYNQMP=y
+CONFIG_MEM_HOTPLUG=y
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index a193b5a..f7ff411 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -58,6 +58,8 @@
#include "hw/smbios/smbios.h"
#include "qapi/visitor.h"
#include "standard-headers/linux/input.h"
+#include "hw/mem/pc-dimm.h"
+#include "hw/acpi/acpi.h"
/* Number of external interrupt lines to configure the GIC with */
#define NUM_IRQS 256
@@ -91,6 +93,7 @@ typedef struct {
bool secure;
bool highmem;
int32_t gic_version;
+ MemoryHotplugState hotplug_memory;
} VirtMachineState;
#define TYPE_VIRT_MACHINE MACHINE_TYPE_NAME("virt")
@@ -1376,6 +1379,40 @@ static void machvirt_init(MachineState *machine)
guest_info_state->machine_done.notify = virt_guest_info_machine_done;
qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
+ /* initialize hotplug memory address space */
+ if (machine->ram_size < machine->maxram_size) {
+ ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
+
+ if (machine->ram_slots > ACPI_MAX_RAM_SLOTS) {
+ error_report("unsupported amount of memory slots: %"PRIu64,
+ machine->ram_slots);
+ exit(EXIT_FAILURE);
+ }
+
+ if (QEMU_ALIGN_UP(machine->maxram_size,
+ TARGET_PAGE_SIZE) != machine->maxram_size) {
+ error_report("maximum memory size must by aligned to multiple of "
+ "%d bytes", TARGET_PAGE_SIZE);
+ exit(EXIT_FAILURE);
+ }
+
+ vms->hotplug_memory.base =
+ ROUND_UP(vbi->memmap[VIRT_MEM].base + machine->ram_size,
+ ARCH_VIRT_HOTPLUG_MEM_ALIGN);
+
+ if ((vms->hotplug_memory.base + hotplug_mem_size) <
+ hotplug_mem_size) {
+ error_report("unsupported amount of maximum memory: " RAM_ADDR_FMT,
+ machine->maxram_size);
+ exit(EXIT_FAILURE);
+ }
+
+ memory_region_init(&vms->hotplug_memory.mr, OBJECT(vms),
+ "hotplug-memory", hotplug_mem_size);
+ memory_region_add_subregion(sysmem, vms->hotplug_memory.base,
+ &vms->hotplug_memory.mr);
+ }
+
vbi->bootinfo.ram_size = machine->ram_size;
vbi->bootinfo.kernel_filename = machine->kernel_filename;
vbi->bootinfo.kernel_cmdline = machine->kernel_cmdline;
@@ -1448,9 +1485,75 @@ static void virt_set_gic_version(Object *obj, const char *value, Error **errp)
}
}
+static void virt_dimm_plug(HotplugHandler *hotplug_dev,
+ DeviceState *dev, Error **errp)
+{
+ VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
+ PCDIMMDevice *dimm = PC_DIMM(dev);
+ PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
+ MemoryRegion *mr = ddc->get_memory_region(dimm);
+ Error *local_err = NULL;
+ uint64_t align = memory_region_get_alignment(mr);
+
+ pc_dimm_memory_plug(dev, &vms->hotplug_memory, mr, align, &local_err);
+ if (local_err) {
+ goto out;
+ }
+
+out:
+ error_propagate(errp, local_err);
+}
+
+static void virt_dimm_unplug(HotplugHandler *hotplug_dev,
+ DeviceState *dev, Error **errp)
+{
+ VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
+ PCDIMMDevice *dimm = PC_DIMM(dev);
+ PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
+ MemoryRegion *mr = ddc->get_memory_region(dimm);
+ Error *local_err = NULL;
+
+ pc_dimm_memory_unplug(dev, &vms->hotplug_memory, mr);
+ object_unparent(OBJECT(dev));
+
+ error_propagate(errp, local_err);
+}
+
+static void virt_machinedevice_plug_cb(HotplugHandler *hotplug_dev,
+ DeviceState *dev, Error **errp)
+{
+ if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+ virt_dimm_plug(hotplug_dev, dev, errp);
+ } else {
+ error_setg(errp, "acpi: device plug request for not supported device"
+ " type: %s", object_get_typename(OBJECT(dev)));
+ }
+}
+
+static void virt_machinedevice_unplug_cb(HotplugHandler *hotplug_dev,
+ DeviceState *dev, Error **errp)
+{
+ if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+ virt_dimm_unplug(hotplug_dev, dev, errp);
+ } else {
+ error_setg(errp, "acpi: device unplug for not supported device"
+ " type: %s", object_get_typename(OBJECT(dev)));
+ }
+}
+
+static HotplugHandler *virt_get_hotplug_handler(MachineState *machine,
+ DeviceState *dev)
+{
+ if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+ return HOTPLUG_HANDLER(machine);
+ }
+ return NULL;
+}
+
static void virt_machine_class_init(ObjectClass *oc, void *data)
{
MachineClass *mc = MACHINE_CLASS(oc);
+ HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
mc->init = machvirt_init;
/* Start max_cpus at the maximum QEMU supports. We'll further restrict
@@ -1462,6 +1565,9 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
mc->block_default_type = IF_VIRTIO;
mc->no_cdrom = 1;
mc->pci_allow_0_address = true;
+ mc->get_hotplug_handler = virt_get_hotplug_handler;
+ hc->plug = virt_machinedevice_plug_cb;
+ hc->unplug = virt_machinedevice_unplug_cb;
}
static const TypeInfo virt_machine_info = {
@@ -1471,6 +1577,10 @@ static const TypeInfo virt_machine_info = {
.instance_size = sizeof(VirtMachineState),
.class_size = sizeof(VirtMachineClass),
.class_init = virt_machine_class_init,
+ .interfaces = (InterfaceInfo[]) {
+ { TYPE_HOTPLUG_HANDLER },
+ { }
+ },
};
static void machvirt_machine_init(void)
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 9650193..35aa3cc 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -45,6 +45,9 @@
#define PPI(irq) ((irq) + 16)
+/* 1GB alignment for hotplug memory region */
+#define ARCH_VIRT_HOTPLUG_MEM_ALIGN (1ULL << 30)
+
enum {
VIRT_FLASH,
VIRT_MEM,
--
2.5.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [RFC PATCH 1/3] hw/arm/virt: add hotplug memory support
2016-07-20 0:49 ` [Qemu-devel] [RFC PATCH 1/3] hw/arm/virt: add hotplug memory support Kwangwoo Lee
@ 2016-07-29 18:10 ` Peter Maydell
2016-08-01 0:35 ` kwangwoo.lee
0 siblings, 1 reply; 22+ messages in thread
From: Peter Maydell @ 2016-07-29 18:10 UTC (permalink / raw)
To: Kwangwoo Lee
Cc: Xiao Guangrong, Michael S. Tsirkin, Igor Mammedov, Paolo Bonzini,
Richard Henderson, Eduardo Habkost, Shannon Zhao, Shannon Zhao,
QEMU Developers, qemu-arm, Woosuk Chung, Hyunchul Kim
On 20 July 2016 at 01:49, Kwangwoo Lee <kwangwoo.lee@sk.com> wrote:
> Add hotplug memory feature on aarch64 virt platfom. This patch is
> required to emulate a DIMM type memory like NVDIMM.
>
> Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com>
> ---
> default-configs/aarch64-softmmu.mak | 1 +
> hw/arm/virt.c | 110 ++++++++++++++++++++++++++++++++++++
> include/hw/arm/virt.h | 3 +
> 3 files changed, 114 insertions(+)
>
> diff --git a/default-configs/aarch64-softmmu.mak b/default-configs/aarch64-softmmu.mak
> index 2449483..5790cd2 100644
> --- a/default-configs/aarch64-softmmu.mak
> +++ b/default-configs/aarch64-softmmu.mak
> @@ -7,3 +7,4 @@ CONFIG_AUX=y
> CONFIG_DDC=y
> CONFIG_DPCD=y
> CONFIG_XLNX_ZYNQMP=y
> +CONFIG_MEM_HOTPLUG=y
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index a193b5a..f7ff411 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -58,6 +58,8 @@
> #include "hw/smbios/smbios.h"
> #include "qapi/visitor.h"
> #include "standard-headers/linux/input.h"
> +#include "hw/mem/pc-dimm.h"
> +#include "hw/acpi/acpi.h"
>
> /* Number of external interrupt lines to configure the GIC with */
> #define NUM_IRQS 256
> @@ -91,6 +93,7 @@ typedef struct {
> bool secure;
> bool highmem;
> int32_t gic_version;
> + MemoryHotplugState hotplug_memory;
> } VirtMachineState;
>
> #define TYPE_VIRT_MACHINE MACHINE_TYPE_NAME("virt")
> @@ -1376,6 +1379,40 @@ static void machvirt_init(MachineState *machine)
> guest_info_state->machine_done.notify = virt_guest_info_machine_done;
> qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
>
> + /* initialize hotplug memory address space */
> + if (machine->ram_size < machine->maxram_size) {
> + ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
> +
> + if (machine->ram_slots > ACPI_MAX_RAM_SLOTS) {
> + error_report("unsupported amount of memory slots: %"PRIu64,
"number of"
> + machine->ram_slots);
> + exit(EXIT_FAILURE);
> + }
> +
> + if (QEMU_ALIGN_UP(machine->maxram_size,
> + TARGET_PAGE_SIZE) != machine->maxram_size) {
> + error_report("maximum memory size must by aligned to multiple of "
"must be"
> + "%d bytes", TARGET_PAGE_SIZE);
> + exit(EXIT_FAILURE);
> + }
> +
> + vms->hotplug_memory.base =
> + ROUND_UP(vbi->memmap[VIRT_MEM].base + machine->ram_size,
> + ARCH_VIRT_HOTPLUG_MEM_ALIGN);
> +
> + if ((vms->hotplug_memory.base + hotplug_mem_size) <
> + hotplug_mem_size) {
This expression is confusing. Is it trying to test for overflow?
When can that happen?
> + error_report("unsupported amount of maximum memory: " RAM_ADDR_FMT,
> + machine->maxram_size);
> + exit(EXIT_FAILURE);
> + }
> +
> + memory_region_init(&vms->hotplug_memory.mr, OBJECT(vms),
> + "hotplug-memory", hotplug_mem_size);
> + memory_region_add_subregion(sysmem, vms->hotplug_memory.base,
> + &vms->hotplug_memory.mr);
> + }
> +
> vbi->bootinfo.ram_size = machine->ram_size;
> vbi->bootinfo.kernel_filename = machine->kernel_filename;
> vbi->bootinfo.kernel_cmdline = machine->kernel_cmdline;
> @@ -1448,9 +1485,75 @@ static void virt_set_gic_version(Object *obj, const char *value, Error **errp)
> }
> }
>
> +static void virt_dimm_plug(HotplugHandler *hotplug_dev,
> + DeviceState *dev, Error **errp)
> +{
> + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> + PCDIMMDevice *dimm = PC_DIMM(dev);
> + PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> + MemoryRegion *mr = ddc->get_memory_region(dimm);
> + Error *local_err = NULL;
> + uint64_t align = memory_region_get_alignment(mr);
> +
> + pc_dimm_memory_plug(dev, &vms->hotplug_memory, mr, align, &local_err);
> + if (local_err) {
> + goto out;
> + }
> +
> +out:
> + error_propagate(errp, local_err);
> +}
> +
> +static void virt_dimm_unplug(HotplugHandler *hotplug_dev,
> + DeviceState *dev, Error **errp)
> +{
> + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> + PCDIMMDevice *dimm = PC_DIMM(dev);
> + PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> + MemoryRegion *mr = ddc->get_memory_region(dimm);
> + Error *local_err = NULL;
> +
> + pc_dimm_memory_unplug(dev, &vms->hotplug_memory, mr);
> + object_unparent(OBJECT(dev));
> +
> + error_propagate(errp, local_err);
> +}
> +
> +static void virt_machinedevice_plug_cb(HotplugHandler *hotplug_dev,
> + DeviceState *dev, Error **errp)
> +{
> + if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> + virt_dimm_plug(hotplug_dev, dev, errp);
> + } else {
> + error_setg(errp, "acpi: device plug request for not supported device"
"unsupported"
> + " type: %s", object_get_typename(OBJECT(dev)));
> + }
> +}
> +
> +static void virt_machinedevice_unplug_cb(HotplugHandler *hotplug_dev,
> + DeviceState *dev, Error **errp)
> +{
> + if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> + virt_dimm_unplug(hotplug_dev, dev, errp);
> + } else {
> + error_setg(errp, "acpi: device unplug for not supported device"
"unsupported"
> + " type: %s", object_get_typename(OBJECT(dev)));
> + }
> +}
> +
> +static HotplugHandler *virt_get_hotplug_handler(MachineState *machine,
> + DeviceState *dev)
> +{
> + if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> + return HOTPLUG_HANDLER(machine);
> + }
> + return NULL;
> +}
> +
> static void virt_machine_class_init(ObjectClass *oc, void *data)
> {
> MachineClass *mc = MACHINE_CLASS(oc);
> + HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
>
> mc->init = machvirt_init;
> /* Start max_cpus at the maximum QEMU supports. We'll further restrict
> @@ -1462,6 +1565,9 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
> mc->block_default_type = IF_VIRTIO;
> mc->no_cdrom = 1;
> mc->pci_allow_0_address = true;
> + mc->get_hotplug_handler = virt_get_hotplug_handler;
> + hc->plug = virt_machinedevice_plug_cb;
> + hc->unplug = virt_machinedevice_unplug_cb;
> }
>
> static const TypeInfo virt_machine_info = {
> @@ -1471,6 +1577,10 @@ static const TypeInfo virt_machine_info = {
> .instance_size = sizeof(VirtMachineState),
> .class_size = sizeof(VirtMachineClass),
> .class_init = virt_machine_class_init,
> + .interfaces = (InterfaceInfo[]) {
> + { TYPE_HOTPLUG_HANDLER },
> + { }
> + },
> };
>
> static void machvirt_machine_init(void)
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 9650193..35aa3cc 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -45,6 +45,9 @@
>
> #define PPI(irq) ((irq) + 16)
>
> +/* 1GB alignment for hotplug memory region */
> +#define ARCH_VIRT_HOTPLUG_MEM_ALIGN (1ULL << 30)
Where does the 1GB alignment come from? Why do we need 1GB
alignment for the base but only 1KB alignment for the size?
> +
> enum {
> VIRT_FLASH,
> VIRT_MEM,
> --
> 2.5.0
thanks
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [RFC PATCH 1/3] hw/arm/virt: add hotplug memory support
2016-07-29 18:10 ` Peter Maydell
@ 2016-08-01 0:35 ` kwangwoo.lee
2016-08-01 7:46 ` Igor Mammedov
0 siblings, 1 reply; 22+ messages in thread
From: kwangwoo.lee @ 2016-08-01 0:35 UTC (permalink / raw)
To: Peter Maydell
Cc: Xiao Guangrong, Michael S. Tsirkin, Igor Mammedov, Paolo Bonzini,
Richard Henderson, Eduardo Habkost, Shannon Zhao, Shannon Zhao,
QEMU Developers, qemu-arm, woosuk.chung, hyunchul3.kim
Hi Peter,
Thanks a lot for your comments! I answered in line below.
> -----Original Message-----
> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Saturday, July 30, 2016 3:10 AM
> To: 이광우(LEE KWANGWOO) MS SW
> Cc: Xiao Guangrong; Michael S. Tsirkin; Igor Mammedov; Paolo Bonzini; Richard Henderson; Eduardo
> Habkost; Shannon Zhao; Shannon Zhao; QEMU Developers; qemu-arm; 정우석(CHUNG WOO SUK) MS SW; 김현철(KIM
> HYUNCHUL) MS SW
> Subject: Re: [RFC PATCH 1/3] hw/arm/virt: add hotplug memory support
>
> On 20 July 2016 at 01:49, Kwangwoo Lee <kwangwoo.lee@sk.com> wrote:
> > Add hotplug memory feature on aarch64 virt platfom. This patch is
> > required to emulate a DIMM type memory like NVDIMM.
> >
> > Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com>
> > ---
> > default-configs/aarch64-softmmu.mak | 1 +
> > hw/arm/virt.c | 110 ++++++++++++++++++++++++++++++++++++
> > include/hw/arm/virt.h | 3 +
> > 3 files changed, 114 insertions(+)
> >
> > diff --git a/default-configs/aarch64-softmmu.mak b/default-configs/aarch64-softmmu.mak
> > index 2449483..5790cd2 100644
> > --- a/default-configs/aarch64-softmmu.mak
> > +++ b/default-configs/aarch64-softmmu.mak
> > @@ -7,3 +7,4 @@ CONFIG_AUX=y
> > CONFIG_DDC=y
> > CONFIG_DPCD=y
> > CONFIG_XLNX_ZYNQMP=y
> > +CONFIG_MEM_HOTPLUG=y
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index a193b5a..f7ff411 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -58,6 +58,8 @@
> > #include "hw/smbios/smbios.h"
> > #include "qapi/visitor.h"
> > #include "standard-headers/linux/input.h"
> > +#include "hw/mem/pc-dimm.h"
> > +#include "hw/acpi/acpi.h"
> >
> > /* Number of external interrupt lines to configure the GIC with */
> > #define NUM_IRQS 256
> > @@ -91,6 +93,7 @@ typedef struct {
> > bool secure;
> > bool highmem;
> > int32_t gic_version;
> > + MemoryHotplugState hotplug_memory;
> > } VirtMachineState;
> >
> > #define TYPE_VIRT_MACHINE MACHINE_TYPE_NAME("virt")
> > @@ -1376,6 +1379,40 @@ static void machvirt_init(MachineState *machine)
> > guest_info_state->machine_done.notify = virt_guest_info_machine_done;
> > qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
> >
> > + /* initialize hotplug memory address space */
> > + if (machine->ram_size < machine->maxram_size) {
> > + ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
> > +
> > + if (machine->ram_slots > ACPI_MAX_RAM_SLOTS) {
> > + error_report("unsupported amount of memory slots: %"PRIu64,
>
> "number of"
OK. I'll fix this.
> > + machine->ram_slots);
> > + exit(EXIT_FAILURE);
> > + }
> > +
> > + if (QEMU_ALIGN_UP(machine->maxram_size,
> > + TARGET_PAGE_SIZE) != machine->maxram_size) {
> > + error_report("maximum memory size must by aligned to multiple of "
>
> "must be"
OK. I'll fix this
> > + "%d bytes", TARGET_PAGE_SIZE);
> > + exit(EXIT_FAILURE);
> > + }
> > +
> > + vms->hotplug_memory.base =
> > + ROUND_UP(vbi->memmap[VIRT_MEM].base + machine->ram_size,
> > + ARCH_VIRT_HOTPLUG_MEM_ALIGN);
> > +
> > + if ((vms->hotplug_memory.base + hotplug_mem_size) <
> > + hotplug_mem_size) {
>
> This expression is confusing. Is it trying to test for overflow?
> When can that happen?
Ah.. you are right. No need to check this. I'll remove this. Thanks!
> > + error_report("unsupported amount of maximum memory: " RAM_ADDR_FMT,
> > + machine->maxram_size);
> > + exit(EXIT_FAILURE);
> > + }
> > +
> > + memory_region_init(&vms->hotplug_memory.mr, OBJECT(vms),
> > + "hotplug-memory", hotplug_mem_size);
> > + memory_region_add_subregion(sysmem, vms->hotplug_memory.base,
> > + &vms->hotplug_memory.mr);
> > + }
> > +
> > vbi->bootinfo.ram_size = machine->ram_size;
> > vbi->bootinfo.kernel_filename = machine->kernel_filename;
> > vbi->bootinfo.kernel_cmdline = machine->kernel_cmdline;
> > @@ -1448,9 +1485,75 @@ static void virt_set_gic_version(Object *obj, const char *value, Error **errp)
> > }
> > }
> >
> > +static void virt_dimm_plug(HotplugHandler *hotplug_dev,
> > + DeviceState *dev, Error **errp)
> > +{
> > + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> > + PCDIMMDevice *dimm = PC_DIMM(dev);
> > + PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > + MemoryRegion *mr = ddc->get_memory_region(dimm);
> > + Error *local_err = NULL;
> > + uint64_t align = memory_region_get_alignment(mr);
> > +
> > + pc_dimm_memory_plug(dev, &vms->hotplug_memory, mr, align, &local_err);
> > + if (local_err) {
> > + goto out;
> > + }
> > +
> > +out:
> > + error_propagate(errp, local_err);
> > +}
> > +
> > +static void virt_dimm_unplug(HotplugHandler *hotplug_dev,
> > + DeviceState *dev, Error **errp)
> > +{
> > + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> > + PCDIMMDevice *dimm = PC_DIMM(dev);
> > + PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > + MemoryRegion *mr = ddc->get_memory_region(dimm);
> > + Error *local_err = NULL;
> > +
> > + pc_dimm_memory_unplug(dev, &vms->hotplug_memory, mr);
> > + object_unparent(OBJECT(dev));
> > +
> > + error_propagate(errp, local_err);
> > +}
> > +
> > +static void virt_machinedevice_plug_cb(HotplugHandler *hotplug_dev,
> > + DeviceState *dev, Error **errp)
> > +{
> > + if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > + virt_dimm_plug(hotplug_dev, dev, errp);
> > + } else {
> > + error_setg(errp, "acpi: device plug request for not supported device"
>
> "unsupported"
OK. Will fix.
> > + " type: %s", object_get_typename(OBJECT(dev)));
> > + }
> > +}
> > +
> > +static void virt_machinedevice_unplug_cb(HotplugHandler *hotplug_dev,
> > + DeviceState *dev, Error **errp)
> > +{
> > + if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > + virt_dimm_unplug(hotplug_dev, dev, errp);
> > + } else {
> > + error_setg(errp, "acpi: device unplug for not supported device"
>
> "unsupported"
OK. Will fix.
> > + " type: %s", object_get_typename(OBJECT(dev)));
> > + }
> > +}
> > +
> > +static HotplugHandler *virt_get_hotplug_handler(MachineState *machine,
> > + DeviceState *dev)
> > +{
> > + if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > + return HOTPLUG_HANDLER(machine);
> > + }
> > + return NULL;
> > +}
> > +
> > static void virt_machine_class_init(ObjectClass *oc, void *data)
> > {
> > MachineClass *mc = MACHINE_CLASS(oc);
> > + HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
> >
> > mc->init = machvirt_init;
> > /* Start max_cpus at the maximum QEMU supports. We'll further restrict
> > @@ -1462,6 +1565,9 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
> > mc->block_default_type = IF_VIRTIO;
> > mc->no_cdrom = 1;
> > mc->pci_allow_0_address = true;
> > + mc->get_hotplug_handler = virt_get_hotplug_handler;
> > + hc->plug = virt_machinedevice_plug_cb;
> > + hc->unplug = virt_machinedevice_unplug_cb;
> > }
> >
> > static const TypeInfo virt_machine_info = {
> > @@ -1471,6 +1577,10 @@ static const TypeInfo virt_machine_info = {
> > .instance_size = sizeof(VirtMachineState),
> > .class_size = sizeof(VirtMachineClass),
> > .class_init = virt_machine_class_init,
> > + .interfaces = (InterfaceInfo[]) {
> > + { TYPE_HOTPLUG_HANDLER },
> > + { }
> > + },
> > };
> >
> > static void machvirt_machine_init(void)
> > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > index 9650193..35aa3cc 100644
> > --- a/include/hw/arm/virt.h
> > +++ b/include/hw/arm/virt.h
> > @@ -45,6 +45,9 @@
> >
> > #define PPI(irq) ((irq) + 16)
> >
> > +/* 1GB alignment for hotplug memory region */
> > +#define ARCH_VIRT_HOTPLUG_MEM_ALIGN (1ULL << 30)
>
> Where does the 1GB alignment come from? Why do we need 1GB
> alignment for the base but only 1KB alignment for the size?
The alignment value of hotplug_memory.base referred from i386 pc.c and ppc spapr.c.
Could you suggest a proper range for this?
> > +
> > enum {
> > VIRT_FLASH,
> > VIRT_MEM,
> > --
> > 2.5.0
>
> thanks
> -- PMM
Best Regards,
Kwangwoo Lee
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [RFC PATCH 1/3] hw/arm/virt: add hotplug memory support
2016-08-01 0:35 ` kwangwoo.lee
@ 2016-08-01 7:46 ` Igor Mammedov
2016-08-01 8:13 ` Peter Maydell
0 siblings, 1 reply; 22+ messages in thread
From: Igor Mammedov @ 2016-08-01 7:46 UTC (permalink / raw)
To: kwangwoo.lee
Cc: Peter Maydell, Xiao Guangrong, Michael S. Tsirkin, Paolo Bonzini,
Richard Henderson, Eduardo Habkost, Shannon Zhao, Shannon Zhao,
QEMU Developers, qemu-arm, woosuk.chung, hyunchul3.kim
On Mon, 1 Aug 2016 00:35:35 +0000
"kwangwoo.lee@sk.com" <kwangwoo.lee@sk.com> wrote:
> Hi Peter,
>
> Thanks a lot for your comments! I answered in line below.
>
> > -----Original Message-----
> > From: Peter Maydell [mailto:peter.maydell@linaro.org]
> > Sent: Saturday, July 30, 2016 3:10 AM
> > To: 이광우(LEE KWANGWOO) MS SW
> > Cc: Xiao Guangrong; Michael S. Tsirkin; Igor Mammedov; Paolo Bonzini; Richard Henderson; Eduardo
> > Habkost; Shannon Zhao; Shannon Zhao; QEMU Developers; qemu-arm; 정우석(CHUNG WOO SUK) MS SW; 김현철(KIM
> > HYUNCHUL) MS SW
> > Subject: Re: [RFC PATCH 1/3] hw/arm/virt: add hotplug memory support
> >
> > On 20 July 2016 at 01:49, Kwangwoo Lee <kwangwoo.lee@sk.com> wrote:
> > > Add hotplug memory feature on aarch64 virt platfom. This patch is
> > > required to emulate a DIMM type memory like NVDIMM.
> > >
> > > Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com>
> > > ---
> > > default-configs/aarch64-softmmu.mak | 1 +
> > > hw/arm/virt.c | 110 ++++++++++++++++++++++++++++++++++++
> > > include/hw/arm/virt.h | 3 +
> > > 3 files changed, 114 insertions(+)
> > >
> > > diff --git a/default-configs/aarch64-softmmu.mak b/default-configs/aarch64-softmmu.mak
> > > index 2449483..5790cd2 100644
> > > --- a/default-configs/aarch64-softmmu.mak
> > > +++ b/default-configs/aarch64-softmmu.mak
> > > @@ -7,3 +7,4 @@ CONFIG_AUX=y
> > > CONFIG_DDC=y
> > > CONFIG_DPCD=y
> > > CONFIG_XLNX_ZYNQMP=y
> > > +CONFIG_MEM_HOTPLUG=y
> > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > index a193b5a..f7ff411 100644
> > > --- a/hw/arm/virt.c
> > > +++ b/hw/arm/virt.c
> > > @@ -58,6 +58,8 @@
> > > #include "hw/smbios/smbios.h"
> > > #include "qapi/visitor.h"
> > > #include "standard-headers/linux/input.h"
> > > +#include "hw/mem/pc-dimm.h"
> > > +#include "hw/acpi/acpi.h"
> > >
> > > /* Number of external interrupt lines to configure the GIC with */
> > > #define NUM_IRQS 256
> > > @@ -91,6 +93,7 @@ typedef struct {
> > > bool secure;
> > > bool highmem;
> > > int32_t gic_version;
> > > + MemoryHotplugState hotplug_memory;
> > > } VirtMachineState;
> > >
> > > #define TYPE_VIRT_MACHINE MACHINE_TYPE_NAME("virt")
> > > @@ -1376,6 +1379,40 @@ static void machvirt_init(MachineState *machine)
> > > guest_info_state->machine_done.notify = virt_guest_info_machine_done;
> > > qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
> > >
> > > + /* initialize hotplug memory address space */
> > > + if (machine->ram_size < machine->maxram_size) {
> > > + ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
> > > +
> > > + if (machine->ram_slots > ACPI_MAX_RAM_SLOTS) {
> > > + error_report("unsupported amount of memory slots: %"PRIu64,
> >
> > "number of"
>
> OK. I'll fix this.
>
> > > + machine->ram_slots);
> > > + exit(EXIT_FAILURE);
> > > + }
> > > +
> > > + if (QEMU_ALIGN_UP(machine->maxram_size,
> > > + TARGET_PAGE_SIZE) != machine->maxram_size) {
> > > + error_report("maximum memory size must by aligned to multiple of "
> >
> > "must be"
>
> OK. I'll fix this
>
> > > + "%d bytes", TARGET_PAGE_SIZE);
> > > + exit(EXIT_FAILURE);
> > > + }
> > > +
> > > + vms->hotplug_memory.base =
> > > + ROUND_UP(vbi->memmap[VIRT_MEM].base + machine->ram_size,
> > > + ARCH_VIRT_HOTPLUG_MEM_ALIGN);
> > > +
> > > + if ((vms->hotplug_memory.base + hotplug_mem_size) <
> > > + hotplug_mem_size) {
> >
> > This expression is confusing. Is it trying to test for overflow?
> > When can that happen?
>
> Ah.. you are right. No need to check this. I'll remove this. Thanks!
>
> > > + error_report("unsupported amount of maximum memory: " RAM_ADDR_FMT,
> > > + machine->maxram_size);
> > > + exit(EXIT_FAILURE);
> > > + }
> > > +
> > > + memory_region_init(&vms->hotplug_memory.mr, OBJECT(vms),
> > > + "hotplug-memory", hotplug_mem_size);
> > > + memory_region_add_subregion(sysmem, vms->hotplug_memory.base,
> > > + &vms->hotplug_memory.mr);
> > > + }
> > > +
> > > vbi->bootinfo.ram_size = machine->ram_size;
> > > vbi->bootinfo.kernel_filename = machine->kernel_filename;
> > > vbi->bootinfo.kernel_cmdline = machine->kernel_cmdline;
> > > @@ -1448,9 +1485,75 @@ static void virt_set_gic_version(Object *obj, const char *value, Error **errp)
> > > }
> > > }
> > >
> > > +static void virt_dimm_plug(HotplugHandler *hotplug_dev,
> > > + DeviceState *dev, Error **errp)
> > > +{
> > > + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> > > + PCDIMMDevice *dimm = PC_DIMM(dev);
> > > + PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > > + MemoryRegion *mr = ddc->get_memory_region(dimm);
> > > + Error *local_err = NULL;
> > > + uint64_t align = memory_region_get_alignment(mr);
> > > +
> > > + pc_dimm_memory_plug(dev, &vms->hotplug_memory, mr, align, &local_err);
> > > + if (local_err) {
> > > + goto out;
> > > + }
> > > +
> > > +out:
> > > + error_propagate(errp, local_err);
> > > +}
> > > +
> > > +static void virt_dimm_unplug(HotplugHandler *hotplug_dev,
> > > + DeviceState *dev, Error **errp)
> > > +{
> > > + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> > > + PCDIMMDevice *dimm = PC_DIMM(dev);
> > > + PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > > + MemoryRegion *mr = ddc->get_memory_region(dimm);
> > > + Error *local_err = NULL;
> > > +
> > > + pc_dimm_memory_unplug(dev, &vms->hotplug_memory, mr);
> > > + object_unparent(OBJECT(dev));
> > > +
> > > + error_propagate(errp, local_err);
> > > +}
> > > +
> > > +static void virt_machinedevice_plug_cb(HotplugHandler *hotplug_dev,
> > > + DeviceState *dev, Error **errp)
> > > +{
> > > + if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > > + virt_dimm_plug(hotplug_dev, dev, errp);
> > > + } else {
> > > + error_setg(errp, "acpi: device plug request for not supported device"
> >
> > "unsupported"
>
> OK. Will fix.
>
> > > + " type: %s", object_get_typename(OBJECT(dev)));
> > > + }
> > > +}
> > > +
> > > +static void virt_machinedevice_unplug_cb(HotplugHandler *hotplug_dev,
> > > + DeviceState *dev, Error **errp)
> > > +{
> > > + if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > > + virt_dimm_unplug(hotplug_dev, dev, errp);
> > > + } else {
> > > + error_setg(errp, "acpi: device unplug for not supported device"
> >
> > "unsupported"
>
> OK. Will fix.
>
> > > + " type: %s", object_get_typename(OBJECT(dev)));
> > > + }
> > > +}
> > > +
> > > +static HotplugHandler *virt_get_hotplug_handler(MachineState *machine,
> > > + DeviceState *dev)
> > > +{
> > > + if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > > + return HOTPLUG_HANDLER(machine);
> > > + }
> > > + return NULL;
> > > +}
> > > +
> > > static void virt_machine_class_init(ObjectClass *oc, void *data)
> > > {
> > > MachineClass *mc = MACHINE_CLASS(oc);
> > > + HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
> > >
> > > mc->init = machvirt_init;
> > > /* Start max_cpus at the maximum QEMU supports. We'll further restrict
> > > @@ -1462,6 +1565,9 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
> > > mc->block_default_type = IF_VIRTIO;
> > > mc->no_cdrom = 1;
> > > mc->pci_allow_0_address = true;
> > > + mc->get_hotplug_handler = virt_get_hotplug_handler;
> > > + hc->plug = virt_machinedevice_plug_cb;
> > > + hc->unplug = virt_machinedevice_unplug_cb;
> > > }
> > >
> > > static const TypeInfo virt_machine_info = {
> > > @@ -1471,6 +1577,10 @@ static const TypeInfo virt_machine_info = {
> > > .instance_size = sizeof(VirtMachineState),
> > > .class_size = sizeof(VirtMachineClass),
> > > .class_init = virt_machine_class_init,
> > > + .interfaces = (InterfaceInfo[]) {
> > > + { TYPE_HOTPLUG_HANDLER },
> > > + { }
> > > + },
> > > };
> > >
> > > static void machvirt_machine_init(void)
> > > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > > index 9650193..35aa3cc 100644
> > > --- a/include/hw/arm/virt.h
> > > +++ b/include/hw/arm/virt.h
> > > @@ -45,6 +45,9 @@
> > >
> > > #define PPI(irq) ((irq) + 16)
> > >
> > > +/* 1GB alignment for hotplug memory region */
> > > +#define ARCH_VIRT_HOTPLUG_MEM_ALIGN (1ULL << 30)
> >
> > Where does the 1GB alignment come from? Why do we need 1GB
> > alignment for the base but only 1KB alignment for the size?
>
> The alignment value of hotplug_memory.base referred from i386 pc.c and ppc spapr.c.
> Could you suggest a proper range for this?
Base alignment comes from max supported hugepage size, while
size alignment should depend on backend's page size.
>
> > > +
> > > enum {
> > > VIRT_FLASH,
> > > VIRT_MEM,
> > > --
> > > 2.5.0
> >
> > thanks
> > -- PMM
>
> Best Regards,
> Kwangwoo Lee
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [RFC PATCH 1/3] hw/arm/virt: add hotplug memory support
2016-08-01 7:46 ` Igor Mammedov
@ 2016-08-01 8:13 ` Peter Maydell
2016-08-01 9:14 ` Igor Mammedov
0 siblings, 1 reply; 22+ messages in thread
From: Peter Maydell @ 2016-08-01 8:13 UTC (permalink / raw)
To: Igor Mammedov
Cc: kwangwoo.lee, Xiao Guangrong, Michael S. Tsirkin, Paolo Bonzini,
Richard Henderson, Eduardo Habkost, Shannon Zhao, Shannon Zhao,
QEMU Developers, qemu-arm, woosuk.chung, hyunchul3.kim
On 1 August 2016 at 08:46, Igor Mammedov <imammedo@redhat.com> wrote:
> On Mon, 1 Aug 2016 00:35:35 +0000
> "kwangwoo.lee@sk.com" <kwangwoo.lee@sk.com> wrote:
>> > > +/* 1GB alignment for hotplug memory region */
>> > > +#define ARCH_VIRT_HOTPLUG_MEM_ALIGN (1ULL << 30)
>> >
>> > Where does the 1GB alignment come from? Why do we need 1GB
>> > alignment for the base but only 1KB alignment for the size?
>>
>> The alignment value of hotplug_memory.base referred from i386 pc.c and ppc spapr.c.
>> Could you suggest a proper range for this?
> Base alignment comes from max supported hugepage size,
Max hugepage size for any host? (if so, should be defined
in a common header somewhere)
Max hugepage size for ARM hosts? (if so, why is TCG
different from KVM?, and should still be in a common
header somewhere)
> while
> size alignment should depend on backend's page size
Which page size do you have in mind here? TARGET_PAGE_SIZE
is often not the right answer, since it doesn't
correspond either to the actual page size being used
by the host kernel or to the actual page size used
by the guest kernel...
thanks
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 1/3] hw/arm/virt: add hotplug memory support
2016-08-01 8:13 ` Peter Maydell
@ 2016-08-01 9:14 ` Igor Mammedov
2016-08-02 5:54 ` kwangwoo.lee
2016-08-02 7:59 ` Peter Maydell
0 siblings, 2 replies; 22+ messages in thread
From: Igor Mammedov @ 2016-08-01 9:14 UTC (permalink / raw)
To: Peter Maydell
Cc: kwangwoo.lee, Xiao Guangrong, Michael S. Tsirkin, Paolo Bonzini,
Richard Henderson, Eduardo Habkost, Shannon Zhao, Shannon Zhao,
QEMU Developers, qemu-arm, woosuk.chung, hyunchul3.kim
On Mon, 1 Aug 2016 09:13:34 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 August 2016 at 08:46, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Mon, 1 Aug 2016 00:35:35 +0000
> > "kwangwoo.lee@sk.com" <kwangwoo.lee@sk.com> wrote:
>
> >> > > +/* 1GB alignment for hotplug memory region */
> >> > > +#define ARCH_VIRT_HOTPLUG_MEM_ALIGN (1ULL << 30)
> >> >
> >> > Where does the 1GB alignment come from? Why do we need 1GB
> >> > alignment for the base but only 1KB alignment for the size?
> >>
> >> The alignment value of hotplug_memory.base referred from i386 pc.c and ppc spapr.c.
> >> Could you suggest a proper range for this?
>
> > Base alignment comes from max supported hugepage size,
>
> Max hugepage size for any host? (if so, should be defined
> in a common header somewhere)
> Max hugepage size for ARM hosts? (if so, why is TCG
> different from KVM?, and should still be in a common
> header somewhere)
It's the same for TCG but it probably doesn't matter much there,
main usage is to provide better performance with KVM.
So I'd say it's host depended (for x86 it's 1Gb),
probably other values for ARM and PPC
>
> > while
> > size alignment should depend on backend's page size
>
> Which page size do you have in mind here? TARGET_PAGE_SIZE
> is often not the right answer, since it doesn't
> correspond either to the actual page size being used
> by the host kernel or to the actual page size used
> by the guest kernel...
alignment comes from here: memory_region_get_alignment()
exec:c
MAX(page_size, QEMU_VMALLOC_ALIGN)
so it's either backend's page size or a min chunk QEMU
allocates memory to make KVM/valgrind/whatnot happy.
>
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 1/3] hw/arm/virt: add hotplug memory support
2016-08-01 9:14 ` Igor Mammedov
@ 2016-08-02 5:54 ` kwangwoo.lee
2016-08-02 7:59 ` Peter Maydell
1 sibling, 0 replies; 22+ messages in thread
From: kwangwoo.lee @ 2016-08-02 5:54 UTC (permalink / raw)
To: Igor Mammedov, Peter Maydell
Cc: Xiao Guangrong, Michael S. Tsirkin, Paolo Bonzini,
Richard Henderson, Eduardo Habkost, Shannon Zhao, Shannon Zhao,
QEMU Developers, qemu-arm, woosuk.chung, hyunchul3.kim
Hi,
Thank you for the alignment information and discussion, Igor and Peter!
I'll try to find out proper alignment by looking at the codes.
Best Regards,
Kwangwoo Lee
> -----Original Message-----
> From: Igor Mammedov [mailto:imammedo@redhat.com]
> Sent: Monday, August 01, 2016 6:15 PM
> To: Peter Maydell
> Cc: 이광우(LEE KWANGWOO) MS SW; Xiao Guangrong; Michael S. Tsirkin; Paolo Bonzini; Richard Henderson;
> Eduardo Habkost; Shannon Zhao; Shannon Zhao; QEMU Developers; qemu-arm; 정우석(CHUNG WOO SUK) MS SW; 김
> 현철(KIM HYUNCHUL) MS SW
> Subject: Re: [RFC PATCH 1/3] hw/arm/virt: add hotplug memory support
>
> On Mon, 1 Aug 2016 09:13:34 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
>
> > On 1 August 2016 at 08:46, Igor Mammedov <imammedo@redhat.com> wrote:
> > > On Mon, 1 Aug 2016 00:35:35 +0000
> > > "kwangwoo.lee@sk.com" <kwangwoo.lee@sk.com> wrote:
> >
> > >> > > +/* 1GB alignment for hotplug memory region */
> > >> > > +#define ARCH_VIRT_HOTPLUG_MEM_ALIGN (1ULL << 30)
> > >> >
> > >> > Where does the 1GB alignment come from? Why do we need 1GB
> > >> > alignment for the base but only 1KB alignment for the size?
> > >>
> > >> The alignment value of hotplug_memory.base referred from i386 pc.c and ppc spapr.c.
> > >> Could you suggest a proper range for this?
> >
> > > Base alignment comes from max supported hugepage size,
> >
> > Max hugepage size for any host? (if so, should be defined
> > in a common header somewhere)
> > Max hugepage size for ARM hosts? (if so, why is TCG
> > different from KVM?, and should still be in a common
> > header somewhere)
> It's the same for TCG but it probably doesn't matter much there,
> main usage is to provide better performance with KVM.
>
> So I'd say it's host depended (for x86 it's 1Gb),
> probably other values for ARM and PPC
>
> >
> > > while
> > > size alignment should depend on backend's page size
> >
> > Which page size do you have in mind here? TARGET_PAGE_SIZE
> > is often not the right answer, since it doesn't
> > correspond either to the actual page size being used
> > by the host kernel or to the actual page size used
> > by the guest kernel...
> alignment comes from here: memory_region_get_alignment()
>
> exec:c
> MAX(page_size, QEMU_VMALLOC_ALIGN)
> so it's either backend's page size or a min chunk QEMU
> allocates memory to make KVM/valgrind/whatnot happy.
>
> >
> > thanks
> > -- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 1/3] hw/arm/virt: add hotplug memory support
2016-08-01 9:14 ` Igor Mammedov
2016-08-02 5:54 ` kwangwoo.lee
@ 2016-08-02 7:59 ` Peter Maydell
2016-08-02 12:18 ` Igor Mammedov
1 sibling, 1 reply; 22+ messages in thread
From: Peter Maydell @ 2016-08-02 7:59 UTC (permalink / raw)
To: Igor Mammedov
Cc: kwangwoo.lee, Xiao Guangrong, Michael S. Tsirkin, Paolo Bonzini,
Richard Henderson, Eduardo Habkost, Shannon Zhao, Shannon Zhao,
QEMU Developers, qemu-arm, woosuk.chung, hyunchul3.kim
On 1 August 2016 at 10:14, Igor Mammedov <imammedo@redhat.com> wrote:
> On Mon, 1 Aug 2016 09:13:34 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 1 August 2016 at 08:46, Igor Mammedov <imammedo@redhat.com> wrote:
>> > Base alignment comes from max supported hugepage size,
>>
>> Max hugepage size for any host? (if so, should be defined
>> in a common header somewhere)
>> Max hugepage size for ARM hosts? (if so, why is TCG
>> different from KVM?, and should still be in a common
>> header somewhere)
> It's the same for TCG but it probably doesn't matter much there,
> main usage is to provide better performance with KVM.
>
> So I'd say it's host depended (for x86 it's 1Gb),
> probably other values for ARM and PPC
We probably don't want to make the memory layout depend
on the host architecture, though :-(
>>
>> > while
>> > size alignment should depend on backend's page size
>>
>> Which page size do you have in mind here? TARGET_PAGE_SIZE
>> is often not the right answer, since it doesn't
>> correspond either to the actual page size being used
>> by the host kernel or to the actual page size used
>> by the guest kernel...
> alignment comes from here: memory_region_get_alignment()
>
> exec:c
> MAX(page_size, QEMU_VMALLOC_ALIGN)
> so it's either backend's page size or a min chunk QEMU
> allocates memory to make KVM/valgrind/whatnot happy.
Since that's always larger than TARGET_PAGE_SIZE
why are we checking for an alignment here that's
not actually sufficient to make things work?
thanks
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 1/3] hw/arm/virt: add hotplug memory support
2016-08-02 7:59 ` Peter Maydell
@ 2016-08-02 12:18 ` Igor Mammedov
2016-08-04 23:30 ` kwangwoo.lee
0 siblings, 1 reply; 22+ messages in thread
From: Igor Mammedov @ 2016-08-02 12:18 UTC (permalink / raw)
To: Peter Maydell
Cc: Xiao Guangrong, Eduardo Habkost, Michael S. Tsirkin,
QEMU Developers, woosuk.chung, qemu-arm, Shannon Zhao,
Shannon Zhao, Paolo Bonzini, hyunchul3.kim, kwangwoo.lee,
Richard Henderson
On Tue, 2 Aug 2016 08:59:46 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 August 2016 at 10:14, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Mon, 1 Aug 2016 09:13:34 +0100
> > Peter Maydell <peter.maydell@linaro.org> wrote:
> >> On 1 August 2016 at 08:46, Igor Mammedov <imammedo@redhat.com> wrote:
> >> > Base alignment comes from max supported hugepage size,
> >>
> >> Max hugepage size for any host? (if so, should be defined
> >> in a common header somewhere)
> >> Max hugepage size for ARM hosts? (if so, why is TCG
> >> different from KVM?, and should still be in a common
> >> header somewhere)
> > It's the same for TCG but it probably doesn't matter much there,
> > main usage is to provide better performance with KVM.
> >
> > So I'd say it's host depended (for x86 it's 1Gb),
> > probably other values for ARM and PPC
>
> We probably don't want to make the memory layout depend
> on the host architecture, though :-(
Important here is not change it dynamically so it won't
break migration.
Otherwise it could be a value that makes a sense for
the use-case where performance matters most, i.e. KVM
which makes us to derive value from max supported page
size for a KVM host (meaning guest is the same arch)
In case of x86 value is constant hardcoded in target
specific code which applies to both KVM and TCG.
Perhaps there is a better way to handle it which I just
don't see.
>
> >>
> >> > while
> >> > size alignment should depend on backend's page size
> >>
> >> Which page size do you have in mind here? TARGET_PAGE_SIZE
> >> is often not the right answer, since it doesn't
> >> correspond either to the actual page size being used
> >> by the host kernel or to the actual page size used
> >> by the guest kernel...
> > alignment comes from here: memory_region_get_alignment()
> >
> > exec:c
> > MAX(page_size, QEMU_VMALLOC_ALIGN)
> > so it's either backend's page size or a min chunk QEMU
> > allocates memory to make KVM/valgrind/whatnot happy.
>
> Since that's always larger than TARGET_PAGE_SIZE
> why are we checking for an alignment here that's
> not actually sufficient to make things work?
You mean following hunk:
+ if (QEMU_ALIGN_UP(machine->maxram_size,
+ TARGET_PAGE_SIZE) != machine->maxram_size) {
It's a bit late to fix for x86 without breaking CLI,
side effect would be inability to hotplug upto maxmem if maxmem
is misaligned wrt used backend page size
ARCH_VIRT_HOTPLUG_MEM_ALIGN should be used instead of TARGET_PAGE_SIZE
PS:
I haven't reviewed series yet, but I'd split this patch in 3
to make review easier
1st - introduce machine level hotplug hooks
2nd - add MemoryHotplugState to VirtMachineState and initialize it
3rd - add virt_dimm_plug() handler
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [RFC PATCH 1/3] hw/arm/virt: add hotplug memory support
2016-08-02 12:18 ` Igor Mammedov
@ 2016-08-04 23:30 ` kwangwoo.lee
0 siblings, 0 replies; 22+ messages in thread
From: kwangwoo.lee @ 2016-08-04 23:30 UTC (permalink / raw)
To: Igor Mammedov, Peter Maydell
Cc: Xiao Guangrong, Eduardo Habkost, Michael S. Tsirkin,
QEMU Developers, woosuk.chung, qemu-arm, Shannon Zhao,
Shannon Zhao, Paolo Bonzini, hyunchul3.kim, Richard Henderson
Hi Igor,
Thank you for your guide to split the hotplug patch.
Currently I'm looking at the Linux kernel codes related with huge page size.
> -----Original Message-----
> From: Igor Mammedov [mailto:imammedo@redhat.com]
> Sent: Tuesday, August 02, 2016 9:18 PM
> To: Peter Maydell
> Cc: Xiao Guangrong; Eduardo Habkost; Michael S. Tsirkin; QEMU Developers; 정우석(CHUNG WOO SUK) MS SW;
> qemu-arm; Shannon Zhao; Shannon Zhao; Paolo Bonzini; 김현철(KIM HYUNCHUL) MS SW; 이광우(LEE KWANGWOO) MS
> SW; Richard Henderson
> Subject: Re: [Qemu-devel] [RFC PATCH 1/3] hw/arm/virt: add hotplug memory support
>
> On Tue, 2 Aug 2016 08:59:46 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
>
> > On 1 August 2016 at 10:14, Igor Mammedov <imammedo@redhat.com> wrote:
> > > On Mon, 1 Aug 2016 09:13:34 +0100
> > > Peter Maydell <peter.maydell@linaro.org> wrote:
> > >> On 1 August 2016 at 08:46, Igor Mammedov <imammedo@redhat.com> wrote:
> > >> > Base alignment comes from max supported hugepage size,
> > >>
> > >> Max hugepage size for any host? (if so, should be defined
> > >> in a common header somewhere)
> > >> Max hugepage size for ARM hosts? (if so, why is TCG
> > >> different from KVM?, and should still be in a common
> > >> header somewhere)
> > > It's the same for TCG but it probably doesn't matter much there,
> > > main usage is to provide better performance with KVM.
> > >
> > > So I'd say it's host depended (for x86 it's 1Gb),
> > > probably other values for ARM and PPC
> >
> > We probably don't want to make the memory layout depend
> > on the host architecture, though :-(
> Important here is not change it dynamically so it won't
> break migration.
> Otherwise it could be a value that makes a sense for
> the use-case where performance matters most, i.e. KVM
> which makes us to derive value from max supported page
> size for a KVM host (meaning guest is the same arch)
>
> In case of x86 value is constant hardcoded in target
> specific code which applies to both KVM and TCG.
>
> Perhaps there is a better way to handle it which I just
> don't see.
>
> >
> > >>
> > >> > while
> > >> > size alignment should depend on backend's page size
> > >>
> > >> Which page size do you have in mind here? TARGET_PAGE_SIZE
> > >> is often not the right answer, since it doesn't
> > >> correspond either to the actual page size being used
> > >> by the host kernel or to the actual page size used
> > >> by the guest kernel...
> > > alignment comes from here: memory_region_get_alignment()
> > >
> > > exec:c
> > > MAX(page_size, QEMU_VMALLOC_ALIGN)
> > > so it's either backend's page size or a min chunk QEMU
> > > allocates memory to make KVM/valgrind/whatnot happy.
> >
> > Since that's always larger than TARGET_PAGE_SIZE
> > why are we checking for an alignment here that's
> > not actually sufficient to make things work?
> You mean following hunk:
>
> + if (QEMU_ALIGN_UP(machine->maxram_size,
> + TARGET_PAGE_SIZE) != machine->maxram_size) {
>
> It's a bit late to fix for x86 without breaking CLI,
> side effect would be inability to hotplug upto maxmem if maxmem
> is misaligned wrt used backend page size
>
> ARCH_VIRT_HOTPLUG_MEM_ALIGN should be used instead of TARGET_PAGE_SIZE
>
> PS:
> I haven't reviewed series yet, but I'd split this patch in 3
> to make review easier
> 1st - introduce machine level hotplug hooks
> 2nd - add MemoryHotplugState to VirtMachineState and initialize it
> 3rd - add virt_dimm_plug() handler
OK. I'll try to split it.
> >
> > thanks
> > -- PMM
> >
Best Regards,
Kwangwoo Lee
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [RFC PATCH 2/3] nvdimm: use configurable ACPI IO base and size
2016-07-20 0:49 [Qemu-devel] [RFC PATCH 0/3] add nvdimm support on AArch64 virt platform Kwangwoo Lee
2016-07-20 0:49 ` [Qemu-devel] [RFC PATCH 1/3] hw/arm/virt: add hotplug memory support Kwangwoo Lee
@ 2016-07-20 0:49 ` Kwangwoo Lee
2016-07-25 16:12 ` Peter Maydell
2016-07-20 0:49 ` [Qemu-devel] [RFC PATCH 3/3] hw/arm/virt: add nvdimm emulation support Kwangwoo Lee
2016-07-25 16:06 ` [Qemu-devel] [RFC PATCH 0/3] add nvdimm support on AArch64 virt platform Peter Maydell
3 siblings, 1 reply; 22+ messages in thread
From: Kwangwoo Lee @ 2016-07-20 0:49 UTC (permalink / raw)
To: Xiao Guangrong, Peter Maydell, Michael S. Tsirkin, Igor Mammedov,
Paolo Bonzini, Richard Henderson, Eduardo Habkost, Shannon Zhao,
Shannon Zhao, qemu-devel, qemu-arm
Cc: Kwangwoo Lee, Woosuk Chung, Hyunchul Kim
This patch uses configurable IO base and size 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>
---
hw/acpi/nvdimm.c | 23 +++++++++++++++--------
hw/i386/acpi-build.c | 2 +-
hw/i386/pc_piix.c | 8 +++++++-
hw/i386/pc_q35.c | 8 +++++++-
include/hw/mem/nvdimm.h | 17 ++++++++++++++++-
5 files changed, 46 insertions(+), 12 deletions(-)
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index e486128..57e03ee 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -765,8 +765,8 @@ 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);
+ "nvdimm-acpi-io", state->dsm_io.size);
+ memory_region_add_subregion(io, state->dsm_io.base, &state->io_mr);
state->dsm_mem = g_array_new(false, true /* clear */, 1);
acpi_data_push(state->dsm_mem, sizeof(NvdimmDsmIn));
@@ -912,9 +912,10 @@ 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, BIOSLinker *linker,
- GArray *dsm_dma_arrea)
+ AcpiNVDIMMState *acpi_nvdimm_state)
{
Aml *ssdt, *sb_scope, *dev, *field;
+ AmlRegionSpace rs;
int mem_addr_offset, nvdimm_ssdt;
acpi_add_table(table_offsets, table_data);
@@ -940,8 +941,14 @@ 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));
+ if (acpi_nvdimm_state->dsm_io.type == NVDIMM_ACPI_IO_PORT) {
+ rs = AML_SYSTEM_IO;
+ } else {
+ rs = AML_SYSTEM_MEMORY;
+ }
+ aml_append(dev, aml_operation_region("NPIO", rs,
+ aml_int(acpi_nvdimm_state->dsm_io.base),
+ acpi_nvdimm_state->dsm_io.size));
aml_append(dev, aml_operation_region("NRAM", AML_SYSTEM_MEMORY,
aml_name(NVDIMM_ACPI_MEM_ADDR), sizeof(NvdimmDsmIn)));
@@ -1014,7 +1021,7 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
NVDIMM_ACPI_MEM_ADDR);
bios_linker_loader_alloc(linker,
- NVDIMM_DSM_MEM_FILE, dsm_dma_arrea,
+ NVDIMM_DSM_MEM_FILE, acpi_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),
@@ -1026,7 +1033,7 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
}
void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
- BIOSLinker *linker, GArray *dsm_dma_arrea)
+ BIOSLinker *linker, AcpiNVDIMMState *acpi_nvdimm_state)
{
GSList *device_list;
@@ -1037,6 +1044,6 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
}
nvdimm_build_nfit(device_list, table_offsets, table_data, linker);
nvdimm_build_ssdt(device_list, table_offsets, table_data, linker,
- dsm_dma_arrea);
+ acpi_nvdimm_state);
g_slist_free(device_list);
}
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index fbba461..54b09a9 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2632,7 +2632,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
}
if (pcms->acpi_nvdimm_state.is_enabled) {
nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
- pcms->acpi_nvdimm_state.dsm_mem);
+ &pcms->acpi_nvdimm_state);
}
/* Add tables supplied by user (if any) */
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index a07dc81..b624f59 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -298,7 +298,13 @@ static void pc_init1(MachineState *machine,
}
if (pcms->acpi_nvdimm_state.is_enabled) {
- nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,
+ AcpiNVDIMMState *acpi_nvdimm_state = &pcms->acpi_nvdimm_state;
+
+ acpi_nvdimm_state->dsm_io.type = NVDIMM_ACPI_IO_PORT;
+ acpi_nvdimm_state->dsm_io.base = NVDIMM_ACPI_IO_BASE;
+ acpi_nvdimm_state->dsm_io.size = NVDIMM_ACPI_IO_LEN;
+
+ nvdimm_init_acpi_state(acpi_nvdimm_state, system_io,
pcms->fw_cfg, OBJECT(pcms));
}
}
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index c0b9961..779ac32 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -263,7 +263,13 @@ static void pc_q35_init(MachineState *machine)
}
if (pcms->acpi_nvdimm_state.is_enabled) {
- nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,
+ AcpiNVDIMMState *acpi_nvdimm_state = &pcms->acpi_nvdimm_state;
+
+ acpi_nvdimm_state->dsm_io.type = NVDIMM_ACPI_IO_PORT;
+ acpi_nvdimm_state->dsm_io.base = NVDIMM_ACPI_IO_BASE;
+ acpi_nvdimm_state->dsm_io.size = NVDIMM_ACPI_IO_LEN;
+
+ nvdimm_init_acpi_state(acpi_nvdimm_state, system_io,
pcms->fw_cfg, OBJECT(pcms));
}
}
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index 1cfe9e0..8c917c3 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -98,10 +98,25 @@ typedef struct NVDIMMClass NVDIMMClass;
#define NVDIMM_ACPI_IO_BASE 0x0a18
#define NVDIMM_ACPI_IO_LEN 4
+typedef enum {
+ NVDIMM_ACPI_IO_PORT = 0X00,
+ NVDIMM_ACPI_IO_MEMORY = 0X01,
+} AcpiNVDIMMIOType;
+
+struct AcpiNVDIMMIOEntry {
+ AcpiNVDIMMIOType type;
+ hwaddr base;
+ hwaddr size;
+};
+typedef struct AcpiNVDIMMIOEntry AcpiNVDIMMIOEntry;
+
struct AcpiNVDIMMState {
/* detect if NVDIMM support is enabled. */
bool is_enabled;
+ /* NVDIMM IO Type, Address, and Size */
+ AcpiNVDIMMIOEntry dsm_io;
+
/* the data of the fw_cfg file NVDIMM_DSM_MEM_FILE. */
GArray *dsm_mem;
/* the IO region used by OSPM to transfer control to QEMU. */
@@ -112,5 +127,5 @@ 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,
- BIOSLinker *linker, GArray *dsm_dma_arrea);
+ BIOSLinker *linker, AcpiNVDIMMState *acpi_nvdimm_state);
#endif
--
2.5.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [RFC PATCH 2/3] nvdimm: use configurable ACPI IO base and size
2016-07-20 0:49 ` [Qemu-devel] [RFC PATCH 2/3] nvdimm: use configurable ACPI IO base and size Kwangwoo Lee
@ 2016-07-25 16:12 ` Peter Maydell
2016-07-26 6:55 ` kwangwoo.lee
0 siblings, 1 reply; 22+ messages in thread
From: Peter Maydell @ 2016-07-25 16:12 UTC (permalink / raw)
To: Kwangwoo Lee
Cc: Xiao Guangrong, Michael S. Tsirkin, Igor Mammedov, Paolo Bonzini,
Richard Henderson, Eduardo Habkost, Shannon Zhao, Shannon Zhao,
QEMU Developers, qemu-arm, Woosuk Chung, Hyunchul Kim
On 20 July 2016 at 01:49, Kwangwoo Lee <kwangwoo.lee@sk.com> wrote:
> This patch uses configurable IO base and size 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>
> ---
> hw/acpi/nvdimm.c | 23 +++++++++++++++--------
> hw/i386/acpi-build.c | 2 +-
> hw/i386/pc_piix.c | 8 +++++++-
> hw/i386/pc_q35.c | 8 +++++++-
> include/hw/mem/nvdimm.h | 17 ++++++++++++++++-
> 5 files changed, 46 insertions(+), 12 deletions(-)
>
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index e486128..57e03ee 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -765,8 +765,8 @@ 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);
> + "nvdimm-acpi-io", state->dsm_io.size);
> + memory_region_add_subregion(io, state->dsm_io.base, &state->io_mr);
>
> state->dsm_mem = g_array_new(false, true /* clear */, 1);
> acpi_data_push(state->dsm_mem, sizeof(NvdimmDsmIn));
Why does this function take a MemoryRegion to insert itself into,
rather than returning a MemoryRegion for the caller to map into
wherever is appropriate, or even being a DeviceState which has
mappable memory regions via the sysbus API ?
I guess the answer is "that's the way it happens to be at the moment",
so I'm not really asking for a change here necessarily.
> @@ -912,9 +912,10 @@ 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, BIOSLinker *linker,
> - GArray *dsm_dma_arrea)
> + AcpiNVDIMMState *acpi_nvdimm_state)
> {
> Aml *ssdt, *sb_scope, *dev, *field;
> + AmlRegionSpace rs;
> int mem_addr_offset, nvdimm_ssdt;
>
> acpi_add_table(table_offsets, table_data);
> @@ -940,8 +941,14 @@ 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));
> + if (acpi_nvdimm_state->dsm_io.type == NVDIMM_ACPI_IO_PORT) {
> + rs = AML_SYSTEM_IO;
> + } else {
> + rs = AML_SYSTEM_MEMORY;
> + }
> + aml_append(dev, aml_operation_region("NPIO", rs,
> + aml_int(acpi_nvdimm_state->dsm_io.base),
> + acpi_nvdimm_state->dsm_io.size));
> aml_append(dev, aml_operation_region("NRAM", AML_SYSTEM_MEMORY,
> aml_name(NVDIMM_ACPI_MEM_ADDR), sizeof(NvdimmDsmIn)));
>
> @@ -1014,7 +1021,7 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
> NVDIMM_ACPI_MEM_ADDR);
>
> bios_linker_loader_alloc(linker,
> - NVDIMM_DSM_MEM_FILE, dsm_dma_arrea,
> + NVDIMM_DSM_MEM_FILE, acpi_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),
> @@ -1026,7 +1033,7 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
> }
>
> void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
> - BIOSLinker *linker, GArray *dsm_dma_arrea)
> + BIOSLinker *linker, AcpiNVDIMMState *acpi_nvdimm_state)
> {
> GSList *device_list;
>
> @@ -1037,6 +1044,6 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
> }
> nvdimm_build_nfit(device_list, table_offsets, table_data, linker);
> nvdimm_build_ssdt(device_list, table_offsets, table_data, linker,
> - dsm_dma_arrea);
> + acpi_nvdimm_state);
> g_slist_free(device_list);
> }
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index fbba461..54b09a9 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2632,7 +2632,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> }
> if (pcms->acpi_nvdimm_state.is_enabled) {
> nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
> - pcms->acpi_nvdimm_state.dsm_mem);
> + &pcms->acpi_nvdimm_state);
> }
>
> /* Add tables supplied by user (if any) */
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index a07dc81..b624f59 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -298,7 +298,13 @@ static void pc_init1(MachineState *machine,
> }
>
> if (pcms->acpi_nvdimm_state.is_enabled) {
> - nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,
> + AcpiNVDIMMState *acpi_nvdimm_state = &pcms->acpi_nvdimm_state;
> +
> + acpi_nvdimm_state->dsm_io.type = NVDIMM_ACPI_IO_PORT;
> + acpi_nvdimm_state->dsm_io.base = NVDIMM_ACPI_IO_BASE;
> + acpi_nvdimm_state->dsm_io.size = NVDIMM_ACPI_IO_LEN;
> +
> + nvdimm_init_acpi_state(acpi_nvdimm_state, system_io,
> pcms->fw_cfg, OBJECT(pcms));
> }
> }
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index c0b9961..779ac32 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -263,7 +263,13 @@ static void pc_q35_init(MachineState *machine)
> }
>
> if (pcms->acpi_nvdimm_state.is_enabled) {
> - nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,
> + AcpiNVDIMMState *acpi_nvdimm_state = &pcms->acpi_nvdimm_state;
> +
> + acpi_nvdimm_state->dsm_io.type = NVDIMM_ACPI_IO_PORT;
> + acpi_nvdimm_state->dsm_io.base = NVDIMM_ACPI_IO_BASE;
> + acpi_nvdimm_state->dsm_io.size = NVDIMM_ACPI_IO_LEN;
> +
> + nvdimm_init_acpi_state(acpi_nvdimm_state, system_io,
> pcms->fw_cfg, OBJECT(pcms));
> }
> }
Ideally this would be a QOM object with QOM properties, rather
than an ad-hoc init function.
thanks
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [RFC PATCH 2/3] nvdimm: use configurable ACPI IO base and size
2016-07-25 16:12 ` Peter Maydell
@ 2016-07-26 6:55 ` kwangwoo.lee
0 siblings, 0 replies; 22+ messages in thread
From: kwangwoo.lee @ 2016-07-26 6:55 UTC (permalink / raw)
To: Peter Maydell
Cc: Xiao Guangrong, Michael S. Tsirkin, Igor Mammedov, Paolo Bonzini,
Richard Henderson, Eduardo Habkost, Shannon Zhao, Shannon Zhao,
QEMU Developers, qemu-arm, woosuk.chung, hyunchul3.kim
Hi Peter,
Thanks for reviewing the patches! I added some comments below.
> -----Original Message-----
> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Tuesday, July 26, 2016 1:13 AM
> To: 이광우(LEE KWANGWOO) MS SW
> Cc: Xiao Guangrong; Michael S. Tsirkin; Igor Mammedov; Paolo Bonzini; Richard Henderson; Eduardo
> Habkost; Shannon Zhao; Shannon Zhao; QEMU Developers; qemu-arm; 정우석(CHUNG WOO SUK) MS SW; 김현철(KIM
> HYUNCHUL) MS SW
> Subject: Re: [RFC PATCH 2/3] nvdimm: use configurable ACPI IO base and size
>
> On 20 July 2016 at 01:49, Kwangwoo Lee <kwangwoo.lee@sk.com> wrote:
> > This patch uses configurable IO base and size 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>
> > ---
> > hw/acpi/nvdimm.c | 23 +++++++++++++++--------
> > hw/i386/acpi-build.c | 2 +-
> > hw/i386/pc_piix.c | 8 +++++++-
> > hw/i386/pc_q35.c | 8 +++++++-
> > include/hw/mem/nvdimm.h | 17 ++++++++++++++++-
> > 5 files changed, 46 insertions(+), 12 deletions(-)
> >
> > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> > index e486128..57e03ee 100644
> > --- a/hw/acpi/nvdimm.c
> > +++ b/hw/acpi/nvdimm.c
> > @@ -765,8 +765,8 @@ 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);
> > + "nvdimm-acpi-io", state->dsm_io.size);
> > + memory_region_add_subregion(io, state->dsm_io.base, &state->io_mr);
> >
> > state->dsm_mem = g_array_new(false, true /* clear */, 1);
> > acpi_data_push(state->dsm_mem, sizeof(NvdimmDsmIn));
>
> Why does this function take a MemoryRegion to insert itself into,
> rather than returning a MemoryRegion for the caller to map into
> wherever is appropriate, or even being a DeviceState which has
> mappable memory regions via the sysbus API ?
On i386, the region is added to system_io which is ported-mapped IO at 0x0a18.
Therefore, to use memory-mapped IO for NVDIMM DSM(Device Specific Method), a
member variable - dsm_io - which containing type, base address and size need to
be added on ARM64 platform.
> I guess the answer is "that's the way it happens to be at the moment",
> so I'm not really asking for a change here necessarily.
Thanks for your understanding!
> > @@ -912,9 +912,10 @@ 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, BIOSLinker *linker,
> > - GArray *dsm_dma_arrea)
> > + AcpiNVDIMMState *acpi_nvdimm_state)
> > {
> > Aml *ssdt, *sb_scope, *dev, *field;
> > + AmlRegionSpace rs;
> > int mem_addr_offset, nvdimm_ssdt;
> >
> > acpi_add_table(table_offsets, table_data);
> > @@ -940,8 +941,14 @@ 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));
> > + if (acpi_nvdimm_state->dsm_io.type == NVDIMM_ACPI_IO_PORT) {
> > + rs = AML_SYSTEM_IO;
> > + } else {
> > + rs = AML_SYSTEM_MEMORY;
> > + }
> > + aml_append(dev, aml_operation_region("NPIO", rs,
> > + aml_int(acpi_nvdimm_state->dsm_io.base),
> > + acpi_nvdimm_state->dsm_io.size));
> > aml_append(dev, aml_operation_region("NRAM", AML_SYSTEM_MEMORY,
> > aml_name(NVDIMM_ACPI_MEM_ADDR), sizeof(NvdimmDsmIn)));
> >
> > @@ -1014,7 +1021,7 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
> > NVDIMM_ACPI_MEM_ADDR);
> >
> > bios_linker_loader_alloc(linker,
> > - NVDIMM_DSM_MEM_FILE, dsm_dma_arrea,
> > + NVDIMM_DSM_MEM_FILE, acpi_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),
> > @@ -1026,7 +1033,7 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
> > }
> >
> > void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
> > - BIOSLinker *linker, GArray *dsm_dma_arrea)
> > + BIOSLinker *linker, AcpiNVDIMMState *acpi_nvdimm_state)
> > {
> > GSList *device_list;
> >
> > @@ -1037,6 +1044,6 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
> > }
> > nvdimm_build_nfit(device_list, table_offsets, table_data, linker);
> > nvdimm_build_ssdt(device_list, table_offsets, table_data, linker,
> > - dsm_dma_arrea);
> > + acpi_nvdimm_state);
> > g_slist_free(device_list);
> > }
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index fbba461..54b09a9 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -2632,7 +2632,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> > }
> > if (pcms->acpi_nvdimm_state.is_enabled) {
> > nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
> > - pcms->acpi_nvdimm_state.dsm_mem);
> > + &pcms->acpi_nvdimm_state);
> > }
> >
> > /* Add tables supplied by user (if any) */
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index a07dc81..b624f59 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -298,7 +298,13 @@ static void pc_init1(MachineState *machine,
> > }
> >
> > if (pcms->acpi_nvdimm_state.is_enabled) {
> > - nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,
> > + AcpiNVDIMMState *acpi_nvdimm_state = &pcms->acpi_nvdimm_state;
> > +
> > + acpi_nvdimm_state->dsm_io.type = NVDIMM_ACPI_IO_PORT;
> > + acpi_nvdimm_state->dsm_io.base = NVDIMM_ACPI_IO_BASE;
> > + acpi_nvdimm_state->dsm_io.size = NVDIMM_ACPI_IO_LEN;
> > +
> > + nvdimm_init_acpi_state(acpi_nvdimm_state, system_io,
> > pcms->fw_cfg, OBJECT(pcms));
> > }
> > }
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index c0b9961..779ac32 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -263,7 +263,13 @@ static void pc_q35_init(MachineState *machine)
> > }
> >
> > if (pcms->acpi_nvdimm_state.is_enabled) {
> > - nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,
> > + AcpiNVDIMMState *acpi_nvdimm_state = &pcms->acpi_nvdimm_state;
> > +
> > + acpi_nvdimm_state->dsm_io.type = NVDIMM_ACPI_IO_PORT;
> > + acpi_nvdimm_state->dsm_io.base = NVDIMM_ACPI_IO_BASE;
> > + acpi_nvdimm_state->dsm_io.size = NVDIMM_ACPI_IO_LEN;
> > +
> > + nvdimm_init_acpi_state(acpi_nvdimm_state, system_io,
> > pcms->fw_cfg, OBJECT(pcms));
> > }
> > }
>
> Ideally this would be a QOM object with QOM properties, rather
> than an ad-hoc init function.
On i386, NVDIMM_ACPI_IO_BASE is fixed with 0x0a18. Should it be changed, too?
I'm not sure that I need to change it and can affect the i386 platform..
> thanks
> -- PMM
Best Regards,
Kwangwoo Lee
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [RFC PATCH 3/3] hw/arm/virt: add nvdimm emulation support
2016-07-20 0:49 [Qemu-devel] [RFC PATCH 0/3] add nvdimm support on AArch64 virt platform Kwangwoo Lee
2016-07-20 0:49 ` [Qemu-devel] [RFC PATCH 1/3] hw/arm/virt: add hotplug memory support Kwangwoo Lee
2016-07-20 0:49 ` [Qemu-devel] [RFC PATCH 2/3] nvdimm: use configurable ACPI IO base and size Kwangwoo Lee
@ 2016-07-20 0:49 ` Kwangwoo Lee
2016-07-25 16:05 ` Peter Maydell
2016-07-25 16:06 ` [Qemu-devel] [RFC PATCH 0/3] add nvdimm support on AArch64 virt platform Peter Maydell
3 siblings, 1 reply; 22+ messages in thread
From: Kwangwoo Lee @ 2016-07-20 0:49 UTC (permalink / raw)
To: Xiao Guangrong, Peter Maydell, Michael S. Tsirkin, Igor Mammedov,
Paolo Bonzini, Richard Henderson, Eduardo Habkost, Shannon Zhao,
Shannon Zhao, qemu-devel, qemu-arm
Cc: Kwangwoo Lee, Woosuk Chung, Hyunchul Kim
This patch enables evaluating NVDIMM on aarch64 virt platform. The
option - nvdimm - passed after machine type is disabled by default.
The command below has been used to test the feature:
./aarch64-softmmu/qemu-system-aarch64 \
-machine type=virt,nvdimm=on \
-cpu cortex-a57 -smp 1 \
-bios ~/oss/edk2/Build/ArmVirtQemu-AARCH64/DEBUG_GCC49/FV/QEMU_EFI.fd \
-m 512,maxmem=2G,slots=2 \
-object memory-backend-file,id=mem1,share,mem-path=./nvdimm1,size=1G \
-device nvdimm,memdev=mem1,id=nv1 \
-fsdev local,id=r,path=/media/sf_Share,security_model=none \
-device virtio-9p-device,fsdev=r,mount_tag=r \
-kernel ../linux/arch/arm64/boot/Image \
--append "console=ttyAMA0 acpi=force" \
-nographic
Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com>
---
default-configs/aarch64-softmmu.mak | 2 ++
hw/arm/virt-acpi-build.c | 5 +++++
hw/arm/virt.c | 34 ++++++++++++++++++++++++++++++++++
include/hw/arm/virt-acpi-build.h | 1 +
include/hw/arm/virt.h | 4 ++++
5 files changed, 46 insertions(+)
diff --git a/default-configs/aarch64-softmmu.mak b/default-configs/aarch64-softmmu.mak
index 5790cd2..295816b 100644
--- a/default-configs/aarch64-softmmu.mak
+++ b/default-configs/aarch64-softmmu.mak
@@ -8,3 +8,5 @@ CONFIG_DDC=y
CONFIG_DPCD=y
CONFIG_XLNX_ZYNQMP=y
CONFIG_MEM_HOTPLUG=y
+CONFIG_NVDIMM=y
+CONFIG_ACPI_NVDIMM=y
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 28fc59c..c3caaa9 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -648,6 +648,7 @@ struct AcpiBuildState {
static
void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
{
+ AcpiNVDIMMState *acpi_nvdimm = guest_info->acpi_nvdimm;
GArray *table_offsets;
unsigned dsdt, rsdt;
GArray *tables_blob = tables->table_data;
@@ -695,6 +696,10 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
build_srat(tables_blob, tables->linker, guest_info);
}
+ if (acpi_nvdimm->is_enabled) {
+ nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
+ acpi_nvdimm);
+ }
/* RSDT is pointed to by RSDP */
rsdt = tables_blob->len;
build_rsdt(tables_blob, tables->linker, table_offsets, NULL, NULL);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index f7ff411..f9db19c 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -94,6 +94,7 @@ typedef struct {
bool highmem;
int32_t gic_version;
MemoryHotplugState hotplug_memory;
+ AcpiNVDIMMState acpi_nvdimm;
} VirtMachineState;
#define TYPE_VIRT_MACHINE MACHINE_TYPE_NAME("virt")
@@ -180,6 +181,7 @@ static const MemMapEntry a15memmap[] = {
[VIRT_FW_CFG] = { 0x09020000, 0x00000018 },
[VIRT_GPIO] = { 0x09030000, 0x00001000 },
[VIRT_SECURE_UART] = { 0x09040000, 0x00001000 },
+ [VIRT_ACPI_IO] = { 0x09050000, 0x00001000 },
[VIRT_MMIO] = { 0x0a000000, 0x00000200 },
/* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
[VIRT_PLATFORM_BUS] = { 0x0c000000, 0x02000000 },
@@ -1376,6 +1378,7 @@ static void machvirt_init(MachineState *machine)
guest_info->irqmap = vbi->irqmap;
guest_info->use_highmem = vms->highmem;
guest_info->gic_version = gic_version;
+ guest_info->acpi_nvdimm = &vms->acpi_nvdimm;
guest_info_state->machine_done.notify = virt_guest_info_machine_done;
qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
@@ -1413,6 +1416,18 @@ static void machvirt_init(MachineState *machine)
&vms->hotplug_memory.mr);
}
+ if (vms->acpi_nvdimm.is_enabled) {
+ AcpiNVDIMMState *acpi_nvdimm = &vms->acpi_nvdimm;
+
+ acpi_nvdimm->dsm_io.type = NVDIMM_ACPI_IO_MEMORY;
+ acpi_nvdimm->dsm_io.base =
+ vbi->memmap[VIRT_ACPI_IO].base + NVDIMM_ACPI_IO_BASE;
+ acpi_nvdimm->dsm_io.size = NVDIMM_ACPI_IO_LEN;
+
+ nvdimm_init_acpi_state(acpi_nvdimm, sysmem,
+ guest_info->fw_cfg, OBJECT(vms));
+ }
+
vbi->bootinfo.ram_size = machine->ram_size;
vbi->bootinfo.kernel_filename = machine->kernel_filename;
vbi->bootinfo.kernel_cmdline = machine->kernel_cmdline;
@@ -1550,6 +1565,20 @@ static HotplugHandler *virt_get_hotplug_handler(MachineState *machine,
return NULL;
}
+static bool virt_get_nvdimm(Object *obj, Error **errp)
+{
+ VirtMachineState *vms = VIRT_MACHINE(obj);
+
+ return vms->acpi_nvdimm.is_enabled;
+}
+
+static void virt_set_nvdimm(Object *obj, bool value, Error **errp)
+{
+ VirtMachineState *vms = VIRT_MACHINE(obj);
+
+ vms->acpi_nvdimm.is_enabled = value;
+}
+
static void virt_machine_class_init(ObjectClass *oc, void *data)
{
MachineClass *mc = MACHINE_CLASS(oc);
@@ -1620,6 +1649,11 @@ static void virt_2_7_instance_init(Object *obj)
object_property_set_description(obj, "gic-version",
"Set GIC version. "
"Valid values are 2, 3 and host", NULL);
+
+ /* nvdimm is disabled on default. */
+ vms->acpi_nvdimm.is_enabled = false;
+ object_property_add_bool(obj, ARCH_VIRT_NVDIMM, virt_get_nvdimm,
+ virt_set_nvdimm, &error_abort);
}
static void virt_machine_2_7_options(MachineClass *mc)
diff --git a/include/hw/arm/virt-acpi-build.h b/include/hw/arm/virt-acpi-build.h
index e43330a..167bbb4 100644
--- a/include/hw/arm/virt-acpi-build.h
+++ b/include/hw/arm/virt-acpi-build.h
@@ -33,6 +33,7 @@ typedef struct VirtGuestInfo {
const int *irqmap;
bool use_highmem;
int gic_version;
+ AcpiNVDIMMState *acpi_nvdimm;
} VirtGuestInfo;
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 35aa3cc..c5cf7e3 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -32,6 +32,7 @@
#include "qemu-common.h"
#include "exec/hwaddr.h"
+#include "hw/mem/nvdimm.h"
#define NUM_GICV2M_SPIS 64
#define NUM_VIRTIO_TRANSPORTS 32
@@ -48,6 +49,8 @@
/* 1GB alignment for hotplug memory region */
#define ARCH_VIRT_HOTPLUG_MEM_ALIGN (1ULL << 30)
+#define ARCH_VIRT_NVDIMM "nvdimm"
+
enum {
VIRT_FLASH,
VIRT_MEM,
@@ -70,6 +73,7 @@ enum {
VIRT_GPIO,
VIRT_SECURE_UART,
VIRT_SECURE_MEM,
+ VIRT_ACPI_IO,
};
typedef struct MemMapEntry {
--
2.5.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [RFC PATCH 3/3] hw/arm/virt: add nvdimm emulation support
2016-07-20 0:49 ` [Qemu-devel] [RFC PATCH 3/3] hw/arm/virt: add nvdimm emulation support Kwangwoo Lee
@ 2016-07-25 16:05 ` Peter Maydell
2016-07-26 7:03 ` kwangwoo.lee
0 siblings, 1 reply; 22+ messages in thread
From: Peter Maydell @ 2016-07-25 16:05 UTC (permalink / raw)
To: Kwangwoo Lee
Cc: Xiao Guangrong, Michael S. Tsirkin, Igor Mammedov, Paolo Bonzini,
Richard Henderson, Eduardo Habkost, Shannon Zhao, Shannon Zhao,
QEMU Developers, qemu-arm, Woosuk Chung, Hyunchul Kim
On 20 July 2016 at 01:49, Kwangwoo Lee <kwangwoo.lee@sk.com> wrote:
> This patch enables evaluating NVDIMM on aarch64 virt platform. The
> option - nvdimm - passed after machine type is disabled by default.
>
> The command below has been used to test the feature:
>
> ./aarch64-softmmu/qemu-system-aarch64 \
> -machine type=virt,nvdimm=on \
> -cpu cortex-a57 -smp 1 \
> -bios ~/oss/edk2/Build/ArmVirtQemu-AARCH64/DEBUG_GCC49/FV/QEMU_EFI.fd \
> -m 512,maxmem=2G,slots=2 \
> -object memory-backend-file,id=mem1,share,mem-path=./nvdimm1,size=1G \
> -device nvdimm,memdev=mem1,id=nv1 \
> -fsdev local,id=r,path=/media/sf_Share,security_model=none \
> -device virtio-9p-device,fsdev=r,mount_tag=r \
> -kernel ../linux/arch/arm64/boot/Image \
> --append "console=ttyAMA0 acpi=force" \
> -nographic
>
> Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com>
> ---
> default-configs/aarch64-softmmu.mak | 2 ++
> hw/arm/virt-acpi-build.c | 5 +++++
> hw/arm/virt.c | 34 ++++++++++++++++++++++++++++++++++
> include/hw/arm/virt-acpi-build.h | 1 +
> include/hw/arm/virt.h | 4 ++++
> 5 files changed, 46 insertions(+)
>
> diff --git a/default-configs/aarch64-softmmu.mak b/default-configs/aarch64-softmmu.mak
> index 5790cd2..295816b 100644
> --- a/default-configs/aarch64-softmmu.mak
> +++ b/default-configs/aarch64-softmmu.mak
> @@ -8,3 +8,5 @@ CONFIG_DDC=y
> CONFIG_DPCD=y
> CONFIG_XLNX_ZYNQMP=y
> CONFIG_MEM_HOTPLUG=y
> +CONFIG_NVDIMM=y
> +CONFIG_ACPI_NVDIMM=y
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 28fc59c..c3caaa9 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -648,6 +648,7 @@ struct AcpiBuildState {
> static
> void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
> {
> + AcpiNVDIMMState *acpi_nvdimm = guest_info->acpi_nvdimm;
> GArray *table_offsets;
> unsigned dsdt, rsdt;
> GArray *tables_blob = tables->table_data;
> @@ -695,6 +696,10 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
> build_srat(tables_blob, tables->linker, guest_info);
> }
>
> + if (acpi_nvdimm->is_enabled) {
> + nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
> + acpi_nvdimm);
> + }
> /* RSDT is pointed to by RSDP */
> rsdt = tables_blob->len;
> build_rsdt(tables_blob, tables->linker, table_offsets, NULL, NULL);
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index f7ff411..f9db19c 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -94,6 +94,7 @@ typedef struct {
> bool highmem;
> int32_t gic_version;
> MemoryHotplugState hotplug_memory;
> + AcpiNVDIMMState acpi_nvdimm;
> } VirtMachineState;
>
> #define TYPE_VIRT_MACHINE MACHINE_TYPE_NAME("virt")
> @@ -180,6 +181,7 @@ static const MemMapEntry a15memmap[] = {
> [VIRT_FW_CFG] = { 0x09020000, 0x00000018 },
> [VIRT_GPIO] = { 0x09030000, 0x00001000 },
> [VIRT_SECURE_UART] = { 0x09040000, 0x00001000 },
> + [VIRT_ACPI_IO] = { 0x09050000, 0x00001000 },
> [VIRT_MMIO] = { 0x0a000000, 0x00000200 },
> /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
> [VIRT_PLATFORM_BUS] = { 0x0c000000, 0x02000000 },
> @@ -1376,6 +1378,7 @@ static void machvirt_init(MachineState *machine)
> guest_info->irqmap = vbi->irqmap;
> guest_info->use_highmem = vms->highmem;
> guest_info->gic_version = gic_version;
> + guest_info->acpi_nvdimm = &vms->acpi_nvdimm;
> guest_info_state->machine_done.notify = virt_guest_info_machine_done;
> qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
>
> @@ -1413,6 +1416,18 @@ static void machvirt_init(MachineState *machine)
> &vms->hotplug_memory.mr);
> }
>
> + if (vms->acpi_nvdimm.is_enabled) {
> + AcpiNVDIMMState *acpi_nvdimm = &vms->acpi_nvdimm;
> +
> + acpi_nvdimm->dsm_io.type = NVDIMM_ACPI_IO_MEMORY;
> + acpi_nvdimm->dsm_io.base =
> + vbi->memmap[VIRT_ACPI_IO].base + NVDIMM_ACPI_IO_BASE;
> + acpi_nvdimm->dsm_io.size = NVDIMM_ACPI_IO_LEN;
> +
> + nvdimm_init_acpi_state(acpi_nvdimm, sysmem,
> + guest_info->fw_cfg, OBJECT(vms));
> + }
This seems to be missing code to write the device tree
information about whatever this device is?
> +
> vbi->bootinfo.ram_size = machine->ram_size;
> vbi->bootinfo.kernel_filename = machine->kernel_filename;
> vbi->bootinfo.kernel_cmdline = machine->kernel_cmdline;
> @@ -1550,6 +1565,20 @@ static HotplugHandler *virt_get_hotplug_handler(MachineState *machine,
> return NULL;
> }
>
> +static bool virt_get_nvdimm(Object *obj, Error **errp)
> +{
> + VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> + return vms->acpi_nvdimm.is_enabled;
> +}
> +
> +static void virt_set_nvdimm(Object *obj, bool value, Error **errp)
> +{
> + VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> + vms->acpi_nvdimm.is_enabled = value;
> +}
> +
> static void virt_machine_class_init(ObjectClass *oc, void *data)
> {
> MachineClass *mc = MACHINE_CLASS(oc);
> @@ -1620,6 +1649,11 @@ static void virt_2_7_instance_init(Object *obj)
> object_property_set_description(obj, "gic-version",
> "Set GIC version. "
> "Valid values are 2, 3 and host", NULL);
> +
> + /* nvdimm is disabled on default. */
> + vms->acpi_nvdimm.is_enabled = false;
> + object_property_add_bool(obj, ARCH_VIRT_NVDIMM, virt_get_nvdimm,
> + virt_set_nvdimm, &error_abort);
> }
>
> static void virt_machine_2_7_options(MachineClass *mc)
> diff --git a/include/hw/arm/virt-acpi-build.h b/include/hw/arm/virt-acpi-build.h
> index e43330a..167bbb4 100644
> --- a/include/hw/arm/virt-acpi-build.h
> +++ b/include/hw/arm/virt-acpi-build.h
> @@ -33,6 +33,7 @@ typedef struct VirtGuestInfo {
> const int *irqmap;
> bool use_highmem;
> int gic_version;
> + AcpiNVDIMMState *acpi_nvdimm;
> } VirtGuestInfo;
>
>
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 35aa3cc..c5cf7e3 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -32,6 +32,7 @@
>
> #include "qemu-common.h"
> #include "exec/hwaddr.h"
> +#include "hw/mem/nvdimm.h"
>
> #define NUM_GICV2M_SPIS 64
> #define NUM_VIRTIO_TRANSPORTS 32
> @@ -48,6 +49,8 @@
> /* 1GB alignment for hotplug memory region */
> #define ARCH_VIRT_HOTPLUG_MEM_ALIGN (1ULL << 30)
>
> +#define ARCH_VIRT_NVDIMM "nvdimm"
Why the #define ?
> +
> enum {
> VIRT_FLASH,
> VIRT_MEM,
> @@ -70,6 +73,7 @@ enum {
> VIRT_GPIO,
> VIRT_SECURE_UART,
> VIRT_SECURE_MEM,
> + VIRT_ACPI_IO,
> };
>
> typedef struct MemMapEntry {
> --
> 2.5.0
thanks
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [RFC PATCH 3/3] hw/arm/virt: add nvdimm emulation support
2016-07-25 16:05 ` Peter Maydell
@ 2016-07-26 7:03 ` kwangwoo.lee
2016-07-26 8:23 ` Peter Maydell
0 siblings, 1 reply; 22+ messages in thread
From: kwangwoo.lee @ 2016-07-26 7:03 UTC (permalink / raw)
To: Peter Maydell
Cc: Xiao Guangrong, Michael S. Tsirkin, Igor Mammedov, Paolo Bonzini,
Richard Henderson, Eduardo Habkost, Shannon Zhao, Shannon Zhao,
QEMU Developers, qemu-arm, woosuk.chung, hyunchul3.kim
Hi Peter,
Please, check the comments below. Thanks a lot!
> -----Original Message-----
> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Tuesday, July 26, 2016 1:06 AM
> To: 이광우(LEE KWANGWOO) MS SW
> Cc: Xiao Guangrong; Michael S. Tsirkin; Igor Mammedov; Paolo Bonzini; Richard Henderson; Eduardo
> Habkost; Shannon Zhao; Shannon Zhao; QEMU Developers; qemu-arm; 정우석(CHUNG WOO SUK) MS SW; 김현철(KIM
> HYUNCHUL) MS SW
> Subject: Re: [RFC PATCH 3/3] hw/arm/virt: add nvdimm emulation support
>
> On 20 July 2016 at 01:49, Kwangwoo Lee <kwangwoo.lee@sk.com> wrote:
> > This patch enables evaluating NVDIMM on aarch64 virt platform. The
> > option - nvdimm - passed after machine type is disabled by default.
> >
> > The command below has been used to test the feature:
> >
> > ./aarch64-softmmu/qemu-system-aarch64 \
> > -machine type=virt,nvdimm=on \
> > -cpu cortex-a57 -smp 1 \
> > -bios ~/oss/edk2/Build/ArmVirtQemu-AARCH64/DEBUG_GCC49/FV/QEMU_EFI.fd \
> > -m 512,maxmem=2G,slots=2 \
> > -object memory-backend-file,id=mem1,share,mem-path=./nvdimm1,size=1G \
> > -device nvdimm,memdev=mem1,id=nv1 \
> > -fsdev local,id=r,path=/media/sf_Share,security_model=none \
> > -device virtio-9p-device,fsdev=r,mount_tag=r \
> > -kernel ../linux/arch/arm64/boot/Image \
> > --append "console=ttyAMA0 acpi=force" \
> > -nographic
> >
> > Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com>
> > ---
> > default-configs/aarch64-softmmu.mak | 2 ++
> > hw/arm/virt-acpi-build.c | 5 +++++
> > hw/arm/virt.c | 34 ++++++++++++++++++++++++++++++++++
> > include/hw/arm/virt-acpi-build.h | 1 +
> > include/hw/arm/virt.h | 4 ++++
> > 5 files changed, 46 insertions(+)
> >
> > diff --git a/default-configs/aarch64-softmmu.mak b/default-configs/aarch64-softmmu.mak
> > index 5790cd2..295816b 100644
> > --- a/default-configs/aarch64-softmmu.mak
> > +++ b/default-configs/aarch64-softmmu.mak
> > @@ -8,3 +8,5 @@ CONFIG_DDC=y
> > CONFIG_DPCD=y
> > CONFIG_XLNX_ZYNQMP=y
> > CONFIG_MEM_HOTPLUG=y
> > +CONFIG_NVDIMM=y
> > +CONFIG_ACPI_NVDIMM=y
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 28fc59c..c3caaa9 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -648,6 +648,7 @@ struct AcpiBuildState {
> > static
> > void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
> > {
> > + AcpiNVDIMMState *acpi_nvdimm = guest_info->acpi_nvdimm;
> > GArray *table_offsets;
> > unsigned dsdt, rsdt;
> > GArray *tables_blob = tables->table_data;
> > @@ -695,6 +696,10 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
> > build_srat(tables_blob, tables->linker, guest_info);
> > }
> >
> > + if (acpi_nvdimm->is_enabled) {
> > + nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
> > + acpi_nvdimm);
> > + }
> > /* RSDT is pointed to by RSDP */
> > rsdt = tables_blob->len;
> > build_rsdt(tables_blob, tables->linker, table_offsets, NULL, NULL);
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index f7ff411..f9db19c 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -94,6 +94,7 @@ typedef struct {
> > bool highmem;
> > int32_t gic_version;
> > MemoryHotplugState hotplug_memory;
> > + AcpiNVDIMMState acpi_nvdimm;
> > } VirtMachineState;
> >
> > #define TYPE_VIRT_MACHINE MACHINE_TYPE_NAME("virt")
> > @@ -180,6 +181,7 @@ static const MemMapEntry a15memmap[] = {
> > [VIRT_FW_CFG] = { 0x09020000, 0x00000018 },
> > [VIRT_GPIO] = { 0x09030000, 0x00001000 },
> > [VIRT_SECURE_UART] = { 0x09040000, 0x00001000 },
> > + [VIRT_ACPI_IO] = { 0x09050000, 0x00001000 },
> > [VIRT_MMIO] = { 0x0a000000, 0x00000200 },
> > /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
> > [VIRT_PLATFORM_BUS] = { 0x0c000000, 0x02000000 },
> > @@ -1376,6 +1378,7 @@ static void machvirt_init(MachineState *machine)
> > guest_info->irqmap = vbi->irqmap;
> > guest_info->use_highmem = vms->highmem;
> > guest_info->gic_version = gic_version;
> > + guest_info->acpi_nvdimm = &vms->acpi_nvdimm;
> > guest_info_state->machine_done.notify = virt_guest_info_machine_done;
> > qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
> >
> > @@ -1413,6 +1416,18 @@ static void machvirt_init(MachineState *machine)
> > &vms->hotplug_memory.mr);
> > }
> >
> > + if (vms->acpi_nvdimm.is_enabled) {
> > + AcpiNVDIMMState *acpi_nvdimm = &vms->acpi_nvdimm;
> > +
> > + acpi_nvdimm->dsm_io.type = NVDIMM_ACPI_IO_MEMORY;
> > + acpi_nvdimm->dsm_io.base =
> > + vbi->memmap[VIRT_ACPI_IO].base + NVDIMM_ACPI_IO_BASE;
> > + acpi_nvdimm->dsm_io.size = NVDIMM_ACPI_IO_LEN;
> > +
> > + nvdimm_init_acpi_state(acpi_nvdimm, sysmem,
> > + guest_info->fw_cfg, OBJECT(vms));
> > + }
>
> This seems to be missing code to write the device tree
> information about whatever this device is?
Is it OK to just add a memory region which cannot be used without ACPI?
This is unclear to me. If you suggest that it is better way, I'll revise
this patch to add a device tree node. Please, help me to understand.
> > +
> > vbi->bootinfo.ram_size = machine->ram_size;
> > vbi->bootinfo.kernel_filename = machine->kernel_filename;
> > vbi->bootinfo.kernel_cmdline = machine->kernel_cmdline;
> > @@ -1550,6 +1565,20 @@ static HotplugHandler *virt_get_hotplug_handler(MachineState *machine,
> > return NULL;
> > }
> >
> > +static bool virt_get_nvdimm(Object *obj, Error **errp)
> > +{
> > + VirtMachineState *vms = VIRT_MACHINE(obj);
> > +
> > + return vms->acpi_nvdimm.is_enabled;
> > +}
> > +
> > +static void virt_set_nvdimm(Object *obj, bool value, Error **errp)
> > +{
> > + VirtMachineState *vms = VIRT_MACHINE(obj);
> > +
> > + vms->acpi_nvdimm.is_enabled = value;
> > +}
> > +
> > static void virt_machine_class_init(ObjectClass *oc, void *data)
> > {
> > MachineClass *mc = MACHINE_CLASS(oc);
> > @@ -1620,6 +1649,11 @@ static void virt_2_7_instance_init(Object *obj)
> > object_property_set_description(obj, "gic-version",
> > "Set GIC version. "
> > "Valid values are 2, 3 and host", NULL);
> > +
> > + /* nvdimm is disabled on default. */
> > + vms->acpi_nvdimm.is_enabled = false;
> > + object_property_add_bool(obj, ARCH_VIRT_NVDIMM, virt_get_nvdimm,
> > + virt_set_nvdimm, &error_abort);
> > }
> >
> > static void virt_machine_2_7_options(MachineClass *mc)
> > diff --git a/include/hw/arm/virt-acpi-build.h b/include/hw/arm/virt-acpi-build.h
> > index e43330a..167bbb4 100644
> > --- a/include/hw/arm/virt-acpi-build.h
> > +++ b/include/hw/arm/virt-acpi-build.h
> > @@ -33,6 +33,7 @@ typedef struct VirtGuestInfo {
> > const int *irqmap;
> > bool use_highmem;
> > int gic_version;
> > + AcpiNVDIMMState *acpi_nvdimm;
> > } VirtGuestInfo;
> >
> >
> > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > index 35aa3cc..c5cf7e3 100644
> > --- a/include/hw/arm/virt.h
> > +++ b/include/hw/arm/virt.h
> > @@ -32,6 +32,7 @@
> >
> > #include "qemu-common.h"
> > #include "exec/hwaddr.h"
> > +#include "hw/mem/nvdimm.h"
> >
> > #define NUM_GICV2M_SPIS 64
> > #define NUM_VIRTIO_TRANSPORTS 32
> > @@ -48,6 +49,8 @@
> > /* 1GB alignment for hotplug memory region */
> > #define ARCH_VIRT_HOTPLUG_MEM_ALIGN (1ULL << 30)
> >
> > +#define ARCH_VIRT_NVDIMM "nvdimm"
>
> Why the #define ?
OK. I'll move this into virt.c when using object_property_add_bool() directly.
> > +
> > enum {
> > VIRT_FLASH,
> > VIRT_MEM,
> > @@ -70,6 +73,7 @@ enum {
> > VIRT_GPIO,
> > VIRT_SECURE_UART,
> > VIRT_SECURE_MEM,
> > + VIRT_ACPI_IO,
> > };
> >
> > typedef struct MemMapEntry {
> > --
> > 2.5.0
>
> thanks
> -- PMM
Thank you very much!
Best Regards,
Kwangwoo Lee
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [RFC PATCH 3/3] hw/arm/virt: add nvdimm emulation support
2016-07-26 7:03 ` kwangwoo.lee
@ 2016-07-26 8:23 ` Peter Maydell
2016-07-27 2:23 ` kwangwoo.lee
0 siblings, 1 reply; 22+ messages in thread
From: Peter Maydell @ 2016-07-26 8:23 UTC (permalink / raw)
To: kwangwoo.lee
Cc: Xiao Guangrong, Michael S. Tsirkin, Igor Mammedov, Paolo Bonzini,
Richard Henderson, Eduardo Habkost, Shannon Zhao, Shannon Zhao,
QEMU Developers, qemu-arm, woosuk.chung, hyunchul3.kim
On 26 July 2016 at 08:03, kwangwoo.lee@sk.com <kwangwoo.lee@sk.com> wrote:
> Hi Peter,
>
> Please, check the comments below. Thanks a lot!
>
>> -----Original Message-----
>> From: Peter Maydell [mailto:peter.maydell@linaro.org]
>> This seems to be missing code to write the device tree
>> information about whatever this device is?
>
> Is it OK to just add a memory region which cannot be used without ACPI?
> This is unclear to me. If you suggest that it is better way, I'll revise
> this patch to add a device tree node. Please, help me to understand.
They don't necessarily both have to be added in the exact same patch
if it's less confusing to split it, but the series as a whole
should support both DT and ACPI (and your kernel patches also
ought to have both DT and ACPI bindings I think).
thanks
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/3] hw/arm/virt: add nvdimm emulation support
2016-07-26 8:23 ` Peter Maydell
@ 2016-07-27 2:23 ` kwangwoo.lee
0 siblings, 0 replies; 22+ messages in thread
From: kwangwoo.lee @ 2016-07-27 2:23 UTC (permalink / raw)
To: Peter Maydell
Cc: Xiao Guangrong, Michael S. Tsirkin, Igor Mammedov, Paolo Bonzini,
Richard Henderson, Eduardo Habkost, Shannon Zhao, Shannon Zhao,
QEMU Developers, qemu-arm, woosuk.chung, hyunchul3.kim
Hi Peter,
Thanks for your guidance!
> -----Original Message-----
> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Tuesday, July 26, 2016 5:23 PM
> To: 이광우(LEE KWANGWOO) MS SW
> Cc: Xiao Guangrong; Michael S. Tsirkin; Igor Mammedov; Paolo Bonzini; Richard Henderson; Eduardo
> Habkost; Shannon Zhao; Shannon Zhao; QEMU Developers; qemu-arm; 정우석(CHUNG WOO SUK) MS SW; 김현철(KIM
> HYUNCHUL) MS SW
> Subject: Re: [RFC PATCH 3/3] hw/arm/virt: add nvdimm emulation support
>
> On 26 July 2016 at 08:03, kwangwoo.lee@sk.com <kwangwoo.lee@sk.com> wrote:
> > Hi Peter,
> >
> > Please, check the comments below. Thanks a lot!
> >
> >> -----Original Message-----
> >> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> >> This seems to be missing code to write the device tree
> >> information about whatever this device is?
> >
> > Is it OK to just add a memory region which cannot be used without ACPI?
> > This is unclear to me. If you suggest that it is better way, I'll revise
> > this patch to add a device tree node. Please, help me to understand.
>
> They don't necessarily both have to be added in the exact same patch
> if it's less confusing to split it, but the series as a whole
> should support both DT and ACPI (and your kernel patches also
> ought to have both DT and ACPI bindings I think).
Although NVDIMM is dependent on ACPI NFIT table currently, I'll keep
this comment in mind and revise it step by step.
> thanks
> -- PMM
Best Regards,
Kwangwoo Lee
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 0/3] add nvdimm support on AArch64 virt platform
2016-07-20 0:49 [Qemu-devel] [RFC PATCH 0/3] add nvdimm support on AArch64 virt platform Kwangwoo Lee
` (2 preceding siblings ...)
2016-07-20 0:49 ` [Qemu-devel] [RFC PATCH 3/3] hw/arm/virt: add nvdimm emulation support Kwangwoo Lee
@ 2016-07-25 16:06 ` Peter Maydell
2016-07-26 6:32 ` kwangwoo.lee
3 siblings, 1 reply; 22+ messages in thread
From: Peter Maydell @ 2016-07-25 16:06 UTC (permalink / raw)
To: Kwangwoo Lee
Cc: Xiao Guangrong, Michael S. Tsirkin, Igor Mammedov, Paolo Bonzini,
Richard Henderson, Eduardo Habkost, Shannon Zhao, Shannon Zhao,
QEMU Developers, qemu-arm, Woosuk Chung, Hyunchul Kim
On 20 July 2016 at 01:49, Kwangwoo Lee <kwangwoo.lee@sk.com> wrote:
> This patch series add an evaluation environment of NVDIMM on AArch64
> virt platform. Three parts need to be implemented to make this feature
> working properly.
>
> 1. memory hotplug on arm virt platform
> 2. configurable ACPI IO base and size both for i386 and arm platform
> 3. enable NVDIMM on arm virt platform
>
> The memory hotplug is required to recognize DIMM type memory and this
> is also used for NVDIMM device recognition.
>
> The ACPI IO base and size need to be changed on creating ACPI NFIT
> because arm platform uses memory-mapped IO which is different from
> i386 platform. Thus, this patch set includes the changes of setting
> ACPI IO base and size during NVDIMM initialization.
>
> The last part enables NVDIMM on arm virt platform. The feature is
> disabled by default. So explicit option - nvdimm=on - is required to
> enable the feature.
Thanks for this patchset. Could you provide a brief summary of
why this is interesting/useful for the benefit of people who don't
know what an NVDIMM is, please?
thanks
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [RFC PATCH 0/3] add nvdimm support on AArch64 virt platform
2016-07-25 16:06 ` [Qemu-devel] [RFC PATCH 0/3] add nvdimm support on AArch64 virt platform Peter Maydell
@ 2016-07-26 6:32 ` kwangwoo.lee
2016-07-29 18:11 ` Peter Maydell
0 siblings, 1 reply; 22+ messages in thread
From: kwangwoo.lee @ 2016-07-26 6:32 UTC (permalink / raw)
To: Peter Maydell
Cc: Xiao Guangrong, Michael S. Tsirkin, Igor Mammedov, Paolo Bonzini,
Richard Henderson, Eduardo Habkost, Shannon Zhao, Shannon Zhao,
QEMU Developers, qemu-arm, woosuk.chung, hyunchul3.kim
Hi Peter,
Thanks for your kind comment! I described about NVDIMM briefly below.
> -----Original Message-----
> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Tuesday, July 26, 2016 1:07 AM
> To: 이광우(LEE KWANGWOO) MS SW
> Cc: Xiao Guangrong; Michael S. Tsirkin; Igor Mammedov; Paolo Bonzini; Richard Henderson; Eduardo
> Habkost; Shannon Zhao; Shannon Zhao; QEMU Developers; qemu-arm; 정우석(CHUNG WOO SUK) MS SW; 김현철(KIM
> HYUNCHUL) MS SW
> Subject: Re: [RFC PATCH 0/3] add nvdimm support on AArch64 virt platform
>
> On 20 July 2016 at 01:49, Kwangwoo Lee <kwangwoo.lee@sk.com> wrote:
> > This patch series add an evaluation environment of NVDIMM on AArch64
> > virt platform. Three parts need to be implemented to make this feature
> > working properly.
> >
> > 1. memory hotplug on arm virt platform
> > 2. configurable ACPI IO base and size both for i386 and arm platform
> > 3. enable NVDIMM on arm virt platform
> >
> > The memory hotplug is required to recognize DIMM type memory and this
> > is also used for NVDIMM device recognition.
> >
> > The ACPI IO base and size need to be changed on creating ACPI NFIT
> > because arm platform uses memory-mapped IO which is different from
> > i386 platform. Thus, this patch set includes the changes of setting
> > ACPI IO base and size during NVDIMM initialization.
> >
> > The last part enables NVDIMM on arm virt platform. The feature is
> > disabled by default. So explicit option - nvdimm=on - is required to
> > enable the feature.
>
> Thanks for this patchset. Could you provide a brief summary of
> why this is interesting/useful for the benefit of people who don't
> know what an NVDIMM is, please?
NVDIMM stands for Non-Volatile DIMM which has DIMM form factor like PC-DIMM
(Memory Module used in PC) and can be used like a memory, but the stored data
is preserved on power failure. So it can be used as a memory, a storage, or
backup memory on power failure.
NVDIMM-N is already out in the market and Intel, HPE, and other companies
actively developing related software. In Linux kernel, several usages exist
like PMEM, BLK-aperture, and mixed mode. They are actively under developing.
In QEMU, NVDIMM has been emulated on i386 platform. My patch series tries to
enable the environment in AArch64 platform. Related Linux kernel patches were
posted into the mailing list and they are under review/revise process currently.
> thanks
> -- PMM
Best Regards,
Kwangwoo Lee
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 0/3] add nvdimm support on AArch64 virt platform
2016-07-26 6:32 ` kwangwoo.lee
@ 2016-07-29 18:11 ` Peter Maydell
0 siblings, 0 replies; 22+ messages in thread
From: Peter Maydell @ 2016-07-29 18:11 UTC (permalink / raw)
To: kwangwoo.lee
Cc: Xiao Guangrong, Michael S. Tsirkin, Igor Mammedov, Paolo Bonzini,
Richard Henderson, Eduardo Habkost, Shannon Zhao, Shannon Zhao,
QEMU Developers, qemu-arm, woosuk.chung, hyunchul3.kim
On 26 July 2016 at 07:32, kwangwoo.lee@sk.com <kwangwoo.lee@sk.com> wrote:
> NVDIMM stands for Non-Volatile DIMM which has DIMM form factor like PC-DIMM
> (Memory Module used in PC) and can be used like a memory, but the stored data
> is preserved on power failure. So it can be used as a memory, a storage, or
> backup memory on power failure.
>
> NVDIMM-N is already out in the market and Intel, HPE, and other companies
> actively developing related software. In Linux kernel, several usages exist
> like PMEM, BLK-aperture, and mixed mode. They are actively under developing.
>
> In QEMU, NVDIMM has been emulated on i386 platform. My patch series tries to
> enable the environment in AArch64 platform. Related Linux kernel patches were
> posted into the mailing list and they are under review/revise process currently.
Thanks. I think I've now made my comments on this series (mostly
pretty minor ones). I'd appreciate it if QEMU devs with knowledge about
the NVDIMM emulation code we already have did a review pass on the
code as well...
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread