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
