Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[David Hildenbrand]

On 13.02.20 21:56, Peter Xu wrote:
> On Thu, Feb 13, 2020 at 08:42:23PM +0100, David Hildenbrand wrote:
>> On 13.02.20 19:32, Peter Xu wrote:
>>> On Thu, Feb 13, 2020 at 06:20:16PM +0100, David Hildenbrand wrote:
>>>> Resizing while migrating is dangerous and does not work as expected.
>>>> The whole migration code works on the usable_length of ram blocks and does
>>>> not expect this to change at random points in time.
>>>>
>>>> Precopy: The ram block size must not change on the source, after
>>>> ram_save_setup(), so as long as the guest is still running on the source.
>>>>
>>>> Postcopy: The ram block size must not change on the target, after
>>>> synchronizing the RAM block list (ram_load_precopy()).
>>>>
>>>> AFAIKS, resizing can be trigger *after* (but not during) a reset in
>>>> ACPI code by the guest
>>>> - hw/arm/virt-acpi-build.c:acpi_ram_update()
>>>> - hw/i386/acpi-build.c:acpi_ram_update()
>>>
>>> What can be the pre-condition of triggering this after reset?  I'm
>>> thinking whether QEMU can be aware of this "resizing can happen"
>>> condition, then we could simply stop the migration from happening even
>>> before the resizing happens.  Thanks,
>>
>> I think the condition is not known before the guest actually tries to
>> read the relevant memory areas (which trigger the rebuilt+resize, and
>> AFAIK, the new size depends on fw config done by the guest after the
>> reset). So it's hard to "predict".
> 
> I chimmed in without much context, sorry if I'm going to ask naive
> questions. :)

I think the problem is quite involved and not obvious, so there are no
naive questions :)

> 
> What I was asking is about why the resizing can happen.  A quick read
> told me that it was majorly for easier extension of ROMs (firmware
> updates?).  With that, I'm imaging a common case for memory
> resizing...
> 
>   (1) Source QEMU runs VM on old host, with old firmware
> 
>   (2) Migrate source QEMU to destination new host, with new and bigger
>       firmware
> 
>   (3) During the migration, the ROM size on the destination will still
>       be the old, referring to ram_load_precopy(), as long as no
>       system reset
> 
>   (4) After migration finished, when the system reboots, memory
>       resizing happens with the new and bigger firmware loaded

AFAIK it could trigger

a) In precopy during the second migration.
b) In postcopy during the first migration.

> 
> And is this patch trying to fix/warn when there's a reboot during (3)
> so the new size is discovered at a wrong time?  Is my understanding
> correct?

It's trying to bail out early instead of failing at other random points
(with an unclear outcome).

>>
>> In the precopy case it would be easier to abort (although, not simple
>> AFAIKS), in the postcopy not so easy - because you're already partially
>> running on the migration target.
> 
> Prior to this patch, would a precopy still survive with such an
> accident (asked because I _feel_ like migrating a ramblock with
> smaller used_length to the same ramblock with bigger used_length seems
> to be fine?)?  Or we can stop the precopy and restart.  After this

I assume growing the region is the usual case (not shrinking). FW blobs
tend to get bigger.

Migrating while growing a ram block on the source won't work. The source
would try to send a dirt page that's outside of the used_length on the
target, making e.g., ram_load_postcopy()/ram_load_precopy() fail with
"Illegal RAM offset...".

In the postcopy case, e.g., ram_dirty_bitmap_reload() will fail in case
there is a mismatch between ram block size on source/target.

Another issue is if the used_length changes while in ram_save_setup(),
just between storing ram_bytes_total_common(true) and storing
block->used_length. A mismatch will screw up the migration stream.

But these are just the immediately visible issues. I am more concerned
about used_length changing at random points in time, resulting in more
harm. (e.g., non-obvious load-store tearing when accessing the used length)

Migration code is inherently racy when it comes to ram block resizes.
And that might become more dangerous once we want to size the migration
bitmaps smaller (used_length instead of max_length) or disallow access
to ram blocks beyond the used_length. Both are things I am working on :)

> patch, it'll crash the source VM (&error_abort specified in
> memory_region_ram_resize()), which seems a bit more harsh?

There seems to be no easy way to abort migration from outside the
migration thread. As Juan said, you actually don't want to fail
migration but instead soft-abort migration and continue running the
guest on the target on a reset. But that's not easy as well.

One could think about extending ram block notifiers to notify migration
code (before) resizes, so that migration code can work around the
resize (how is TBD). Not easy as well :)

But then, I am not sure
a) If we run into this issue in real life a lot.
b) If we actually need an elaborate solutions within QEMU to handle this
case. For now, it's sufficient to restart the VM on the migration
target. No data was lost. Not nice, but very simple.

Thanks!

-- 
Thanks,

David / dhildenb