Re: [Qemu-devel] [RFC PATCH 1/3] hw/arm/virt: add hotplug memory support

[Igor Mammedov <imammedo@redhat.com>]

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