Re: [Qemu-devel] [RFC v2 3/6] hw/arm/virt: Add pc-dimm mem hotplug framework

2018-05-30 Thread Shameerali Kolothum Thodi 
Hi Eric,

> -Original Message-
> From: Auger Eric [mailto:eric.au...@redhat.com]
> Sent: Monday, May 28, 2018 3:22 PM
> To: Shameerali Kolothum Thodi ;
> qemu-devel@nongnu.org; qemu-...@nongnu.org
> Cc: drjo...@redhat.com; imamm...@redhat.com; peter.mayd...@linaro.org;
> alex.william...@redhat.com; Zhaoshenglong ;
> Jonathan Cameron ; Linuxarm
> 
> Subject: Re: [RFC v2 3/6] hw/arm/virt: Add pc-dimm mem hotplug framework
> 
> Hi Shameer,
> 
> On 05/16/2018 05:20 PM, Shameer Kolothum wrote:
> > This will be used in subsequent patches to model a chunk of
> > memory as pc-dimm(cold plug) if the valid iova regions are
> > non-contiguous. This is not yet a full hotplug support.
> Please can you give more details about this restriction?
> >
> > Signed-off-by: Shameer Kolothum 
> > ---
> >  default-configs/aarch64-softmmu.mak |  1 +
> >  hw/arm/virt.c   | 82
> +
> >  include/hw/arm/virt.h   |  2 +
> >  3 files changed, 85 insertions(+)
> >
> > diff --git a/default-configs/aarch64-softmmu.mak b/default-configs/aarch64-
> softmmu.mak
> > index 9ddccf8..7a82ed8 100644
> > --- a/default-configs/aarch64-softmmu.mak
> > +++ b/default-configs/aarch64-softmmu.mak
> > @@ -8,3 +8,4 @@ CONFIG_DDC=y
> >  CONFIG_DPCD=y
> >  CONFIG_XLNX_ZYNQMP=y
> >  CONFIG_XLNX_ZYNQMP_ARM=y
> > +CONFIG_MEM_HOTPLUG=y
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 05fcb62..be3ad14 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -1552,9 +1552,82 @@ static const CPUArchIdList
> *virt_possible_cpu_arch_ids(MachineState *ms)
> >  return ms->possible_cpus;
> >  }
> >
> > +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;
> > +uint64_t align;
> > +Error *local_err = NULL;
> > +
> > +mr = ddc->get_memory_region(dimm, _err);
> > +if (local_err) {
> > +goto out;
> > +}
> > +
> > +align = memory_region_get_alignment(mr);
> I see that on PC machine class they have an "enforce_aligned_dimm"
> option. Also looks memory_region_get_alignment(mr) can return 0.
> 
> if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {

Ok.
 
> 
> > +pc_dimm_memory_plug(dev, >hotplug_memory, mr, align,
> _err);
> > +if (local_err) {
> > +goto out;
> > +}
> useless block and error_propagate does nothing if local_err is NULL so
> we are fine.

Ok. I will remove this from this patch.

> > +
> > +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;
> > +Error *local_err = NULL;
> > +
> > +mr = ddc->get_memory_region(dimm, _err);
> don't you want to avoid executing the following if local_err?

True.

> > +pc_dimm_memory_unplug(dev, >hotplug_memory, mr);
> needs a rebase on top of "pc-dimm: no need to pass the memory region"
> in master pc_dimm_memory_unplug we now have
>  MemoryRegion *mr = ddc->get_memory_region(dimm, _abort);

Right. I will follow that then.

> Thanks
> 
> Eric
> > +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, "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, "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);
> > +

Re: [Qemu-devel] [RFC v2 3/6] hw/arm/virt: Add pc-dimm mem hotplug framework

2018-05-28 Thread Auger Eric 
Hi Shameer,

On 05/16/2018 05:20 PM, Shameer Kolothum wrote:
> This will be used in subsequent patches to model a chunk of
> memory as pc-dimm(cold plug) if the valid iova regions are
> non-contiguous. This is not yet a full hotplug support.
Please can you give more details about this restriction?
> 
> Signed-off-by: Shameer Kolothum 
> ---
>  default-configs/aarch64-softmmu.mak |  1 +
>  hw/arm/virt.c   | 82 
> +
>  include/hw/arm/virt.h   |  2 +
>  3 files changed, 85 insertions(+)
> 
> diff --git a/default-configs/aarch64-softmmu.mak 
> b/default-configs/aarch64-softmmu.mak
> index 9ddccf8..7a82ed8 100644
> --- a/default-configs/aarch64-softmmu.mak
> +++ b/default-configs/aarch64-softmmu.mak
> @@ -8,3 +8,4 @@ CONFIG_DDC=y
>  CONFIG_DPCD=y
>  CONFIG_XLNX_ZYNQMP=y
>  CONFIG_XLNX_ZYNQMP_ARM=y
> +CONFIG_MEM_HOTPLUG=y
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 05fcb62..be3ad14 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1552,9 +1552,82 @@ static const CPUArchIdList 
> *virt_possible_cpu_arch_ids(MachineState *ms)
>  return ms->possible_cpus;
>  }
>  
> +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;
> +uint64_t align;
> +Error *local_err = NULL;
> +
> +mr = ddc->get_memory_region(dimm, _err);
> +if (local_err) {
> +goto out;
> +}
> +
> +align = memory_region_get_alignment(mr);
I see that on PC machine class they have an "enforce_aligned_dimm"
option. Also looks memory_region_get_alignment(mr) can return 0.

if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {


> +pc_dimm_memory_plug(dev, >hotplug_memory, mr, align, _err);
> +if (local_err) {
> +goto out;
> +}
useless block and error_propagate does nothing if local_err is NULL so
we are fine.
> +
> +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;
> +Error *local_err = NULL;
> +
> +mr = ddc->get_memory_region(dimm, _err);
don't you want to avoid executing the following if local_err?
> +pc_dimm_memory_unplug(dev, >hotplug_memory, mr);
needs a rebase on top of "pc-dimm: no need to pass the memory region"
in master pc_dimm_memory_unplug we now have
 MemoryRegion *mr = ddc->get_memory_region(dimm, _abort);

Thanks

Eric
> +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, "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, "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
> @@ -1573,6 +1646,11 @@ static void virt_machine_class_init(ObjectClass *oc, 
> void *data)
>  mc->cpu_index_to_instance_props = virt_cpu_index_to_props;
>  mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a15");
>  mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
> +
> +mc->get_hotplug_handler = virt_get_hotplug_handler;
This requires a rebase on top of Igor's  and David's series.

Thanks

Eric
> +hc->plug = virt_machinedevice_plug_cb;
> +hc->unplug = virt_machinedevice_unplug_cb;
> +
>  }
>  
>  static const TypeInfo

[Qemu-devel] [RFC v2 3/6] hw/arm/virt: Add pc-dimm mem hotplug framework

2018-05-16 Thread Shameer Kolothum 
This will be used in subsequent patches to model a chunk of
memory as pc-dimm(cold plug) if the valid iova regions are
non-contiguous. This is not yet a full hotplug support.

Signed-off-by: Shameer Kolothum 
---
 default-configs/aarch64-softmmu.mak |  1 +
 hw/arm/virt.c   | 82 +
 include/hw/arm/virt.h   |  2 +
 3 files changed, 85 insertions(+)

diff --git a/default-configs/aarch64-softmmu.mak 
b/default-configs/aarch64-softmmu.mak
index 9ddccf8..7a82ed8 100644
--- a/default-configs/aarch64-softmmu.mak
+++ b/default-configs/aarch64-softmmu.mak
@@ -8,3 +8,4 @@ CONFIG_DDC=y
 CONFIG_DPCD=y
 CONFIG_XLNX_ZYNQMP=y
 CONFIG_XLNX_ZYNQMP_ARM=y
+CONFIG_MEM_HOTPLUG=y
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 05fcb62..be3ad14 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1552,9 +1552,82 @@ static const CPUArchIdList 
*virt_possible_cpu_arch_ids(MachineState *ms)
 return ms->possible_cpus;
 }
 
+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;
+uint64_t align;
+Error *local_err = NULL;
+
+mr = ddc->get_memory_region(dimm, _err);
+if (local_err) {
+goto out;
+}
+
+align = memory_region_get_alignment(mr);
+pc_dimm_memory_plug(dev, >hotplug_memory, mr, align, _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;
+Error *local_err = NULL;
+
+mr = ddc->get_memory_region(dimm, _err);
+pc_dimm_memory_unplug(dev, >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, "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, "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
@@ -1573,6 +1646,11 @@ static void virt_machine_class_init(ObjectClass *oc, 
void *data)
 mc->cpu_index_to_instance_props = virt_cpu_index_to_props;
 mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a15");
 mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
+
+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 = {
@@ -1582,6 +1660,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 fc24f3a..a39f29e 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -35,6 +35,7 @@
 #include "qemu/notify.h"
 #include "hw/boards.h"
 #include "hw/arm/arm.h"
+#include "hw/mem/pc-dimm.h"
 
 #define NUM_GICV2M_SPIS   64
 #define NUM_VIRTIO_TRANSPORTS 32
@@ -108,6 +109,7 @@ typedef struct {
 uint32_t gic_phandle;
 uint32_t msi_phandle;
 int psci_conduit;
+MemoryHotplugState hotplug_memory;
 } VirtMachineState;
 
 #define TYPE_VIRT_MACHINE   MACHINE_TYPE_NAME("virt")
-- 
2.7.4