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

[Bharata B Rao <bharata@linux.vnet.ibm.com>]

On Fri, Jun 19, 2015 at 10:40:21AM +0530, Bharata B Rao 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 <bharata@linux.vnet.ibm.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  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 =
>              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;
>      }

Shouldn't be calling pc_dimm_memory_unplug() here because when
pc_dimm_memory_plug() fails, it wouldn't have done
memory_region_add_subregion() and vmstate_register_ram().

Will fix this in the next post.

Regards,
Bharata.