From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48397) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z7icA-0006qX-Lh for qemu-devel@nongnu.org; Wed, 24 Jun 2015 07:13:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z7ic5-0003Pf-HI for qemu-devel@nongnu.org; Wed, 24 Jun 2015 07:13:10 -0400 Received: from e23smtp04.au.ibm.com ([202.81.31.146]:58363) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z7ic4-0003P9-KL for qemu-devel@nongnu.org; Wed, 24 Jun 2015 07:13:05 -0400 Received: from /spool/local by e23smtp04.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 24 Jun 2015 21:12:59 +1000 Received: from d23relay07.au.ibm.com (d23relay07.au.ibm.com [9.190.26.37]) by d23dlp03.au.ibm.com (Postfix) with ESMTP id C54FE3578048 for ; Wed, 24 Jun 2015 21:12:55 +1000 (EST) Received: from d23av01.au.ibm.com (d23av01.au.ibm.com [9.190.234.96]) by d23relay07.au.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t5OBCl2c15859762 for ; Wed, 24 Jun 2015 21:12:56 +1000 Received: from d23av01.au.ibm.com (localhost [127.0.0.1]) by d23av01.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t5OBCMDo029056 for ; Wed, 24 Jun 2015 21:12:23 +1000 Date: Wed, 24 Jun 2015 16:41:54 +0530 From: Bharata B Rao Message-ID: <20150624111154.GA5569@in.ibm.com> References: <1434690624-8628-1-git-send-email-bharata@linux.vnet.ibm.com> <1434690624-8628-2-git-send-email-bharata@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1434690624-8628-2-git-send-email-bharata@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [PATCH v2 1/4] pc, pc-dimm: Factor out reusable parts in pc_dimm_plug to a separate routine Reply-To: bharata@linux.vnet.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: pbonzini@redhat.com, imammedo@redhat.com, ehabkost@redhat.com, david@gibson.dropbear.id.au 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 > Reviewed-by: David Gibson > --- > 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.