[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 3/3] hostmem-file: make option 'size' optiona

From: Haozhong Zhang
Subject: Re: [Qemu-devel] [PATCH v2 3/3] hostmem-file: make option 'size' optional
Date: Fri, 28 Oct 2016 13:57:08 +0800
User-agent: NeoMutt/20161014 (1.7.1)

On 10/28/16 10:06 +0800, Haozhong Zhang wrote:
On 10/27/16 12:55 -0200, Eduardo Habkost wrote:
On Thu, Oct 27, 2016 at 12:23:00PM +0800, Haozhong Zhang wrote:
static char *get_mem_path(Object *o, Error **errp)
diff --git a/exec.c b/exec.c
index 264a25f..89065bd 100644
--- a/exec.c
+++ b/exec.c
@@ -1234,7 +1234,7 @@ static int64_t get_file_size(int fd)

static void *file_ram_alloc(RAMBlock *block,
-                            ram_addr_t memory,
+                            ram_addr_t *memory,
                            const char *path,
                            Error **errp)
@@ -1245,6 +1245,7 @@ static void *file_ram_alloc(RAMBlock *block,
    void *area = MAP_FAILED;
    int fd = -1;
    int64_t file_size;
+    ram_addr_t mem_size = *memory;

    if (kvm_enabled() && !kvm_has_sync_mmu()) {
@@ -1309,21 +1310,27 @@ static void *file_ram_alloc(RAMBlock *block,

    file_size = get_file_size(fd);

-    if (memory < block->page_size) {
+    if (!mem_size && file_size > 0) {
+        mem_size = file_size;

Maybe we should set *memory here and not below?

Qemu currently sets the memory region size to the file size, and block
length to the aligned file size, so the code here can be changed as below:

          memory_region_set_size(block->mr, mem_size);
          mem_size = HOST_PAGE_ALIGN(mem_size);
          *memory = mem_size;

The second line is necessary because Qemu currently passes the aligned
file size to file_ram_alloc().

I meant that QEMU currently sets the memory region size of the 'size'
option and the block length to the aligned 'size' option. If the file
size is used when 'size' option is not specified, QEMU should do the
same thing.


+        memory_region_set_size(block->mr, mem_size);

This could be simplified (and made safer) if the memory region
got initialized by memory_region_init_ram_from_file() after we
already mapped/allocated the file (so we avoid surprises in case
other code does something weird because of the temporarily
zero-sized MemoryRegion). But it would probably be an intrusive
change, so I guess changing the memory size here is OK. Paolo,
what do you think?

I will add a check to ensure that the size of block->mr is either 0 or
mem_size, and only set the region size in the former case. The former
corresponds to the case that 'size' option is not given or 0. The
latter corresponds to the case that 'size' option is given.

+    }
+    if (mem_size < block->page_size) {
        error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "
                   "or larger than page size 0x%zx",
-                   memory, block->page_size);
+                   mem_size, block->page_size);
        goto error;

-    if (file_size > 0 && file_size < memory) {
+    if (file_size > 0 && file_size < mem_size) {
        error_setg(errp, "backing store %s size %"PRId64
                   " does not match 'size' option %"PRIu64,
-                   path, file_size, memory);
+                   path, file_size, mem_size);
        goto error;

-    memory = ROUND_UP(memory, block->page_size);
+    mem_size = ROUND_UP(mem_size, block->page_size);
+    *memory = mem_size;

Why exactly did you choose to set *memory to the rounded-up size
and not file_size? Changing *memory to the rounded-up value would
be additional behavior that is not described in the commit
message. I believe we should change *memory only if *memory == 0,
and avoid touching it otherwise.

Setting *memory here is buggy. I'll move it as above.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]