Re: [PATCH V5 02/23] physmem: qemu_ram_alloc_from_fd extensions

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

On 1/2/2025 2:48 PM, Peter Xu wrote:
> On Thu, Jan 02, 2025 at 01:36:01PM -0500, Steven Sistare wrote:
>> On 12/24/2024 12:18 PM, Peter Xu wrote:
>>> On Tue, Dec 24, 2024 at 08:16:47AM -0800, Steve Sistare wrote:
>>>> Extend qemu_ram_alloc_from_fd to support resizable ram, and define
>>>> qemu_ram_resize_cb to clean up the API.
>>>>
>>>> Add a grow parameter to extend the file if necessary.  However, if
>>>> grow is false, a zero-sized file is always extended.
>>>>
>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>>> ---
>>>>    include/exec/ram_addr.h | 13 +++++++++----
>>>>    system/memory.c         |  4 ++--
>>>>    system/physmem.c        | 35 ++++++++++++++++++++---------------
>>>>    3 files changed, 31 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
>>>> index ff157c1..94bb3cc 100644
>>>> --- a/include/exec/ram_addr.h
>>>> +++ b/include/exec/ram_addr.h
>>>> @@ -111,23 +111,30 @@ long qemu_maxrampagesize(void);
>>>>     *
>>>>     * Parameters:
>>>>     *  @size: the size in bytes of the ram block
>>>> + *  @max_size: the maximum size of the block after resizing
>>>>     *  @mr: the memory region where the ram block is
>>>> + *  @resized: callback after calls to qemu_ram_resize
>>>>     *  @ram_flags: RamBlock flags. Supported flags: RAM_SHARED, RAM_PMEM,
>>>>     *              RAM_NORESERVE, RAM_PROTECTED, RAM_NAMED_FILE, RAM_READONLY,
>>>>     *              RAM_READONLY_FD, RAM_GUEST_MEMFD
>>>>     *  @mem_path or @fd: specify the backing file or device
>>>>     *  @offset: Offset into target file
>>>> + *  @grow: extend file if necessary (but an empty file is always extended).
>>>>     *  @errp: pointer to Error*, to store an error if it happens
>>>>     *
>>>>     * Return:
>>>>     *  On success, return a pointer to the ram block.
>>>>     *  On failure, return NULL.
>>>>     */
>>>> +typedef void (*qemu_ram_resize_cb)(const char *, uint64_t length, void *host);
>>>> +
>>>>    RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
>>>>                                       uint32_t ram_flags, const char *mem_path,
>>>>                                       off_t offset, Error **errp);
>>>> -RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
>>>> +RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, ram_addr_t max_size,
>>>> +                                 qemu_ram_resize_cb resized, MemoryRegion *mr,
>>>>                                     uint32_t ram_flags, int fd, off_t offset,
>>>> +                                 bool grow,
>>>>                                     Error **errp);
>>>>    RAMBlock *qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
>>>> @@ -135,9 +142,7 @@ RAMBlock *qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
>>>>    RAMBlock *qemu_ram_alloc(ram_addr_t size, uint32_t ram_flags, MemoryRegion *mr,
>>>>                             Error **errp);
>>>>    RAMBlock *qemu_ram_alloc_resizeable(ram_addr_t size, ram_addr_t max_size,
>>>> -                                    void (*resized)(const char*,
>>>> -                                                    uint64_t length,
>>>> -                                                    void *host),
>>>> +                                    qemu_ram_resize_cb resized,
>>>>                                        MemoryRegion *mr, Error **errp);
>>>>    void qemu_ram_free(RAMBlock *block);
>>>> diff --git a/system/memory.c b/system/memory.c
>>>> index 78e17e0..290c522 100644
>>>> --- a/system/memory.c
>>>> +++ b/system/memory.c
>>>> @@ -1680,8 +1680,8 @@ bool memory_region_init_ram_from_fd(MemoryRegion *mr,
>>>>        mr->readonly = !!(ram_flags & RAM_READONLY);
>>>>        mr->terminates = true;
>>>>        mr->destructor = memory_region_destructor_ram;
>>>> -    mr->ram_block = qemu_ram_alloc_from_fd(size, mr, ram_flags, fd, offset,
>>>> -                                           &err);
>>>> +    mr->ram_block = qemu_ram_alloc_from_fd(size, size, NULL, mr, ram_flags, fd,
>>>> +                                           offset, false, &err);
>>>>        if (err) {
>>>>            mr->size = int128_zero();
>>>>            object_unparent(OBJECT(mr));
>>>> diff --git a/system/physmem.c b/system/physmem.c
>>>> index c76503a..48c544f 100644
>>>> --- a/system/physmem.c
>>>> +++ b/system/physmem.c
>>>> @@ -1942,8 +1942,10 @@ out_free:
>>>>    }
>>>>    #ifdef CONFIG_POSIX
>>>> -RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
>>>> +RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, ram_addr_t max_size,
>>>> +                                 qemu_ram_resize_cb resized, MemoryRegion *mr,
>>>>                                     uint32_t ram_flags, int fd, off_t offset,
>>>> +                                 bool grow,
>>>>                                     Error **errp)
>>>>    {
>>>>        RAMBlock *new_block;
>>>> @@ -1953,7 +1955,9 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
>>>>        /* Just support these ram flags by now. */
>>>>        assert((ram_flags & ~(RAM_SHARED | RAM_PMEM | RAM_NORESERVE |
>>>>                              RAM_PROTECTED | RAM_NAMED_FILE | RAM_READONLY |
>>>> -                          RAM_READONLY_FD | RAM_GUEST_MEMFD)) == 0);
>>>> +                          RAM_READONLY_FD | RAM_GUEST_MEMFD |
>>>> +                          RAM_RESIZEABLE)) == 0);
>>>> +    assert(max_size >= size);
>>>>        if (xen_enabled()) {
>>>>            error_setg(errp, "-mem-path not supported with Xen");
>>>> @@ -1968,12 +1972,14 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
>>>>        size = TARGET_PAGE_ALIGN(size);
>>>>        size = REAL_HOST_PAGE_ALIGN(size);
>>>> +    max_size = TARGET_PAGE_ALIGN(max_size);
>>>> +    max_size = REAL_HOST_PAGE_ALIGN(max_size);
>>>>        file_size = get_file_size(fd);
>>>> -    if (file_size > offset && file_size < (offset + size)) {
>>>> +    if (file_size && file_size < offset + max_size && !grow) {
>>>
>>> Is this a bugfix for the case offset < fsize?  If so, better make it a
>>> small patch and copy stable..
>>>
>>> $ touch ramfile
>>> $ truncate -s 64M ramfile
>>> $ ./qemu-system-x86_64 -object memory-backend-file,mem-path=./ramfile,offset=128M,size=128M,id=mem1,prealloc=on
>>> qemu-system-x86_64: qemu_prealloc_mem: preallocating memory failed: Bad address
>>>
>>> So yes, it's a bug..
>>
>> Yes, it's a bug I noticed by inspection.
>> I will split and submit to stable.
> 
> Thanks.
> 
>>
>>>>            error_setg(errp, "backing store size 0x%" PRIx64
>>>>                       " does not match 'size' option 0x" RAM_ADDR_FMT,
>>>> -                   file_size, size);
>>>> +                   file_size, max_size);
>>>>            return NULL;
>>>>        }
>>>> @@ -1988,11 +1994,13 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
>>>>        new_block = g_malloc0(sizeof(*new_block));
>>>>        new_block->mr = mr;
>>>>        new_block->used_length = size;
>>>> -    new_block->max_length = size;
>>>> +    new_block->max_length = max_size;
>>>> +    new_block->resized = resized;
>>>>        new_block->flags = ram_flags;
>>>>        new_block->guest_memfd = -1;
>>>> -    new_block->host = file_ram_alloc(new_block, size, fd, !file_size, offset,
>>>> -                                     errp);
>>>> +    new_block->host = file_ram_alloc(new_block, max_size, fd,
>>>> +                                     file_size < offset + max_size,
>>>
>>> Same here, looks like relevant to above.
>>
>> This line would not be part of the fix for stable.  The pre-cpr code should only
>> truncate (allocate) if !file_size.  If file_size > 0, the fixed conditional above
>> verifies that file_size is large enough.
>>
>> The fix will be a 1-liner:
>>   -    if (file_size > offset && file_size < (offset + size)) {
>>   +    if (file_size && file_size < offset + size) {
> 
> Indeed, this should work.

I will also fix the error message, which should have been updated when
the offset option was added.  With the fix and your test program,
it says:
    backing store size 0x4000000 does not match 'size' option 0x8000000

I will change it to
   backing store size 0x4000000 is too small for 'size' option 0x8000000
   plus 'offset' option 0x8000000

- Steve