On Thu, 17 May 2018 10:15:25 +0200 David Hildenbrand <da...@redhat.com> wrote:
> Let's move all pre-plug checks we can do without the device being > realized into the applicable hotplug handler for pc and spapr. > > Signed-off-by: David Hildenbrand <da...@redhat.com> > --- > hw/i386/pc.c | 11 +++++++ > hw/mem/memory-device.c | 72 > +++++++++++++++++++----------------------- > hw/ppc/spapr.c | 11 +++++++ > include/hw/mem/memory-device.h | 2 ++ > 4 files changed, 57 insertions(+), 39 deletions(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 8bc41ef24b..61f1537e14 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -2010,6 +2010,16 @@ static void > pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, > { > Error *local_err = NULL; > > + /* first stage hotplug handler */ > + if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) { > + memory_device_pre_plug(MACHINE(hotplug_dev), MEMORY_DEVICE(dev), > + &local_err); > + } > + > + if (local_err) { > + goto out; > + } > + > /* final stage hotplug handler */ > if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > pc_cpu_pre_plug(hotplug_dev, dev, &local_err); > @@ -2017,6 +2027,7 @@ static void > pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, > hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev, > &local_err); > } > +out: > error_propagate(errp, local_err); > } > > diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c > index 361d38bfc5..d22c91993f 100644 > --- a/hw/mem/memory-device.c > +++ b/hw/mem/memory-device.c > @@ -68,58 +68,26 @@ static int memory_device_used_region_size(Object *obj, > void *opaque) > return 0; > } > > -static void memory_device_check_addable(MachineState *ms, uint64_t size, > - Error **errp) > -{ > - uint64_t used_region_size = 0; > - > - /* we will need a new memory slot for kvm and vhost */ > - if (kvm_enabled() && !kvm_has_free_slot(ms)) { > - error_setg(errp, "hypervisor has no free memory slots left"); > - return; > - } > - if (!vhost_has_free_slot()) { > - error_setg(errp, "a used vhost backend has no free memory slots > left"); > - return; > - } > - > - /* will we exceed the total amount of memory specified */ > - memory_device_used_region_size(OBJECT(ms), &used_region_size); > - if (used_region_size + size > ms->maxram_size - ms->ram_size) { > - error_setg(errp, "not enough space, currently 0x%" PRIx64 > - " in use of total hot pluggable 0x" RAM_ADDR_FMT, > - used_region_size, ms->maxram_size - ms->ram_size); > - return; > - } > - > -} > - > 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; > GSList *list = NULL, *item; > uint64_t new_addr = 0; > > - if (!ms->device_memory) { > - error_setg(errp, "memory devices (e.g. for memory hotplug) are not " > - "supported by the machine"); > - return 0; > - } > - > - if (!memory_region_size(&ms->device_memory->mr)) { > - error_setg(errp, "memory devices (e.g. for memory hotplug) are not " > - "enabled, please specify the maxmem option"); > - return 0; > - } > address_space_start = ms->device_memory->base; > address_space_end = address_space_start + > memory_region_size(&ms->device_memory->mr); > g_assert(address_space_end >= address_space_start); > > - memory_device_check_addable(ms, size, errp); > - if (*errp) { > + /* will we exceed the total amount of memory specified */ > + memory_device_used_region_size(OBJECT(ms), &used_region_size); > + if (used_region_size + size > ms->maxram_size - ms->ram_size) { > + error_setg(errp, "not enough space, currently 0x%" PRIx64 > + " in use of total hot pluggable 0x" RAM_ADDR_FMT, > + used_region_size, ms->maxram_size - ms->ram_size); > return 0; > } > > @@ -242,6 +210,32 @@ uint64_t get_plugged_memory_size(void) > return size; > } > > +void memory_device_pre_plug(MachineState *ms, const MemoryDeviceState *md, > + Error **errp) > +{ > + if (!ms->device_memory) { > + error_setg(errp, "memory devices (e.g. for memory hotplug) are not " > + "supported by the machine"); > + return; > + } > + > + if (!memory_region_size(&ms->device_memory->mr)) { > + error_setg(errp, "memory devices (e.g. for memory hotplug) are not " > + "enabled, please specify the maxmem option"); > + return; > + } > + > + /* we will need a new memory slot for kvm and vhost */ > + if (kvm_enabled() && !kvm_has_free_slot(ms)) { > + error_setg(errp, "hypervisor has no free memory slots left"); > + return; > + } > + if (!vhost_has_free_slot()) { > + error_setg(errp, "a used vhost backend has no free memory slots > left"); > + return; > + } thanks for extracting preparations steps into the right callback. on top of this _preplug() handler should also set being plugged device properties if they weren't set by user. memory_device_get_free_addr() should be here to. general rule for _preplug() would be to check and prepare device for being plugged but not touch anything beside the device (it's _plug handler job) > +} > + > void memory_device_plug_region(MachineState *ms, MemoryRegion *mr, > uint64_t addr) > { > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 13d153b5a6..562712def2 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3676,6 +3676,16 @@ static void > spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev, > { > Error *local_err = NULL; > > + /* first stage hotplug handler */ > + if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) { > + memory_device_pre_plug(MACHINE(hotplug_dev), MEMORY_DEVICE(dev), > + &local_err); > + } > + > + if (local_err) { > + goto out; > + } > + > /* final stage hotplug handler */ > if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > spapr_memory_pre_plug(hotplug_dev, dev, &local_err); > @@ -3685,6 +3695,7 @@ static void > spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev, > hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev, > &local_err); > } > +out: > error_propagate(errp, local_err); > } > > diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h > index 62d906be50..3a4e9edc92 100644 > --- a/include/hw/mem/memory-device.h > +++ b/include/hw/mem/memory-device.h > @@ -51,6 +51,8 @@ typedef struct MemoryDeviceClass { > > 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);