Re: [Qemu-devel] [PATCH v4 14/14] memory-device: factor out plug into hotplug handler

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 17 May 2018 10:15:27 +0200
David Hildenbrand <david@redhat.com> wrote:

> Let's move the plug logic into the applicable hotplug handler for pc and
> spapr.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/i386/pc.c                   | 35 ++++++++++++++++++++---------------
>  hw/mem/memory-device.c         | 40 ++++++++++++++++++++++++++++++++++------
>  hw/mem/pc-dimm.c               | 29 +----------------------------
>  hw/mem/trace-events            |  2 +-
>  hw/ppc/spapr.c                 | 15 ++++++++++++---
>  include/hw/mem/memory-device.h |  7 ++-----
>  include/hw/mem/pc-dimm.h       |  3 +--
>  7 files changed, 71 insertions(+), 60 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 426fb534c2..f022eb042e 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1682,22 +1682,8 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>      HotplugHandlerClass *hhc;
>      Error *local_err = NULL;
>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> -    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> -    PCDIMMDevice *dimm = PC_DIMM(dev);
> -    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr;
> -    uint64_t align = TARGET_PAGE_SIZE;
>      bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>  
> -    mr = ddc->get_memory_region(dimm, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -
> -    if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {
> -        align = memory_region_get_alignment(mr);
> -    }
> -
>      /*
>       * When -no-acpi is used with Q35 machine type, no ACPI is built,
>       * but pcms->acpi_dev is still created. Check !acpi_enabled in
> @@ -1715,7 +1701,7 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>          goto out;
>      }
>  
> -    pc_dimm_memory_plug(dev, MACHINE(pcms), align, &local_err);
> +    pc_dimm_memory_plug(dev, MACHINE(pcms), &local_err);
>      if (local_err) {
>          goto out;
>      }
> @@ -2036,6 +2022,25 @@ static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>  {
>      Error *local_err = NULL;
>  
> +    /* first stage hotplug handler */
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
> +        const PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(hotplug_dev);
> +        uint64_t align = 0;
> +
> +        /* compat handling: force to TARGET_PAGE_SIZE */
> +        if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
> +            !pcmc->enforce_aligned_dimm) {
> +            align = TARGET_PAGE_SIZE;
> +        }
> +        memory_device_plug(MACHINE(hotplug_dev), MEMORY_DEVICE(dev),
> +                           align ? &align : NULL, &local_err);
> +    }
> +
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
>      /* final stage hotplug handler */
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>          pc_dimm_plug(hotplug_dev, dev, &local_err);
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index 8f10d613ea..04bdb30f22 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -69,9 +69,10 @@ static int memory_device_used_region_size(Object *obj, void *opaque)
>      return 0;
>  }
>  
> -uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
> -                                     uint64_t align, uint64_t size,
> -                                     Error **errp)
> +static uint64_t memory_device_get_free_addr(MachineState *ms,
> +                                            const uint64_t *hint,
> +                                            uint64_t align, uint64_t size,
> +                                            Error **errp)
>  {
>      uint64_t address_space_start, address_space_end;
>      uint64_t used_region_size = 0;
> @@ -237,11 +238,38 @@ void memory_device_pre_plug(MachineState *ms, const MemoryDeviceState *md,
>      }
>  }
>  
> -void memory_device_plug_region(MachineState *ms, MemoryRegion *mr,
> -                               uint64_t addr)
> +void memory_device_plug(MachineState *ms, MemoryDeviceState *md,
> +                        uint64_t *enforced_align, Error **errp)
enforced_align is PC machine specific compat flag
to keep old machines with unaligned layout work (i.e. don't break CLI/migration)
it shouldn't go into a generic code.
By default all new machines should use aligned layout. 

>  {
> -    /* we expect a previous call to memory_device_get_free_addr() */
> +    const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(md);
> +    const uint64_t size = mdc->get_region_size(md);
> +    MemoryRegion *mr = mdc->get_memory_region(md);
> +    uint64_t addr = mdc->get_addr(md);
> +    uint64_t align;
> +
> +    /* we expect a previous call to memory_device_pre_plug */
>      g_assert(ms->device_memory);
> +    g_assert(mr && !memory_region_is_mapped(mr));
> +
> +    /* compat handling, some alignment has to be enforced for DIMMs */
> +    if (enforced_align) {
> +        align = *enforced_align;
> +    } else {
> +        align = memory_region_get_alignment(mr);
> +    }
> +
> +    /* our device might have stronger alignment requirements */
> +    if (mdc->get_align) {
> +        align = MAX(align, mdc->get_align(md));
> +    }
> +
> +    addr = memory_device_get_free_addr(ms, !addr ? NULL : &addr, align,
> +                                       size, errp);
> +    if (*errp) {
> +        return;
> +    }
> +    trace_memory_device_assign_address(addr);
> +    mdc->set_addr(md, addr);
>  
>      memory_region_add_subregion(&ms->device_memory->mr,
>                                  addr - ms->device_memory->base, mr);
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index d487bb513b..8b1dcb3260 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -32,39 +32,13 @@ typedef struct pc_dimms_capacity {
>       Error    **errp;
>  } pc_dimms_capacity;
>  
> -void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
> -                         uint64_t align, Error **errp)
> +void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine, Error **errp)
>  {
>      int slot;
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm);
>      Error *local_err = NULL;
> -    MemoryRegion *mr;
> -    uint64_t addr;
> -
> -    mr = ddc->get_memory_region(dimm, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -
> -    addr = object_property_get_uint(OBJECT(dimm),
> -                                    PC_DIMM_ADDR_PROP, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -
> -    addr = memory_device_get_free_addr(machine, !addr ? NULL : &addr, align,
> -                                       memory_region_size(mr), &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -
> -    object_property_set_uint(OBJECT(dev), addr, PC_DIMM_ADDR_PROP, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -    trace_mhp_pc_dimm_assigned_address(addr);
>  
>      slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP, &local_err);
>      if (local_err) {
> @@ -82,7 +56,6 @@ void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
>      }
>      trace_mhp_pc_dimm_assigned_slot(slot);
>  
> -    memory_device_plug_region(machine, mr, addr);
>      vmstate_register_ram(vmstate_mr, dev);
>  
>  out:
> diff --git a/hw/mem/trace-events b/hw/mem/trace-events
> index a661ee49a3..930b6aa6ea 100644
> --- a/hw/mem/trace-events
> +++ b/hw/mem/trace-events
> @@ -2,6 +2,6 @@
>  
>  # hw/mem/pc-dimm.c
>  mhp_pc_dimm_assigned_slot(int slot) "%d"
> -mhp_pc_dimm_assigned_address(uint64_t addr) "0x%"PRIx64
>  # hw/mem/memory-device.c
> +memory_device_assign_address(uint64_t addr) "0x%"PRIx64
>  memory_device_unassign_address(uint64_t addr) "0x%"PRIx64
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index abdd38a6b5..5a4dbbf31e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3144,16 +3144,15 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      MemoryRegion *mr;
> -    uint64_t align, size, addr;
> +    uint64_t size, addr;
>  
>      mr = ddc->get_memory_region(dimm, &local_err);
>      if (local_err) {
>          goto out;
>      }
> -    align = memory_region_get_alignment(mr);
>      size = memory_region_size(mr);
>  
> -    pc_dimm_memory_plug(dev, MACHINE(ms), align, &local_err);
> +    pc_dimm_memory_plug(dev, MACHINE(ms), &local_err);
>      if (local_err) {
>          goto out;
>      }
> @@ -3595,6 +3594,16 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>      sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
>      Error *local_err = NULL;
>  
> +    /* first stage hotplug handler */
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
> +        memory_device_plug(ms, MEMORY_DEVICE(dev), NULL, &local_err);
> +    }
> +
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
>      /* final stage hotplug handler */
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>          int node;
> diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
> index b8365959e7..a7408597fd 100644
> --- a/include/hw/mem/memory-device.h
> +++ b/include/hw/mem/memory-device.h
> @@ -53,11 +53,8 @@ MemoryDeviceInfoList *qmp_memory_device_list(void);
>  uint64_t get_plugged_memory_size(void);
>  void memory_device_pre_plug(MachineState *ms, const MemoryDeviceState *md,
>                              Error **errp);
> -uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
> -                                     uint64_t align, uint64_t size,
> -                                     Error **errp);
> -void memory_device_plug_region(MachineState *ms, MemoryRegion *mr,
> -                               uint64_t addr);
> +void memory_device_plug(MachineState *ms, MemoryDeviceState *md,
> +                        uint64_t *enforced_align, Error **errp);
>  void memory_device_unplug(MachineState *ms, MemoryDeviceState *md);
>  
>  #endif
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index 627c8601d9..006c80fb2e 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -78,7 +78,6 @@ typedef struct PCDIMMDeviceClass {
>  
>  int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
>  
> -void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
> -                         uint64_t align, Error **errp);
> +void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine, Error **errp);
>  void pc_dimm_memory_unplug(DeviceState *dev, MachineState *machine);
>  #endif