[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH V5 02/23] physmem: qemu_ram_alloc_from_fd extensions
|
From: |
Peter Xu |
|
Subject: |
Re: [PATCH V5 02/23] physmem: qemu_ram_alloc_from_fd extensions |
|
Date: |
Thu, 2 Jan 2025 14:48:36 -0500 |
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.
--
Peter Xu