Re: [PATCH V2 05/13] physmem: preserve ram blocks for cpr

[Steven Sistare <steven.sistare@oracle.com>]

On 10/7/2024 12:28 PM, Peter Xu wrote:
> On Mon, Oct 07, 2024 at 11:49:25AM -0400, Peter Xu wrote:
>> On Mon, Sep 30, 2024 at 12:40:36PM -0700, Steve Sistare wrote:
>>> Save the memfd for anonymous ramblocks in CPR state, along with a name
>>> that uniquely identifies it.  The block's idstr is not yet set, so it
>>> cannot be used for this purpose.  Find the saved memfd in new QEMU when
>>> creating a block.  QEMU hard-codes the length of some internally-created
>>> blocks, so to guard against that length changing, use lseek to get the
>>> actual length of an incoming memfd.
>>>
>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>> ---
>>>   system/physmem.c | 25 ++++++++++++++++++++++++-
>>>   1 file changed, 24 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/system/physmem.c b/system/physmem.c
>>> index 174f7e0..ddbeec9 100644
>>> --- a/system/physmem.c
>>> +++ b/system/physmem.c
>>> @@ -72,6 +72,7 @@
>>>   
>>>   #include "qapi/qapi-types-migration.h"
>>>   #include "migration/options.h"
>>> +#include "migration/cpr.h"
>>>   #include "migration/vmstate.h"
>>>   
>>>   #include "qemu/range.h"
>>> @@ -1663,6 +1664,19 @@ void qemu_ram_unset_idstr(RAMBlock *block)
>>>       }
>>>   }
>>>   
>>> +static char *cpr_name(RAMBlock *block)
>>> +{
>>> +    MemoryRegion *mr = block->mr;
>>> +    const char *mr_name = memory_region_name(mr);
>>> +    g_autofree char *id = mr->dev ? qdev_get_dev_path(mr->dev) : NULL;
>>> +
>>> +    if (id) {
>>> +        return g_strdup_printf("%s/%s", id, mr_name);
>>> +    } else {
>>> +        return g_strdup(mr_name);
>>> +    }
>>> +}
>>> +
>>>   size_t qemu_ram_pagesize(RAMBlock *rb)
>>>   {
>>>       return rb->page_size;
>>> @@ -1858,14 +1872,18 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
>>>                                           TYPE_MEMORY_BACKEND)) {
>>>               size_t max_length = new_block->max_length;
>>>               MemoryRegion *mr = new_block->mr;
>>> -            const char *name = memory_region_name(mr);
>>> +            g_autofree char *name = cpr_name(new_block);
>>>   
>>>               new_block->mr->align = QEMU_VMALLOC_ALIGN;
>>>               new_block->flags |= RAM_SHARED;
>>> +            new_block->fd = cpr_find_fd(name, 0);
>>>   
>>>               if (new_block->fd == -1) {
>>>                   new_block->fd = qemu_memfd_create(name, max_length + mr->align,
>>>                                                     0, 0, 0, errp);
>>> +                cpr_save_fd(name, 0, new_block->fd);
>>> +            } else {
>>> +                new_block->max_length = lseek(new_block->fd, 0, SEEK_END);
>>
>> So this can overwrite the max_length that the caller specified..
>>
>> I remember we used to have some tricks on specifying different max_length
>> for ROMs on dest QEMU (on which, qemu firmwares also upgraded on the dest
>> host so the size can be bigger than src qemu's old ramblocks), so that the
>> MR is always large enough to reload even the new firmwares, while migration
>> only migrates the smaller size (used_length) so it's fine as we keep the
>> extra sizes empty. I think that can relevant to the qemu_ram_resize() call
>> of parse_ramblock().

Yes, resizable ram block for firmware blob is the only case I know of where
the length changed in the past.  If a length changes in the future, we will
need to detect and accommodate that change here, and I believe the fix will
be to simply use the actual length, as per the code above.  But if you prefer,
for now I can check for length change and return an error. New qemu will fail
to start, and old qemu will recover.

>> The reload will not happen until some point, perhaps system resets.  I
>> wonder whether that is an issue in this case.

Firmware is only generated once, via this path on x86:
   qmp_x_exit_preconfig
     qemu_machine_creation_done
       qdev_machine_creation_done
         pc_machine_done
           acpi_setup
             acpi_add_rom_blob
               rom_add_blob
                 rom_set_mr

After a system reset, the ramblock contents from memory are used as-is.

> PS: If this is needed by CPR-transfer only because mmap() later can fail
> due to a bigger max_length, 

That is the reason.  IMO adjusting max_length is more robust than fiddling
with truncate and pretending that max_length is larger, when qemu will never
be able to use the phantom space up to max_length.

- Steve

> I wonder whether it can be fixed by passing
> truncate=true in the upcoming file_ram_alloc(), rather than overwritting
> the max_length value itself.
> 
>>
>>>               }
>>>   
>>>               if (new_block->fd >= 0) {
>>> @@ -1875,6 +1893,7 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
>>>                                                    false, 0, errp);
>>>               }
>>>               if (!new_block->host) {
>>> +                cpr_delete_fd(name, 0);
>>>                   qemu_mutex_unlock_ramlist();
>>>                   return;
>>>               }
>>> @@ -2182,6 +2201,8 @@ static void reclaim_ramblock(RAMBlock *block)
>>>   
>>>   void qemu_ram_free(RAMBlock *block)
>>>   {
>>> +    g_autofree char *name = NULL;
>>> +
>>>       if (!block) {
>>>           return;
>>>       }
>>> @@ -2192,6 +2213,8 @@ void qemu_ram_free(RAMBlock *block)
>>>       }
>>>   
>>>       qemu_mutex_lock_ramlist();
>>> +    name = cpr_name(block);
>>> +    cpr_delete_fd(name, 0);
>>>       QLIST_REMOVE_RCU(block, next);
>>>       ram_list.mru_block = NULL;
>>>       /* Write list before version */
>>> -- 
>>> 1.8.3.1
>>>
>>
>> -- 
>> Peter Xu
>