On 07.06.2018 17:00, Igor Mammedov wrote: > On Mon, 4 Jun 2018 13:45:38 +0200 > David Hildenbrand <da...@redhat.com> wrote: > >> On 01.06.2018 13:17, Igor Mammedov wrote: >>> 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) >> >> I disagree. Or at least I disagree as part of this patch series because >> it over-complicates things :) > "Welcome to rewrite QEMU first before you do your thing" club :)
I feels like I'm doing nothing else all time :) > > That's why I've suggested to split series in several small ones > tackling concrete goals /1st: unplug cleanups, 2nd: preplug refactoring/ > instead of intermixing loosely related patches. I'll send the fixes and cleanups fairly soon. That will hopefully reduce the patch count (it increased already a lot due to your review already). > > It should help to review and merge prerequisite work fast as it doesn't > related to virtio-mem. The rest of refactoring (which is not much once > you split out the 2 first series) that's is done directly for virtio-mem > sake should be posted as part of that series. > It probably would be biger series but posting them separately doesn't > make sense either as reviewer would have to jump between them anyways > to make sensible review. We'll find a way. As you want to have TYPE_VIRTIO_MEM specific handling code, this can go into the virtio-mem series. > >> preplug() can do basic checks but I don't think it should be used to >> change actual properties. And I give you the main reason for this: >> >> We have to do quite some amount of unnecessary error handling (please >> remember the pc_dimm plug code madness before I refactored it - 80% >> consisted of error handling) if we want to work on device properties >> before a device is realized. And we even have duplicate checks both in >> the realize() and the preplug() code then (again, what we had in the >> pc_dimm plug code - do we have a memdev already or not). >> >> Right now, I assume, that all MemoryDevice functions can be safely >> called after the device has been realized without error handling. This >> is nice. It e.g. guarantees that we have a memdev assigned. Otherwise, >> every time we access some MemoryDevice property (e.g. region size), we >> have to handle possible uninitialized properties (e.g. memdev) - >> something I don't like. >> >> So I want to avoid this by any means. And I don't really see a benefit >> for this additional error handling that will be necessary: We don't care >> about the performance in case something went wrong. >> > error checks are not really important here. > Here I care about keeping QOM model work as it supposed to, i.e. > object_new() -> set properties -> realize() > set properties should happen before realize() is called and > that's preplug callback. > > Currently setting properties is still in plug() handler > because preplug handler was introduced later and nobody was > rewriting that code to the extent you do. > /me regretting he started to touch that code I still don't think performing that change is wort it. As I said, we will need a lot of duplicate error handling "is memdev set or not" - while this is checked at one central place: when realizing. -- Thanks, David / dhildenb