Re: [Qemu-devel] [PATCH v2 1/4] pc, pc-dimm: Factor out reusable parts in pc_dimm_plug to a separate routine

[Igor Mammedov]

On Fri, 19 Jun 2015 10:40:21 +0530
Bharata B Rao <address@hidden> wrote:

> pc_dimm_plug() has code that will be needed for memory plug handlers
> in other archs too. Extract code from pc_dimm_plug() into a generic
> routine pc_dimm_memory_plug() that resides in pc-dimm.c. Also
> correspondingly refactor re-usable unplug code into
> pc_dimm_memory_unplug().
> 
> Signed-off-by: Bharata B Rao <address@hidden>
> Reviewed-by: David Gibson <address@hidden>
> ---
>  hw/i386/acpi-build.c     |  2 +-
>  hw/i386/pc.c             | 90
> +++++++++---------------------------------------
> hw/mem/pc-dimm.c         | 80
> ++++++++++++++++++++++++++++++++++++++++++ include/hw/i386/pc.h
> |  4 +-- include/hw/mem/pc-dimm.h |  9 +++++ 5 files changed, 109
> insertions(+), 76 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b71e942..5f6fa95 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1512,7 +1512,7 @@ build_srat(GArray *table_data, GArray *linker,
> PcGuestInfo *guest_info) */
>      if (hotplugabble_address_space_size) {
>          numamem = acpi_data_push(table_data, sizeof *numamem);
> -        acpi_build_srat_memory(numamem, pcms->hotplug_memory_base,
> +        acpi_build_srat_memory(numamem, pcms->hotplug_memory.base,
>                                 hotplugabble_address_space_size, 0,
>                                 MEM_AFFINITY_HOTPLUGGABLE |
>                                 MEM_AFFINITY_ENABLED);
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 3f0d435..c869588 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -64,7 +64,6 @@
>  #include "hw/pci/pci_host.h"
>  #include "acpi-build.h"
>  #include "hw/mem/pc-dimm.h"
> -#include "trace.h"
>  #include "qapi/visitor.h"
>  #include "qapi-visit.h"
>  
> @@ -1297,7 +1296,7 @@ FWCfgState *pc_memory_init(MachineState
> *machine, exit(EXIT_FAILURE);
>          }
>  
> -        pcms->hotplug_memory_base =
> +        pcms->hotplug_memory.base =
could you split out conversion to structure into separate patch, pls?

>              ROUND_UP(0x100000000ULL + above_4g_mem_size, 1ULL << 30);
>  
>          if (pcms->enforce_aligned_dimm) {
> @@ -1305,17 +1304,17 @@ FWCfgState *pc_memory_init(MachineState
> *machine, hotplug_mem_size += (1ULL << 30) * machine->ram_slots;
>          }
>  
> -        if ((pcms->hotplug_memory_base + hotplug_mem_size) <
> +        if ((pcms->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(&pcms->hotplug_memory, OBJECT(pcms),
> +        memory_region_init(&pcms->hotplug_memory.mr, OBJECT(pcms),
>                             "hotplug-memory", hotplug_mem_size);
> -        memory_region_add_subregion(system_memory,
> pcms->hotplug_memory_base,
> -                                    &pcms->hotplug_memory);
> +        memory_region_add_subregion(system_memory,
> pcms->hotplug_memory.base,
> +                                    &pcms->hotplug_memory.mr);
>      }
>  
>      /* Initialize PC system firmware */
> @@ -1333,9 +1332,9 @@ FWCfgState *pc_memory_init(MachineState
> *machine, fw_cfg = bochs_bios_init();
>      rom_set_fw(fw_cfg);
>  
> -    if (guest_info->has_reserved_memory &&
> pcms->hotplug_memory_base) {
> +    if (guest_info->has_reserved_memory &&
> pcms->hotplug_memory.base) { uint64_t *val = g_malloc(sizeof(*val));
> -        *val = cpu_to_le64(ROUND_UP(pcms->hotplug_memory_base,
> 0x1ULL << 30));
> +        *val = cpu_to_le64(ROUND_UP(pcms->hotplug_memory.base,
> 0x1ULL << 30)); fw_cfg_add_file(fw_cfg, "etc/reserved-memory-end",
> val, sizeof(*val)); }
>  
> @@ -1554,20 +1553,17 @@ void ioapic_init_gsi(GSIState *gsi_state,
> const char *parent_name) static void pc_dimm_plug(HotplugHandler
> *hotplug_dev, DeviceState *dev, Error **errp)
>  {
> -    int slot;
>      HotplugHandlerClass *hhc;
>      Error *local_err = NULL;
>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> -    MachineState *machine = MACHINE(hotplug_dev);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      MemoryRegion *mr = ddc->get_memory_region(dimm);
> -    uint64_t existing_dimms_capacity = 0;
>      uint64_t align = TARGET_PAGE_SIZE;
> -    uint64_t addr;
>  
> -    addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP,
> &local_err);
> -    if (local_err) {
> +    if (!pcms->acpi_dev) {
> +        error_setg(&local_err,
> +                   "memory hotplug is not enabled: missing acpi
> device"); goto out;
>      }
>  
> @@ -1575,67 +1571,18 @@ static void pc_dimm_plug(HotplugHandler
> *hotplug_dev, align = memory_region_get_alignment(mr);
>      }
>  
> -    addr = pc_dimm_get_free_addr(pcms->hotplug_memory_base,
> -
> memory_region_size(&pcms->hotplug_memory),
> -                                 !addr ? NULL : &addr, align,
> -                                 memory_region_size(mr), &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -
> -    existing_dimms_capacity = pc_existing_dimms_capacity(&local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -
> -    if (existing_dimms_capacity + memory_region_size(mr) >
> -        machine->maxram_size - machine->ram_size) {
> -        error_setg(&local_err, "not enough space, currently 0x%"
> PRIx64
> -                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
> -                   existing_dimms_capacity,
> -                   machine->maxram_size - machine->ram_size);
> -        goto out;
> -    }
> -
> -    object_property_set_int(OBJECT(dev), addr, PC_DIMM_ADDR_PROP,
> &local_err);
> +    pc_dimm_memory_plug(dev, &pcms->hotplug_memory, mr, align,
> &local_err); if (local_err) {
> +        pc_dimm_memory_unplug(dev, &pcms->hotplug_memory, mr);
>          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) {
> -        goto out;
> -    }
> -
> -    slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ?
> NULL : &slot,
> -                                 machine->ram_slots, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -    object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP,
> &local_err);
> +    hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
> +    hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
>      if (local_err) {
> -        goto out;
> +        pc_dimm_memory_unplug(dev, &pcms->hotplug_memory, mr);
>      }
> -    trace_mhp_pc_dimm_assigned_slot(slot);
>  
> -    if (!pcms->acpi_dev) {
> -        error_setg(&local_err,
> -                   "memory hotplug is not enabled: missing acpi
> device");
> -        goto out;
> -    }
> -
> -    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
> -        error_setg(&local_err, "hypervisor has no free memory slots
> left");
> -        goto out;
> -    }
> -
> -    memory_region_add_subregion(&pcms->hotplug_memory,
> -                                addr - pcms->hotplug_memory_base,
> mr);
> -    vmstate_register_ram(mr, dev);
> -
> -    hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
> -    hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
>  out:
>      error_propagate(errp, local_err);
>  }
> @@ -1677,11 +1624,8 @@ static void pc_dimm_unplug(HotplugHandler
> *hotplug_dev, goto out;
>      }
>  
> -    memory_region_del_subregion(&pcms->hotplug_memory, mr);
> -    vmstate_unregister_ram(mr, dev);
> -
> +    pc_dimm_memory_unplug(dev, &pcms->hotplug_memory, mr);
>      object_unparent(OBJECT(dev));
> -
>   out:
>      error_propagate(errp, local_err);
>  }
> @@ -1766,7 +1710,7 @@
> pc_machine_get_hotplug_memory_region_size(Object *obj, Visitor *v,
> void *opaque, const char *name, Error **errp) {
>      PCMachineState *pcms = PC_MACHINE(obj);
> -    int64_t value = memory_region_size(&pcms->hotplug_memory);
> +    int64_t value = memory_region_size(&pcms->hotplug_memory.mr);
>  
>      visit_type_int(v, &value, name, errp);
>  }
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index e70633d..98971b7 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -23,12 +23,92 @@
>  #include "qapi/visitor.h"
>  #include "qemu/range.h"
>  #include "sysemu/numa.h"
> +#include "sysemu/kvm.h"
> +#include "trace.h"
>  
>  typedef struct pc_dimms_capacity {
>       uint64_t size;
>       Error    **errp;
>  } pc_dimms_capacity;
>  
> +void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
> +                         MemoryRegion *mr, uint64_t align, Error
> **errp) +{
> +    int slot;
> +    MachineState *machine = MACHINE(qdev_get_machine());
> +    PCDIMMDevice *dimm = PC_DIMM(dev);
> +    Error *local_err = NULL;
> +    uint64_t existing_dimms_capacity = 0;
> +    uint64_t addr;
> +
> +    addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP,
> &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    addr = pc_dimm_get_free_addr(hpms->base,
> +                                 memory_region_size(&hpms->mr),
> +                                 !addr ? NULL : &addr, align,
> +                                 memory_region_size(mr), &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    existing_dimms_capacity = pc_existing_dimms_capacity(&local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    if (existing_dimms_capacity + memory_region_size(mr) >
> +        machine->maxram_size - machine->ram_size) {
> +        error_setg(&local_err, "not enough space, currently 0x%"
> PRIx64
> +                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
> +                   existing_dimms_capacity,
> +                   machine->maxram_size - machine->ram_size);
> +        goto out;
> +    }
> +
> +    object_property_set_int(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) {
> +        goto out;
> +    }
> +
> +    slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ?
> NULL : &slot,
> +                                 machine->ram_slots, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +    object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP,
> &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +    trace_mhp_pc_dimm_assigned_slot(slot);
> +
> +    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
> +        error_setg(&local_err, "hypervisor has no free memory slots
> left");
> +        goto out;
> +    }
> +
> +    memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);
> +    vmstate_register_ram(mr, dev);
> +
> +out:
> +    error_propagate(errp, local_err);
> +}
> +
> +void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState
> *hpms,
> +                           MemoryRegion *mr)
> +{
> +    memory_region_del_subregion(&hpms->mr, mr);
> +    vmstate_unregister_ram(mr, dev);
> +}
> +
>  static int pc_existing_dimms_capacity_internal(Object *obj, void
> *opaque) {
>      pc_dimms_capacity *cap = opaque;
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 86c5651..6628791 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -15,6 +15,7 @@
>  #include "hw/pci/pci.h"
>  #include "hw/boards.h"
>  #include "hw/compat.h"
> +#include "hw/mem/pc-dimm.h"
>  
>  #define HPET_INTCAP "hpet-intcap"
>  
> @@ -32,8 +33,7 @@ struct PCMachineState {
>      MachineState parent_obj;
>  
>      /* <public> */
> -    ram_addr_t hotplug_memory_base;
> -    MemoryRegion hotplug_memory;
> +    MemoryHotplugState hotplug_memory;
>  
>      HotplugHandler *acpi_dev;
>      ISADevice *rtc;
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index f7b80b4..9fbab16 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -70,6 +70,11 @@ typedef struct PCDIMMDeviceClass {
>      MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm);
>  } PCDIMMDeviceClass;
>  
> +typedef struct MemoryHotplugState {
> +    ram_addr_t base;
> +    MemoryRegion mr;
> +} MemoryHotplugState;
> +
>  uint64_t pc_dimm_get_free_addr(uint64_t address_space_start,
>                                 uint64_t address_space_size,
>                                 uint64_t *hint, uint64_t align,
> uint64_t size, @@ -79,4 +84,8 @@ int pc_dimm_get_free_slot(const int
> *hint, int max_slots, Error **errp); 
>  int qmp_pc_dimm_device_list(Object *obj, void *opaque);
>  uint64_t pc_existing_dimms_capacity(Error **errp);
> +void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
> +                         MemoryRegion *mr, uint64_t align, Error
> **errp); +void pc_dimm_memory_unplug(DeviceState *dev,
> MemoryHotplugState *hpms,
> +                           MemoryRegion *mr);
>  #endif