[PATCH v4] hostmem-file: add offset option

Alexander Graf posted 1 patch 1 year, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230401124257.24537-1-graf@amazon.com
Maintainers: David Hildenbrand <david@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Peter Xu <peterx@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, "Daniel P. Berrangé" <berrange@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>
There is a newer version of this series
backends/hostmem-file.c | 40 +++++++++++++++++++++++++++++++++++++++-
include/exec/memory.h   |  2 ++
include/exec/ram_addr.h |  3 ++-
qapi/qom.json           |  5 +++++
qemu-options.hx         |  6 +++++-
softmmu/memory.c        |  3 ++-
softmmu/physmem.c       | 14 ++++++++++----
7 files changed, 65 insertions(+), 8 deletions(-)
[PATCH v4] hostmem-file: add offset option
Posted by Alexander Graf 1 year, 1 month ago
Add an option for hostmem-file to start the memory object at an offset
into the target file. This is useful if multiple memory objects reside
inside the same target file, such as a device node.

In particular, it's useful to map guest memory directly into /dev/mem
for experimentation.

Signed-off-by: Alexander Graf <graf@amazon.com>
Reviewed-by: Stefan Hajnoczi <stefanha@gmail.com>

---

v1 -> v2:

  - add qom documentation
  - propagate offset into truncate, size and alignment checks

v2 -> v3:

  - failed attempt at fixing typo

v2 -> v4:

  - fix typo
---
 backends/hostmem-file.c | 40 +++++++++++++++++++++++++++++++++++++++-
 include/exec/memory.h   |  2 ++
 include/exec/ram_addr.h |  3 ++-
 qapi/qom.json           |  5 +++++
 qemu-options.hx         |  6 +++++-
 softmmu/memory.c        |  3 ++-
 softmmu/physmem.c       | 14 ++++++++++----
 7 files changed, 65 insertions(+), 8 deletions(-)

diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index 25141283c4..38ea65bec5 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -27,6 +27,7 @@ struct HostMemoryBackendFile {
 
     char *mem_path;
     uint64_t align;
+    uint64_t offset;
     bool discard_data;
     bool is_pmem;
     bool readonly;
@@ -58,7 +59,8 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
     ram_flags |= fb->is_pmem ? RAM_PMEM : 0;
     memory_region_init_ram_from_file(&backend->mr, OBJECT(backend), name,
                                      backend->size, fb->align, ram_flags,
-                                     fb->mem_path, fb->readonly, errp);
+                                     fb->mem_path, fb->offset, fb->readonly,
+                                     errp);
     g_free(name);
 #endif
 }
@@ -125,6 +127,36 @@ static void file_memory_backend_set_align(Object *o, Visitor *v,
     fb->align = val;
 }
 
+static void file_memory_backend_get_offset(Object *o, Visitor *v,
+                                          const char *name, void *opaque,
+                                          Error **errp)
+{
+    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
+    uint64_t val = fb->offset;
+
+    visit_type_size(v, name, &val, errp);
+}
+
+static void file_memory_backend_set_offset(Object *o, Visitor *v,
+                                          const char *name, void *opaque,
+                                          Error **errp)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(o);
+    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
+    uint64_t val;
+
+    if (host_memory_backend_mr_inited(backend)) {
+        error_setg(errp, "cannot change property '%s' of %s", name,
+                   object_get_typename(o));
+        return;
+    }
+
+    if (!visit_type_size(v, name, &val, errp)) {
+        return;
+    }
+    fb->offset = val;
+}
+
 #ifdef CONFIG_LIBPMEM
 static bool file_memory_backend_get_pmem(Object *o, Error **errp)
 {
@@ -197,6 +229,12 @@ file_backend_class_init(ObjectClass *oc, void *data)
         file_memory_backend_get_align,
         file_memory_backend_set_align,
         NULL, NULL);
+    object_class_property_add(oc, "offset", "int",
+        file_memory_backend_get_offset,
+        file_memory_backend_set_offset,
+        NULL, NULL);
+    object_class_property_set_description(oc, "offset",
+        "Offset into the target file (ex: 1G)");
 #ifdef CONFIG_LIBPMEM
     object_class_property_add_bool(oc, "pmem",
         file_memory_backend_get_pmem, file_memory_backend_set_pmem);
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 15ade918ba..3b7295fbe2 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1318,6 +1318,7 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr,
  * @ram_flags: RamBlock flags. Supported flags: RAM_SHARED, RAM_PMEM,
  *             RAM_NORESERVE,
  * @path: the path in which to allocate the RAM.
+ * @offset: offset within the file referenced by path
  * @readonly: true to open @path for reading, false for read/write.
  * @errp: pointer to Error*, to store an error if it happens.
  *
@@ -1331,6 +1332,7 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
                                       uint64_t align,
                                       uint32_t ram_flags,
                                       const char *path,
+                                      ram_addr_t offset,
                                       bool readonly,
                                       Error **errp);
 
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index f4fb6a2111..90a8269290 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -110,6 +110,7 @@ long qemu_maxrampagesize(void);
  *  @ram_flags: RamBlock flags. Supported flags: RAM_SHARED, RAM_PMEM,
  *              RAM_NORESERVE.
  *  @mem_path or @fd: specify the backing file or device
+ *  @offset: Offset into target file
  *  @readonly: true to open @path for reading, false for read/write.
  *  @errp: pointer to Error*, to store an error if it happens
  *
@@ -119,7 +120,7 @@ long qemu_maxrampagesize(void);
  */
 RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
                                    uint32_t ram_flags, const char *mem_path,
-                                   bool readonly, Error **errp);
+                                   off_t offset, bool readonly, Error **errp);
 RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
                                  uint32_t ram_flags, int fd, off_t offset,
                                  bool readonly, Error **errp);
diff --git a/qapi/qom.json b/qapi/qom.json
index a877b879b9..bbb0664062 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -635,6 +635,10 @@
 #         specify the required alignment via this option.
 #         0 selects a default alignment (currently the page size). (default: 0)
 #
+# @offset: the offset into the target file that the region starts at. You can
+#          use this option to overload multiple regions into a single file.
+#          (default: 0)
+#
 # @discard-data: if true, the file contents can be destroyed when QEMU exits,
 #                to avoid unnecessarily flushing data to the backing file. Note
 #                that ``discard-data`` is only an optimization, and QEMU might
@@ -655,6 +659,7 @@
 { 'struct': 'MemoryBackendFileProperties',
   'base': 'MemoryBackendProperties',
   'data': { '*align': 'size',
+            '*offset': 'size',
             '*discard-data': 'bool',
             'mem-path': 'str',
             '*pmem': { 'type': 'bool', 'if': 'CONFIG_LIBPMEM' },
diff --git a/qemu-options.hx b/qemu-options.hx
index 59bdf67a2c..701cf39feb 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4859,7 +4859,7 @@ SRST
     they are specified. Note that the 'id' property must be set. These
     objects are placed in the '/objects' path.
 
-    ``-object memory-backend-file,id=id,size=size,mem-path=dir,share=on|off,discard-data=on|off,merge=on|off,dump=on|off,prealloc=on|off,host-nodes=host-nodes,policy=default|preferred|bind|interleave,align=align,readonly=on|off``
+    ``-object memory-backend-file,id=id,size=size,mem-path=dir,share=on|off,discard-data=on|off,merge=on|off,dump=on|off,prealloc=on|off,host-nodes=host-nodes,policy=default|preferred|bind|interleave,align=align,offset=offset,readonly=on|off``
         Creates a memory file backend object, which can be used to back
         the guest RAM with huge pages.
 
@@ -4929,6 +4929,10 @@ SRST
         such cases, users can specify the required alignment via this
         option.
 
+        The ``offset`` option specifies the offset into the target file
+        that the region starts at. You can use this parameter to overload
+        multiple regions into a single file.
+
         The ``pmem`` option specifies whether the backing file specified
         by ``mem-path`` is in host persistent memory that can be
         accessed using the SNIA NVM programming model (e.g. Intel
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 5305aca7ca..9f620085a0 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1601,6 +1601,7 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
                                       uint64_t align,
                                       uint32_t ram_flags,
                                       const char *path,
+                                      ram_addr_t offset,
                                       bool readonly,
                                       Error **errp)
 {
@@ -1612,7 +1613,7 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
     mr->destructor = memory_region_destructor_ram;
     mr->align = align;
     mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path,
-                                             readonly, &err);
+                                             offset, readonly, &err);
     if (err) {
         mr->size = int128_zero();
         object_unparent(OBJECT(mr));
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index e35061bba4..829c508b3b 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -1369,6 +1369,11 @@ static void *file_ram_alloc(RAMBlock *block,
         error_setg(errp, "alignment 0x%" PRIx64
                    " must be a power of two", block->mr->align);
         return NULL;
+    } else if (offset % block->page_size) {
+        error_setg(errp, "offset 0x%" PRIx64
+                   " must be multiples of page size 0x%zx",
+                   offset, block->page_size);
+        return NULL;
     }
     block->mr->align = MAX(block->page_size, block->mr->align);
 #if defined(__s390x__)
@@ -1400,7 +1405,7 @@ static void *file_ram_alloc(RAMBlock *block,
      * those labels. Therefore, extending the non-empty backend file
      * is disabled as well.
      */
-    if (truncate && ftruncate(fd, memory)) {
+    if (truncate && ftruncate(fd, offset + memory)) {
         perror("ftruncate");
     }
 
@@ -1889,7 +1894,7 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
 
     size = HOST_PAGE_ALIGN(size);
     file_size = get_file_size(fd);
-    if (file_size > 0 && file_size < size) {
+    if (file_size > offset && file_size < (offset + size)) {
         error_setg(errp, "backing store size 0x%" PRIx64
                    " does not match 'size' option 0x" RAM_ADDR_FMT,
                    file_size, size);
@@ -1929,7 +1934,7 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
 
 RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
                                    uint32_t ram_flags, const char *mem_path,
-                                   bool readonly, Error **errp)
+                                   off_t offset, bool readonly, Error **errp)
 {
     int fd;
     bool created;
@@ -1941,7 +1946,8 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
         return NULL;
     }
 
-    block = qemu_ram_alloc_from_fd(size, mr, ram_flags, fd, 0, readonly, errp);
+    block = qemu_ram_alloc_from_fd(size, mr, ram_flags, fd, offset, readonly,
+                                   errp);
     if (!block) {
         if (created) {
             unlink(mem_path);
-- 
2.32.0




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
Re: [PATCH v4] hostmem-file: add offset option
Posted by Stefan Hajnoczi 1 year, 1 month ago
On Sat, Apr 01, 2023 at 12:42:57PM +0000, Alexander Graf wrote:
> Add an option for hostmem-file to start the memory object at an offset
> into the target file. This is useful if multiple memory objects reside
> inside the same target file, such as a device node.
> 
> In particular, it's useful to map guest memory directly into /dev/mem
> for experimentation.
> 
> Signed-off-by: Alexander Graf <graf@amazon.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@gmail.com>
> 
> ---
> 
> v1 -> v2:
> 
>   - add qom documentation
>   - propagate offset into truncate, size and alignment checks
> 
> v2 -> v3:
> 
>   - failed attempt at fixing typo
> 
> v2 -> v4:
> 
>   - fix typo
> ---
>  backends/hostmem-file.c | 40 +++++++++++++++++++++++++++++++++++++++-
>  include/exec/memory.h   |  2 ++
>  include/exec/ram_addr.h |  3 ++-
>  qapi/qom.json           |  5 +++++
>  qemu-options.hx         |  6 +++++-
>  softmmu/memory.c        |  3 ++-
>  softmmu/physmem.c       | 14 ++++++++++----
>  7 files changed, 65 insertions(+), 8 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Re: [PATCH v4] hostmem-file: add offset option
Posted by David Hildenbrand 1 year, 1 month ago
On 01.04.23 19:47, Stefan Hajnoczi wrote:
> On Sat, Apr 01, 2023 at 12:42:57PM +0000, Alexander Graf wrote:
>> Add an option for hostmem-file to start the memory object at an offset
>> into the target file. This is useful if multiple memory objects reside
>> inside the same target file, such as a device node.
>>
>> In particular, it's useful to map guest memory directly into /dev/mem
>> for experimentation.
>>
>> Signed-off-by: Alexander Graf <graf@amazon.com>
>> Reviewed-by: Stefan Hajnoczi <stefanha@gmail.com>
>>
>> ---
>>
>> v1 -> v2:
>>
>>    - add qom documentation
>>    - propagate offset into truncate, size and alignment checks
>>
>> v2 -> v3:
>>
>>    - failed attempt at fixing typo
>>
>> v2 -> v4:
>>
>>    - fix typo
>> ---
>>   backends/hostmem-file.c | 40 +++++++++++++++++++++++++++++++++++++++-
>>   include/exec/memory.h   |  2 ++
>>   include/exec/ram_addr.h |  3 ++-
>>   qapi/qom.json           |  5 +++++
>>   qemu-options.hx         |  6 +++++-
>>   softmmu/memory.c        |  3 ++-
>>   softmmu/physmem.c       | 14 ++++++++++----
>>   7 files changed, 65 insertions(+), 8 deletions(-)
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

The change itself looks good to me, but I do think some other QEMU code 
that ends up working on the RAMBlock is not prepared yet. Most probably, 
because we never ended up using fd with an offset as guest RAM.

We don't seem to be remembering that offset in the RAMBlock. First, I 
thought block->offset would be used for that, but that's just the offset 
in the ram_addr_t space. Maybe we need a new "block->fd_offset" to 
remember the offset (unless I am missing something).

The real offset in the file would be required at least in two cases I 
can see (whenever we essentially end up calling mmap() on the fd again):

1) qemu_ram_remap(): We'd have to add the file offset on top of the 
calculated offset.

2) vhost-user: most probably whenever we set the mmap_offset. For 
example, in vhost_user_fill_set_mem_table_msg() we'd similarly have to 
add the file_offset on top of the calculated offset. 
vhost_user_get_mr_data() should most probably do that.

-- 
Thanks,

David / dhildenb
Re: [PATCH v4] hostmem-file: add offset option
Posted by Alexander Graf 1 year, 1 month ago
On 03.04.23 09:13, David Hildenbrand wrote:

>
> On 01.04.23 19:47, Stefan Hajnoczi wrote:
>> On Sat, Apr 01, 2023 at 12:42:57PM +0000, Alexander Graf wrote:
>>> Add an option for hostmem-file to start the memory object at an offset
>>> into the target file. This is useful if multiple memory objects reside
>>> inside the same target file, such as a device node.
>>>
>>> In particular, it's useful to map guest memory directly into /dev/mem
>>> for experimentation.
>>>
>>> Signed-off-by: Alexander Graf <graf@amazon.com>
>>> Reviewed-by: Stefan Hajnoczi <stefanha@gmail.com>
>>>
>>> ---
>>>
>>> v1 -> v2:
>>>
>>>    - add qom documentation
>>>    - propagate offset into truncate, size and alignment checks
>>>
>>> v2 -> v3:
>>>
>>>    - failed attempt at fixing typo
>>>
>>> v2 -> v4:
>>>
>>>    - fix typo
>>> ---
>>>   backends/hostmem-file.c | 40 +++++++++++++++++++++++++++++++++++++++-
>>>   include/exec/memory.h   |  2 ++
>>>   include/exec/ram_addr.h |  3 ++-
>>>   qapi/qom.json           |  5 +++++
>>>   qemu-options.hx         |  6 +++++-
>>>   softmmu/memory.c        |  3 ++-
>>>   softmmu/physmem.c       | 14 ++++++++++----
>>>   7 files changed, 65 insertions(+), 8 deletions(-)
>>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>
> The change itself looks good to me, but I do think some other QEMU code
> that ends up working on the RAMBlock is not prepared yet. Most probably,
> because we never ended up using fd with an offset as guest RAM.
>
> We don't seem to be remembering that offset in the RAMBlock. First, I
> thought block->offset would be used for that, but that's just the offset
> in the ram_addr_t space. Maybe we need a new "block->fd_offset" to
> remember the offset (unless I am missing something).
>
> The real offset in the file would be required at least in two cases I
> can see (whenever we essentially end up calling mmap() on the fd again):
>
> 1) qemu_ram_remap(): We'd have to add the file offset on top of the
> calculated offset.


This one is a bit tricky to test, as we're only running into that code 
path with KVM when we see an #MCE. But it's trivial, so I'm confident it 
will work as expected.


>
> 2) vhost-user: most probably whenever we set the mmap_offset. For
> example, in vhost_user_fill_set_mem_table_msg() we'd similarly have to
> add the file_offset on top of the calculated offset.
> vhost_user_get_mr_data() should most probably do that.


I agree - adding the offset as part of get_mr_data() is sufficient. I 
have validated it works correctly with QEMU's vhost-user-blk target.

I think the changes are still obvious enough that I'll fold them all 
into a single patch.


Alex





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


Re: [PATCH v4] hostmem-file: add offset option
Posted by David Hildenbrand 1 year, 1 month ago
>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>
>> The change itself looks good to me, but I do think some other QEMU code
>> that ends up working on the RAMBlock is not prepared yet. Most probably,
>> because we never ended up using fd with an offset as guest RAM.
>>
>> We don't seem to be remembering that offset in the RAMBlock. First, I
>> thought block->offset would be used for that, but that's just the offset
>> in the ram_addr_t space. Maybe we need a new "block->fd_offset" to
>> remember the offset (unless I am missing something).
>>
>> The real offset in the file would be required at least in two cases I
>> can see (whenever we essentially end up calling mmap() on the fd again):
>>
>> 1) qemu_ram_remap(): We'd have to add the file offset on top of the
>> calculated offset.
> 
> 
> This one is a bit tricky to test, as we're only running into that code
> path with KVM when we see an #MCE. But it's trivial, so I'm confident it
> will work as expected.
> 

Indeed.

> 
>>
>> 2) vhost-user: most probably whenever we set the mmap_offset. For
>> example, in vhost_user_fill_set_mem_table_msg() we'd similarly have to
>> add the file_offset on top of the calculated offset.
>> vhost_user_get_mr_data() should most probably do that.
> 
> 
> I agree - adding the offset as part of get_mr_data() is sufficient. I
> have validated it works correctly with QEMU's vhost-user-blk target.
> 
> I think the changes are still obvious enough that I'll fold them all
> into a single patch.

Most probably good enough. Having the offset part separately as a fix 
for ed5d001916 ("multi-process: setup memory manager for remote device") 
could be beneficial, though.

Thanks Alex!

-- 
Thanks,

David / dhildenb
Re: [PATCH v4] hostmem-file: add offset option
Posted by Peter Xu 1 year, 1 month ago
On Mon, Apr 03, 2023 at 09:13:29AM +0200, David Hildenbrand wrote:
> On 01.04.23 19:47, Stefan Hajnoczi wrote:
> > On Sat, Apr 01, 2023 at 12:42:57PM +0000, Alexander Graf wrote:
> > > Add an option for hostmem-file to start the memory object at an offset
> > > into the target file. This is useful if multiple memory objects reside
> > > inside the same target file, such as a device node.
> > > 
> > > In particular, it's useful to map guest memory directly into /dev/mem
> > > for experimentation.
> > > 
> > > Signed-off-by: Alexander Graf <graf@amazon.com>
> > > Reviewed-by: Stefan Hajnoczi <stefanha@gmail.com>
> > > 
> > > ---
> > > 
> > > v1 -> v2:
> > > 
> > >    - add qom documentation
> > >    - propagate offset into truncate, size and alignment checks
> > > 
> > > v2 -> v3:
> > > 
> > >    - failed attempt at fixing typo
> > > 
> > > v2 -> v4:
> > > 
> > >    - fix typo
> > > ---
> > >   backends/hostmem-file.c | 40 +++++++++++++++++++++++++++++++++++++++-
> > >   include/exec/memory.h   |  2 ++
> > >   include/exec/ram_addr.h |  3 ++-
> > >   qapi/qom.json           |  5 +++++
> > >   qemu-options.hx         |  6 +++++-
> > >   softmmu/memory.c        |  3 ++-
> > >   softmmu/physmem.c       | 14 ++++++++++----
> > >   7 files changed, 65 insertions(+), 8 deletions(-)
> > 
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> The change itself looks good to me, but I do think some other QEMU code that
> ends up working on the RAMBlock is not prepared yet. Most probably, because
> we never ended up using fd with an offset as guest RAM.
> 
> We don't seem to be remembering that offset in the RAMBlock. First, I
> thought block->offset would be used for that, but that's just the offset in
> the ram_addr_t space. Maybe we need a new "block->fd_offset" to remember the
> offset (unless I am missing something).

I think you're right.

> 
> The real offset in the file would be required at least in two cases I can
> see (whenever we essentially end up calling mmap() on the fd again):
> 
> 1) qemu_ram_remap(): We'd have to add the file offset on top of the
> calculated offset.
> 
> 2) vhost-user: most probably whenever we set the mmap_offset. For example,
> in vhost_user_fill_set_mem_table_msg() we'd similarly have to add the
> file_offset on top of the calculated offset. vhost_user_get_mr_data() should
> most probably do that.

I had a patch to add that offset for the upcoming doublemap feature here:

https://lore.kernel.org/all/20230117220914.2062125-8-peterx@redhat.com/

But that was because doublemap wants to map the guest mem twice for other
purposes. I didn't yet notice that the code seem to be already broken if
without offset==0.

While, I _think_ we already have offset!=0 case for a ramblock, since:

        commit ed5d001916dd46ceed6d8850e453bcd7b5db2acb
        Author: Jagannathan Raman <jag.raman@oracle.com>
        Date:   Fri Jan 29 11:46:13 2021 -0500

        multi-process: setup memory manager for remote device

Where there's:

        memory_region_init_ram_from_fd(subregion, NULL,
                                       name, sysmem_info->sizes[region],
                                       RAM_SHARED, msg->fds[region],
                                       sysmem_info->offsets[region],
                                       errp);

Thanks,

-- 
Peter Xu
Re: [PATCH v4] hostmem-file: add offset option
Posted by David Hildenbrand 1 year, 1 month ago
On 03.04.23 17:49, Peter Xu wrote:
> On Mon, Apr 03, 2023 at 09:13:29AM +0200, David Hildenbrand wrote:
>> On 01.04.23 19:47, Stefan Hajnoczi wrote:
>>> On Sat, Apr 01, 2023 at 12:42:57PM +0000, Alexander Graf wrote:
>>>> Add an option for hostmem-file to start the memory object at an offset
>>>> into the target file. This is useful if multiple memory objects reside
>>>> inside the same target file, such as a device node.
>>>>
>>>> In particular, it's useful to map guest memory directly into /dev/mem
>>>> for experimentation.
>>>>
>>>> Signed-off-by: Alexander Graf <graf@amazon.com>
>>>> Reviewed-by: Stefan Hajnoczi <stefanha@gmail.com>
>>>>
>>>> ---
>>>>
>>>> v1 -> v2:
>>>>
>>>>     - add qom documentation
>>>>     - propagate offset into truncate, size and alignment checks
>>>>
>>>> v2 -> v3:
>>>>
>>>>     - failed attempt at fixing typo
>>>>
>>>> v2 -> v4:
>>>>
>>>>     - fix typo
>>>> ---
>>>>    backends/hostmem-file.c | 40 +++++++++++++++++++++++++++++++++++++++-
>>>>    include/exec/memory.h   |  2 ++
>>>>    include/exec/ram_addr.h |  3 ++-
>>>>    qapi/qom.json           |  5 +++++
>>>>    qemu-options.hx         |  6 +++++-
>>>>    softmmu/memory.c        |  3 ++-
>>>>    softmmu/physmem.c       | 14 ++++++++++----
>>>>    7 files changed, 65 insertions(+), 8 deletions(-)
>>>
>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>
>> The change itself looks good to me, but I do think some other QEMU code that
>> ends up working on the RAMBlock is not prepared yet. Most probably, because
>> we never ended up using fd with an offset as guest RAM.
>>
>> We don't seem to be remembering that offset in the RAMBlock. First, I
>> thought block->offset would be used for that, but that's just the offset in
>> the ram_addr_t space. Maybe we need a new "block->fd_offset" to remember the
>> offset (unless I am missing something).
> 
> I think you're right.
> 
>>
>> The real offset in the file would be required at least in two cases I can
>> see (whenever we essentially end up calling mmap() on the fd again):
>>
>> 1) qemu_ram_remap(): We'd have to add the file offset on top of the
>> calculated offset.
>>
>> 2) vhost-user: most probably whenever we set the mmap_offset. For example,
>> in vhost_user_fill_set_mem_table_msg() we'd similarly have to add the
>> file_offset on top of the calculated offset. vhost_user_get_mr_data() should
>> most probably do that.
> 
> I had a patch to add that offset for the upcoming doublemap feature here:
> 
> https://lore.kernel.org/all/20230117220914.2062125-8-peterx@redhat.com/
> 
> But that was because doublemap wants to map the guest mem twice for other
> purposes. I didn't yet notice that the code seem to be already broken if
> without offset==0.
> 
> While, I _think_ we already have offset!=0 case for a ramblock, since:
> 
>          commit ed5d001916dd46ceed6d8850e453bcd7b5db2acb
>          Author: Jagannathan Raman <jag.raman@oracle.com>
>          Date:   Fri Jan 29 11:46:13 2021 -0500
> 
>          multi-process: setup memory manager for remote device
> 
> Where there's:
> 
>          memory_region_init_ram_from_fd(subregion, NULL,
>                                         name, sysmem_info->sizes[region],
>                                         RAM_SHARED, msg->fds[region],
>                                         sysmem_info->offsets[region],
>                                         errp);

Interesting ... maybe so far never used alongside vhost-user.

-- 
Thanks,

David / dhildenb