[Qemu-devel] [PATCH v3 0/2] future proof rom loading for cross versiom migration

[Qemu-devel] [PATCH v3 0/2] future proof rom loading for cross versiom migration

[Michael S. Tsirkin]

Changes from v2: address comments on v2 by Peter Maydell
    - switch from global constant to function
    - use memory_region_init_ram instead of _ram_ptr
    - disable for 1.6

Changes from v1: address comments by Peter Maydell
    - drop useless data=data line
    - rename target_page_size to migration_page_size to make use clear
    Peter, you also suggested somehow hiding this within memory core.
    I don't see a clean way to do this without lots of code
    changes, I think what I propose here is acceptable for now
    and we can always rework APIs without wire format changes.

Please review, and consider for merging.

Original cover letter below.


ROM files that are put in FW CFG are copied to guest ram, by BIOS, but
they are not backed by RAM so they don't get migrated.

Each time we'll change at least two bytes in such a ROM this will break
cross-version migration: since we can migrate after BIOS has read the first
byte but before it has read the second one, getting an inconsistent state.

This patchset makes QEMU future-proof against such changes.

Naturally, this only helps for -M 1.6 and up, older machine types
will still have the cross-version migration bug.

I think this should be applied for 1.6, this way we won't
have this problem from 1.7 and on.

Michael S. Tsirkin (2):
  memory: export migration page size
  loader: put FW CFG ROM files into RAM

 arch_init.c           |  6 ++++++
 hw/core/loader.c      | 54 ++++++++++++++++++++++++++++++++++++++++++++++++---
 hw/i386/pc_piix.c     |  2 ++
 hw/i386/pc_q35.c      |  2 ++
 include/exec/memory.h |  1 +
 include/hw/loader.h   |  1 +
 6 files changed, 63 insertions(+), 3 deletions(-)

-- 
MST

[Qemu-devel] [PATCH v3 1/2] memory: export migration page size

[Michael S. Tsirkin]

Migration code assumes that each RAM block is a multiple of target page
size.
We can fix this in a variety of ways, the simplest way is
exporting the required page size so callers can make regions
large enough.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 arch_init.c           | 6 ++++++
 include/exec/memory.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/arch_init.c b/arch_init.c
index 68a7ab7..c62778f 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -150,6 +150,12 @@ int qemu_read_default_config_files(bool userconfig)
     return 0;
 }
 
+/* Smallest page size for migrated RAM. */
+uint64_t qemu_migration_page_size(void)
+{
+    return TARGET_PAGE_SIZE;
+}
+
 static inline bool is_zero_page(uint8_t *p)
 {
     return buffer_find_nonzero_offset(p, TARGET_PAGE_SIZE) ==
diff --git a/include/exec/memory.h b/include/exec/memory.h
index ebe0d24..6ffffa2 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1055,6 +1055,7 @@ void *address_space_map(AddressSpace *as, hwaddr addr,
 void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
                          int is_write, hwaddr access_len);
 
+extern uint64_t qemu_migration_page_size(void);
 
 #endif
 
-- 
MST

[Qemu-devel] [PATCH v3 2/2] loader: put FW CFG ROM files into RAM

[Michael S. Tsirkin]

ROM files that are put in FW CFG are copied to guest ram, by BIOS, but
they are not backed by RAM so they don't get migrated.

Each time we change two bytes in such a ROM this breaks cross-version
migration: since we can migrate after BIOS has read the first byte but
before it has read the second one, getting an inconsistent state.

Future-proof this by creating, for each such ROM,
an MR serving as the backing store.
This MR is never mapped into guest memory, but it's registered
as RAM so it's migrated with the guest.

Naturally, this only helps for -M 1.7 and up, older machine types
will still have the cross-version migration bug.
Luckily the race window for the problem to trigger is very small,
which is also likely why we didn't notice the cross-version
migration bug in testing yet.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/core/loader.c    | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 hw/i386/pc_piix.c   |  2 ++
 hw/i386/pc_q35.c    |  2 ++
 include/hw/loader.h |  1 +
 4 files changed, 56 insertions(+), 3 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 6875b7e..32d807a 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -54,6 +54,8 @@
 
 #include <zlib.h>
 
+bool rom_file_in_ram = true;
+
 static int roms_loaded;
 
 /* return the size or -1 if error */
@@ -576,6 +578,7 @@ struct Rom {
     size_t datasize;
 
     uint8_t *data;
+    MemoryRegion *mr;
     int isrom;
     char *fw_dir;
     char *fw_file;
@@ -605,6 +608,26 @@ static void rom_insert(Rom *rom)
     QTAILQ_INSERT_TAIL(&roms, rom, next);
 }
 
+static void *rom_set_mr(Rom *rom, Object *owner, const char *name)
+{
+    /*
+     * Migration code expects that all RAM blocks are full pages.
+     * Round MR size up to satisfy this condition.
+     */
+    unsigned size = ROUND_UP(rom->datasize, qemu_migration_page_size());
+    void *data;
+
+    rom->mr = g_malloc(sizeof(*rom->mr));
+    memory_region_init_ram(rom->mr, owner, name, size);
+    memory_region_set_readonly(rom->mr, true);
+    vmstate_register_ram_global(rom->mr);
+
+    data = memory_region_get_ram_ptr(rom->mr);
+    memcpy(data, rom->data, rom->datasize);
+
+    return data;
+}
+
 int rom_add_file(const char *file, const char *fw_dir,
                  hwaddr addr, int32_t bootindex)
 {
@@ -646,6 +669,7 @@ int rom_add_file(const char *file, const char *fw_dir,
     if (rom->fw_file && fw_cfg) {
         const char *basename;
         char fw_file_name[56];
+        void *data;
 
         basename = strrchr(rom->fw_file, '/');
         if (basename) {
@@ -655,8 +679,15 @@ int rom_add_file(const char *file, const char *fw_dir,
         }
         snprintf(fw_file_name, sizeof(fw_file_name), "%s/%s", rom->fw_dir,
                  basename);
-        fw_cfg_add_file(fw_cfg, fw_file_name, rom->data, rom->romsize);
         snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
+
+        if (rom_file_in_ram) {
+            data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
+        } else {
+            data = rom->data;
+        }
+
+        fw_cfg_add_file(fw_cfg, fw_file_name, data, rom->romsize);
     } else {
         snprintf(devpath, sizeof(devpath), "/rom@" TARGET_FMT_plx, addr);
     }
@@ -731,7 +762,12 @@ static void rom_reset(void *unused)
         if (rom->data == NULL) {
             continue;
         }
-        cpu_physical_memory_write_rom(rom->addr, rom->data, rom->datasize);
+        if (rom->mr) {
+            void *host = memory_region_get_ram_ptr(rom->mr);
+            memcpy(host, rom->data, rom->datasize);
+        } else {
+            cpu_physical_memory_write_rom(rom->addr, rom->data, rom->datasize);
+        }
         if (rom->isrom) {
             /* rom needs to be written only once */
             g_free(rom->data);
@@ -781,6 +817,9 @@ static Rom *find_rom(hwaddr addr)
         if (rom->fw_file) {
             continue;
         }
+        if (rom->mr) {
+            continue;
+        }
         if (rom->addr > addr) {
             continue;
         }
@@ -808,6 +847,9 @@ int rom_copy(uint8_t *dest, hwaddr addr, size_t size)
         if (rom->fw_file) {
             continue;
         }
+        if (rom->mr) {
+            continue;
+        }
         if (rom->addr + rom->romsize < addr) {
             continue;
         }
@@ -866,7 +908,13 @@ void do_info_roms(Monitor *mon, const QDict *qdict)
     Rom *rom;
 
     QTAILQ_FOREACH(rom, &roms, next) {
-        if (!rom->fw_file) {
+        if (rom->mr) {
+            monitor_printf(mon, "%s"
+                           " size=0x%06zx name=\"%s\"\n",
+                           rom->mr->name,
+                           rom->romsize,
+                           rom->name);
+        } else if (!rom->fw_file) {
             monitor_printf(mon, "addr=" TARGET_FMT_plx
                            " size=0x%06zx mem=%s name=\"%s\"\n",
                            rom->addr, rom->romsize,
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 95c45b8..2a5348e 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -25,6 +25,7 @@
 #include <glib.h>
 
 #include "hw/hw.h"
+#include "hw/loader.h"
 #include "hw/i386/pc.h"
 #include "hw/i386/apic.h"
 #include "hw/pci/pci.h"
@@ -252,6 +253,7 @@ static void pc_init_pci(QEMUMachineInitArgs *args)
 static void pc_init_pci_1_6(QEMUMachineInitArgs *args)
 {
     has_pci_info = false;
+    rom_file_in_ram = false;
     pc_init_pci(args);
 }
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 6bfc2ca..66e7e1b 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -28,6 +28,7 @@
  * THE SOFTWARE.
  */
 #include "hw/hw.h"
+#include "hw/loader.h"
 #include "sysemu/arch_init.h"
 #include "hw/i2c/smbus.h"
 #include "hw/boards.h"
@@ -220,6 +221,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
 static void pc_q35_init_1_6(QEMUMachineInitArgs *args)
 {
     has_pci_info = false;
+    rom_file_in_ram = false;
     pc_q35_init(args);
 }
 
diff --git a/include/hw/loader.h b/include/hw/loader.h
index eb9c9a3..6145736 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -36,6 +36,7 @@ void pstrcpy_targphys(const char *name,
                       hwaddr dest, int buf_size,
                       const char *source);
 
+extern bool rom_file_in_ram;
 
 int rom_add_file(const char *file, const char *fw_dir,
                  hwaddr addr, int32_t bootindex);
-- 
MST

Re: [Qemu-devel] [PATCH v3 0/2] future proof rom loading for cross versiom migration

[Michael S. Tsirkin]

On Tue, Aug 13, 2013 at 01:43:29AM +0300, Michael S. Tsirkin wrote:
> Changes from v2: address comments on v2 by Peter Maydell
>     - switch from global constant to function
>     - use memory_region_init_ram instead of _ram_ptr
>     - disable for 1.6
> 
> Changes from v1: address comments by Peter Maydell
>     - drop useless data=data line
>     - rename target_page_size to migration_page_size to make use clear
>     Peter, you also suggested somehow hiding this within memory core.
>     I don't see a clean way to do this without lots of code
>     changes, I think what I propose here is acceptable for now
>     and we can always rework APIs without wire format changes.
> 
> Please review, and consider for merging.

Ping. Any more comments?
Also - which tree can this go in through?
Mine?

> Original cover letter below.
> 
> 
> ROM files that are put in FW CFG are copied to guest ram, by BIOS, but
> they are not backed by RAM so they don't get migrated.
> 
> Each time we'll change at least two bytes in such a ROM this will break
> cross-version migration: since we can migrate after BIOS has read the first
> byte but before it has read the second one, getting an inconsistent state.
> 
> This patchset makes QEMU future-proof against such changes.
> 
> Naturally, this only helps for -M 1.6 and up, older machine types
> will still have the cross-version migration bug.
> 
> I think this should be applied for 1.6, this way we won't
> have this problem from 1.7 and on.
> 
> Michael S. Tsirkin (2):
>   memory: export migration page size
>   loader: put FW CFG ROM files into RAM
> 
>  arch_init.c           |  6 ++++++
>  hw/core/loader.c      | 54 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  hw/i386/pc_piix.c     |  2 ++
>  hw/i386/pc_q35.c      |  2 ++
>  include/exec/memory.h |  1 +
>  include/hw/loader.h   |  1 +
>  6 files changed, 63 insertions(+), 3 deletions(-)
> 
> -- 
> MST
> 

Re: [Qemu-devel] [PATCH v3 1/2] memory: export migration page size

[Laszlo Ersek]

On 08/13/13 00:43, Michael S. Tsirkin wrote:
> Migration code assumes that each RAM block is a multiple of target page
> size.

Isn't that a valid assumption, considering the TARGET_PAGE_ALIGN() macro
call in qemu_ram_alloc_from_ptr() [exec.c]?

> We can fix this in a variety of ways, the simplest way is
> exporting the required page size so callers can make regions
> large enough.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  arch_init.c           | 6 ++++++
>  include/exec/memory.h | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 68a7ab7..c62778f 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -150,6 +150,12 @@ int qemu_read_default_config_files(bool userconfig)
>      return 0;
>  }
>  
> +/* Smallest page size for migrated RAM. */
> +uint64_t qemu_migration_page_size(void)
> +{
> +    return TARGET_PAGE_SIZE;
> +}
> +
>  static inline bool is_zero_page(uint8_t *p)
>  {
>      return buffer_find_nonzero_offset(p, TARGET_PAGE_SIZE) ==
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index ebe0d24..6ffffa2 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1055,6 +1055,7 @@ void *address_space_map(AddressSpace *as, hwaddr addr,
>  void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
>                           int is_write, hwaddr access_len);
>  
> +extern uint64_t qemu_migration_page_size(void);
>  
>  #endif

External linkage functions that are defined in "arch_init.c", and relate
to migration -- for example, skipped_mig_bytes_transferred() -- are
declared in "include/migration/migration.h". They seem to use
TARGET_PAGE_SIZE too.

What justifies declaring this new function in "include/exec/memory.h"?

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH v3 1/2] memory: export migration page size

[Peter Maydell]

On 19 August 2013 10:59, Laszlo Ersek <lersek@redhat.com> wrote:
> On 08/13/13 00:43, Michael S. Tsirkin wrote:
>> Migration code assumes that each RAM block is a multiple of target page
>> size.
>
> Isn't that a valid assumption, considering the TARGET_PAGE_ALIGN() macro
> call in qemu_ram_alloc_from_ptr() [exec.c]?

That macro only makes the size we store in the ramblock data
structure be a multiple of the page size -- it does nothing to ensure
that the actual memory that was passed in by the caller is the
right size. (It will have the right effect where qemu_ram_alloc_from_ptr
is allocating the memory itself, obviously.)

-- PMM

Re: [Qemu-devel] [PATCH v3 1/2] memory: export migration page size

[Michael S. Tsirkin]

On Mon, Aug 19, 2013 at 11:59:25AM +0200, Laszlo Ersek wrote:
> On 08/13/13 00:43, Michael S. Tsirkin wrote:
> > Migration code assumes that each RAM block is a multiple of target page
> > size.
> 
> Isn't that a valid assumption, considering the TARGET_PAGE_ALIGN() macro
> call in qemu_ram_alloc_from_ptr() [exec.c]?
> 
> > We can fix this in a variety of ways, the simplest way is
> > exporting the required page size so callers can make regions
> > large enough.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  arch_init.c           | 6 ++++++
> >  include/exec/memory.h | 1 +
> >  2 files changed, 7 insertions(+)
> > 
> > diff --git a/arch_init.c b/arch_init.c
> > index 68a7ab7..c62778f 100644
> > --- a/arch_init.c
> > +++ b/arch_init.c
> > @@ -150,6 +150,12 @@ int qemu_read_default_config_files(bool userconfig)
> >      return 0;
> >  }
> >  
> > +/* Smallest page size for migrated RAM. */
> > +uint64_t qemu_migration_page_size(void)
> > +{
> > +    return TARGET_PAGE_SIZE;
> > +}
> > +
> >  static inline bool is_zero_page(uint8_t *p)
> >  {
> >      return buffer_find_nonzero_offset(p, TARGET_PAGE_SIZE) ==
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index ebe0d24..6ffffa2 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -1055,6 +1055,7 @@ void *address_space_map(AddressSpace *as, hwaddr addr,
> >  void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
> >                           int is_write, hwaddr access_len);
> >  
> > +extern uint64_t qemu_migration_page_size(void);
> >  
> >  #endif
> 
> External linkage functions that are defined in "arch_init.c", and relate
> to migration -- for example, skipped_mig_bytes_transferred() -- are
> declared in "include/migration/migration.h". They seem to use
> TARGET_PAGE_SIZE too.
> 
> What justifies declaring this new function in "include/exec/memory.h"?
> 
> Thanks,
> Laszlo


Well, this one is a bit different as this just helps device
allocate the right amount of RAM.
It doesn't deal with migration directly.


-- 
MST

Re: [Qemu-devel] [PATCH v3 2/2] loader: put FW CFG ROM files into RAM

[Laszlo Ersek]

On 08/13/13 00:43, Michael S. Tsirkin wrote:
> ROM files that are put in FW CFG are copied to guest ram, by BIOS, but
> they are not backed by RAM so they don't get migrated.

Can you please elaborate on this? Do you mean the 384 KB range between
640KB and 1MB that is covered by RAMBlock, but no MemoryRegion?

> 
> Each time we change two bytes in such a ROM this breaks cross-version
> migration: since we can migrate after BIOS has read the first byte but
> before it has read the second one, getting an inconsistent state.
> 
> Future-proof this by creating, for each such ROM,
> an MR serving as the backing store.
> This MR is never mapped into guest memory, but it's registered
> as RAM so it's migrated with the guest.

savevm seems to work off RAMBlocks (ram_list.blocks), not memory regions.

... Ah. You probably don't mean the *target* memory where the guest
copies the fw_cfg contents from the ioport. You probably mean the
*source* (internal representation) of the fw_cfg contents that qemu
passes down via the ioport.

Currently those bytes appear out of thin air.

Is my understanding correct that this patch stuffs them into RAM (that
the guest has no knowledge about), so that migration automatically
copies over the *source* of fw_cfg contents too?

fw_cfg state (current entry selector etc) is tracked according to
"vmstate_fw_cfg". It only includes some metadata, not the actual contents.

I wonder if the "right" fix would be to save the fw_cfg files in
"vmstate_fw_cfg" too.

> 
> Naturally, this only helps for -M 1.7 and up, older machine types
> will still have the cross-version migration bug.
> Luckily the race window for the problem to trigger is very small,
> which is also likely why we didn't notice the cross-version
> migration bug in testing yet.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/core/loader.c    | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  hw/i386/pc_piix.c   |  2 ++
>  hw/i386/pc_q35.c    |  2 ++
>  include/hw/loader.h |  1 +
>  4 files changed, 56 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 6875b7e..32d807a 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -54,6 +54,8 @@
>  
>  #include <zlib.h>
>  
> +bool rom_file_in_ram = true;
> +
>  static int roms_loaded;
>  
>  /* return the size or -1 if error */
> @@ -576,6 +578,7 @@ struct Rom {
>      size_t datasize;
>  
>      uint8_t *data;
> +    MemoryRegion *mr;
>      int isrom;
>      char *fw_dir;
>      char *fw_file;
> @@ -605,6 +608,26 @@ static void rom_insert(Rom *rom)
>      QTAILQ_INSERT_TAIL(&roms, rom, next);
>  }
>  
> +static void *rom_set_mr(Rom *rom, Object *owner, const char *name)
> +{
> +    /*
> +     * Migration code expects that all RAM blocks are full pages.
> +     * Round MR size up to satisfy this condition.
> +     */
> +    unsigned size = ROUND_UP(rom->datasize, qemu_migration_page_size());

I'm not sure this is needed at all (see my comments for 1/2 -- they can
be wrong of course). In any case, wouldn't TARGET_PAGE_ALIGN() be more
appropriate?

> +    void *data;
> +
> +    rom->mr = g_malloc(sizeof(*rom->mr));
> +    memory_region_init_ram(rom->mr, owner, name, size);

memory_region_init_ram()
  qemu_ram_alloc()
    qemu_ram_alloc_from_ptr()
      TARGET_PAGE_ALIGN()

So we don't have to round up the size ourselves, and patch #1 might be
unnecessary after all.

I also understand now that we're allocating a brand new RAMBlock, which
we won't map into guest-phys address space: we'll call *none* of the
memory API functions that would do that, eg:
- memory_region_init_alias(),
- memory_region_add_subregion[_overlap](),
- memory_region_set_address().

> +    memory_region_set_readonly(rom->mr, true);
> +    vmstate_register_ram_global(rom->mr);

OK, this call adds the new RAMBlock to "ram_list.blocks", via
qemu_ram_set_idstr().

> +
> +    data = memory_region_get_ram_ptr(rom->mr);
> +    memcpy(data, rom->data, rom->datasize);
> +
> +    return data;
> +}
> +

So, this function allocates a new RAMBlock, doesn't map it into
guest-phys address-space, and copies the fw_cfg contents into it.


>  int rom_add_file(const char *file, const char *fw_dir,
>                   hwaddr addr, int32_t bootindex)
>  {
> @@ -646,6 +669,7 @@ int rom_add_file(const char *file, const char *fw_dir,
>      if (rom->fw_file && fw_cfg) {
>          const char *basename;
>          char fw_file_name[56];
> +        void *data;
>  
>          basename = strrchr(rom->fw_file, '/');
>          if (basename) {
> @@ -655,8 +679,15 @@ int rom_add_file(const char *file, const char *fw_dir,
>          }
>          snprintf(fw_file_name, sizeof(fw_file_name), "%s/%s", rom->fw_dir,
>                   basename);
> -        fw_cfg_add_file(fw_cfg, fw_file_name, rom->data, rom->romsize);
>          snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
> +
> +        if (rom_file_in_ram) {
> +            data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
> +        } else {
> +            data = rom->data;
> +        }
> +
> +        fw_cfg_add_file(fw_cfg, fw_file_name, data, rom->romsize);

This seems OK, but if "rom_file_in_ram" is nonzero, then we'll store the
ROM contents in the qemu process twice -- once in "rom->data" (allocated
just a bit higher up, not shown in context), and in the new RAMBlock.

This is no bug of course, I'm just wondering if we could drop/repoint
"rom->data" in this case.

>      } else {
>          snprintf(devpath, sizeof(devpath), "/rom@" TARGET_FMT_plx, addr);
>      }
> @@ -731,7 +762,12 @@ static void rom_reset(void *unused)
>          if (rom->data == NULL) {
>              continue;
>          }
> -        cpu_physical_memory_write_rom(rom->addr, rom->data, rom->datasize);
> +        if (rom->mr) {
> +            void *host = memory_region_get_ram_ptr(rom->mr);
> +            memcpy(host, rom->data, rom->datasize);
> +        } else {
> +            cpu_physical_memory_write_rom(rom->addr, rom->data, rom->datasize);
> +        }

Hmmm. Why is this (ie. the pre-patch resetting) necessary at all?

Is this due to the writeability of fw_cfg files via the ioport
(fw_cfg_write())? I think that modifies "rom->data" unconditionally
(which is currently kept separate from the RAMBlock, see above).

So, regarding the patched version:
- not sure if the RAMBlock can change at all -- it is neither mapped
into guest-phys address space, nor does fw_cfg_write() touch it,
- *if* the guest modifies the contents under "rom->addr", via
fw_cfg_write(), then the hva-space memcpy() is insufficient.


>          if (rom->isrom) {
>              /* rom needs to be written only once */
>              g_free(rom->data);
> @@ -781,6 +817,9 @@ static Rom *find_rom(hwaddr addr)
>          if (rom->fw_file) {
>              continue;
>          }
> +        if (rom->mr) {
> +            continue;
> +        }

This means that (rom->fw_file || rom->mr) is sufficient not to return
the ROM.

"rom->mr" depends on "rom->fw_file":

rom_add_file()
  rom->fw_file = NULL // via g_malloc()
  sets "rom->fw_file" // if (fw_dir)
  rom_set_mr()        // if (rom->fw_file && fw_cfg && rom_file_in_ram)
    sets "rom->mr"

So I believe this explicit check here is not necessary; it will always
evaluate to false. (It doesn't hurt of course, for robustness).

>          if (rom->addr > addr) {
>              continue;
>          }
> @@ -808,6 +847,9 @@ int rom_copy(uint8_t *dest, hwaddr addr, size_t size)
>          if (rom->fw_file) {
>              continue;
>          }
> +        if (rom->mr) {
> +            continue;
> +        }
>          if (rom->addr + rom->romsize < addr) {
>              continue;
>          }

Ditto.

> @@ -866,7 +908,13 @@ void do_info_roms(Monitor *mon, const QDict *qdict)
>      Rom *rom;
>  
>      QTAILQ_FOREACH(rom, &roms, next) {
> -        if (!rom->fw_file) {
> +        if (rom->mr) {
> +            monitor_printf(mon, "%s"
> +                           " size=0x%06zx name=\"%s\"\n",
> +                           rom->mr->name,
> +                           rom->romsize,
> +                           rom->name);
> +        } else if (!rom->fw_file) {
>              monitor_printf(mon, "addr=" TARGET_FMT_plx
>                             " size=0x%06zx mem=%s name=\"%s\"\n",
>                             rom->addr, rom->romsize,

"rom->mr" implies "rom->fw_file", which is equivalent to "!rom->fw_file"
implying "!rom->mr".

However "!rom->mr" doesn't imply "!rom->fw_file", so the check for the
latter is needed & correct.

Not sure what size we should print in the "rom->mr" case. The RAMBlock
size is a multiple of the target page size. Maybe we could print both sizes.

What do you think about my comments?

Thanks,
Laszlo

> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 95c45b8..2a5348e 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -25,6 +25,7 @@
>  #include <glib.h>
>  
>  #include "hw/hw.h"
> +#include "hw/loader.h"
>  #include "hw/i386/pc.h"
>  #include "hw/i386/apic.h"
>  #include "hw/pci/pci.h"
> @@ -252,6 +253,7 @@ static void pc_init_pci(QEMUMachineInitArgs *args)
>  static void pc_init_pci_1_6(QEMUMachineInitArgs *args)
>  {
>      has_pci_info = false;
> +    rom_file_in_ram = false;
>      pc_init_pci(args);
>  }
>  
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 6bfc2ca..66e7e1b 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -28,6 +28,7 @@
>   * THE SOFTWARE.
>   */
>  #include "hw/hw.h"
> +#include "hw/loader.h"
>  #include "sysemu/arch_init.h"
>  #include "hw/i2c/smbus.h"
>  #include "hw/boards.h"
> @@ -220,6 +221,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>  static void pc_q35_init_1_6(QEMUMachineInitArgs *args)
>  {
>      has_pci_info = false;
> +    rom_file_in_ram = false;
>      pc_q35_init(args);
>  }
>  
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index eb9c9a3..6145736 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -36,6 +36,7 @@ void pstrcpy_targphys(const char *name,
>                        hwaddr dest, int buf_size,
>                        const char *source);
>  
> +extern bool rom_file_in_ram;
>  
>  int rom_add_file(const char *file, const char *fw_dir,
>                   hwaddr addr, int32_t bootindex);
> 

Re: [Qemu-devel] [PATCH v3 1/2] memory: export migration page size

[Laszlo Ersek]

On 08/19/13 12:21, Peter Maydell wrote:
> On 19 August 2013 10:59, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 08/13/13 00:43, Michael S. Tsirkin wrote:
>>> Migration code assumes that each RAM block is a multiple of target page
>>> size.
>>
>> Isn't that a valid assumption, considering the TARGET_PAGE_ALIGN() macro
>> call in qemu_ram_alloc_from_ptr() [exec.c]?
> 
> That macro only makes the size we store in the ramblock data
> structure be a multiple of the page size -- it does nothing to ensure
> that the actual memory that was passed in by the caller is the
> right size. (It will have the right effect where qemu_ram_alloc_from_ptr
> is allocating the memory itself, obviously.)

Which is the case for 2/2, see my comments there:

memory_region_init_ram()
  qemu_ram_alloc()
    qemu_ram_alloc_from_ptr()  <---- host==NULL
      TARGET_PAGE_ALIGN()

Laszlo

Re: [Qemu-devel] [PATCH v3 2/2] loader: put FW CFG ROM files into RAM

[Michael S. Tsirkin]

On Mon, Aug 19, 2013 at 01:06:07PM +0200, Laszlo Ersek wrote:
> On 08/13/13 00:43, Michael S. Tsirkin wrote:
> > ROM files that are put in FW CFG are copied to guest ram, by BIOS, but
> > they are not backed by RAM so they don't get migrated.
> 
> Can you please elaborate on this? Do you mean the 384 KB range between
> 640KB and 1MB that is covered by RAMBlock, but no MemoryRegion?

No. Answered below.

> > 
> > Each time we change two bytes in such a ROM this breaks cross-version
> > migration: since we can migrate after BIOS has read the first byte but
> > before it has read the second one, getting an inconsistent state.
> > 
> > Future-proof this by creating, for each such ROM,
> > an MR serving as the backing store.
> > This MR is never mapped into guest memory, but it's registered
> > as RAM so it's migrated with the guest.
> 
> savevm seems to work off RAMBlocks (ram_list.blocks), not memory regions.
> 
> ... Ah. You probably don't mean the *target* memory where the guest
> copies the fw_cfg contents from the ioport. You probably mean the
> *source* (internal representation) of the fw_cfg contents that qemu
> passes down via the ioport.
> 
> Currently those bytes appear out of thin air.
> 
> Is my understanding correct that this patch stuffs them into RAM (that
> the guest has no knowledge about), so that migration automatically
> copies over the *source* of fw_cfg contents too?

Exactly.

> fw_cfg state (current entry selector etc) is tracked according to
> "vmstate_fw_cfg". It only includes some metadata, not the actual contents.
> 
> I wonder if the "right" fix would be to save the fw_cfg files in
> "vmstate_fw_cfg" too.

This was proposed, and NACKed by Anthony.
Generally vmstate is for small bits of data,
ROM files are too large ...

You can look at this as some RAM internal to device,
we are migrating it as we would any RAM
even though guest can't access it directly.


> > 
> > Naturally, this only helps for -M 1.7 and up, older machine types
> > will still have the cross-version migration bug.
> > Luckily the race window for the problem to trigger is very small,
> > which is also likely why we didn't notice the cross-version
> > migration bug in testing yet.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/core/loader.c    | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++---
> >  hw/i386/pc_piix.c   |  2 ++
> >  hw/i386/pc_q35.c    |  2 ++
> >  include/hw/loader.h |  1 +
> >  4 files changed, 56 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/core/loader.c b/hw/core/loader.c
> > index 6875b7e..32d807a 100644
> > --- a/hw/core/loader.c
> > +++ b/hw/core/loader.c
> > @@ -54,6 +54,8 @@
> >  
> >  #include <zlib.h>
> >  
> > +bool rom_file_in_ram = true;
> > +
> >  static int roms_loaded;
> >  
> >  /* return the size or -1 if error */
> > @@ -576,6 +578,7 @@ struct Rom {
> >      size_t datasize;
> >  
> >      uint8_t *data;
> > +    MemoryRegion *mr;
> >      int isrom;
> >      char *fw_dir;
> >      char *fw_file;
> > @@ -605,6 +608,26 @@ static void rom_insert(Rom *rom)
> >      QTAILQ_INSERT_TAIL(&roms, rom, next);
> >  }
> >  
> > +static void *rom_set_mr(Rom *rom, Object *owner, const char *name)
> > +{
> > +    /*
> > +     * Migration code expects that all RAM blocks are full pages.
> > +     * Round MR size up to satisfy this condition.
> > +     */
> > +    unsigned size = ROUND_UP(rom->datasize, qemu_migration_page_size());
> 
> I'm not sure this is needed at all (see my comments for 1/2 -- they can
> be wrong of course). In any case, wouldn't TARGET_PAGE_ALIGN() be more
> appropriate?
> 
> > +    void *data;
> > +
> > +    rom->mr = g_malloc(sizeof(*rom->mr));
> > +    memory_region_init_ram(rom->mr, owner, name, size);
> 
> memory_region_init_ram()
>   qemu_ram_alloc()
>     qemu_ram_alloc_from_ptr()
>       TARGET_PAGE_ALIGN()
> 
> So we don't have to round up the size ourselves, and patch #1 might be
> unnecessary after all.
> 
> I also understand now that we're allocating a brand new RAMBlock, which
> we won't map into guest-phys address space: we'll call *none* of the
> memory API functions that would do that, eg:
> - memory_region_init_alias(),
> - memory_region_add_subregion[_overlap](),
> - memory_region_set_address().
> 
> > +    memory_region_set_readonly(rom->mr, true);
> > +    vmstate_register_ram_global(rom->mr);
> 
> OK, this call adds the new RAMBlock to "ram_list.blocks", via
> qemu_ram_set_idstr().
> 
> > +
> > +    data = memory_region_get_ram_ptr(rom->mr);
> > +    memcpy(data, rom->data, rom->datasize);
> > +
> > +    return data;
> > +}
> > +
> 
> So, this function allocates a new RAMBlock, doesn't map it into
> guest-phys address-space, and copies the fw_cfg contents into it.
> 
> 
> >  int rom_add_file(const char *file, const char *fw_dir,
> >                   hwaddr addr, int32_t bootindex)
> >  {
> > @@ -646,6 +669,7 @@ int rom_add_file(const char *file, const char *fw_dir,
> >      if (rom->fw_file && fw_cfg) {
> >          const char *basename;
> >          char fw_file_name[56];
> > +        void *data;
> >  
> >          basename = strrchr(rom->fw_file, '/');
> >          if (basename) {
> > @@ -655,8 +679,15 @@ int rom_add_file(const char *file, const char *fw_dir,
> >          }
> >          snprintf(fw_file_name, sizeof(fw_file_name), "%s/%s", rom->fw_dir,
> >                   basename);
> > -        fw_cfg_add_file(fw_cfg, fw_file_name, rom->data, rom->romsize);
> >          snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
> > +
> > +        if (rom_file_in_ram) {
> > +            data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
> > +        } else {
> > +            data = rom->data;
> > +        }
> > +
> > +        fw_cfg_add_file(fw_cfg, fw_file_name, data, rom->romsize);
> 
> This seems OK, but if "rom_file_in_ram" is nonzero, then we'll store the
> ROM contents in the qemu process twice -- once in "rom->data" (allocated
> just a bit higher up, not shown in context), and in the new RAMBlock.
> 
> This is no bug of course, I'm just wondering if we could drop/repoint
> "rom->data" in this case.
> 
> >      } else {
> >          snprintf(devpath, sizeof(devpath), "/rom@" TARGET_FMT_plx, addr);
> >      }
> > @@ -731,7 +762,12 @@ static void rom_reset(void *unused)
> >          if (rom->data == NULL) {
> >              continue;
> >          }
> > -        cpu_physical_memory_write_rom(rom->addr, rom->data, rom->datasize);
> > +        if (rom->mr) {
> > +            void *host = memory_region_get_ram_ptr(rom->mr);
> > +            memcpy(host, rom->data, rom->datasize);
> > +        } else {
> > +            cpu_physical_memory_write_rom(rom->addr, rom->data, rom->datasize);
> > +        }
> 
> Hmmm. Why is this (ie. the pre-patch resetting) necessary at all?
> 
> Is this due to the writeability of fw_cfg files via the ioport
> (fw_cfg_write())? I think that modifies "rom->data" unconditionally
> (which is currently kept separate from the RAMBlock, see above).
> 
> So, regarding the patched version:
> - not sure if the RAMBlock can change at all -- it is neither mapped
> into guest-phys address space, nor does fw_cfg_write() touch it,
> - *if* the guest modifies the contents under "rom->addr", via
> fw_cfg_write(), then the hva-space memcpy() is insufficient.
> 
> 
> >          if (rom->isrom) {
> >              /* rom needs to be written only once */
> >              g_free(rom->data);
> > @@ -781,6 +817,9 @@ static Rom *find_rom(hwaddr addr)
> >          if (rom->fw_file) {
> >              continue;
> >          }
> > +        if (rom->mr) {
> > +            continue;
> > +        }
> 
> This means that (rom->fw_file || rom->mr) is sufficient not to return
> the ROM.
> 
> "rom->mr" depends on "rom->fw_file":
> 
> rom_add_file()
>   rom->fw_file = NULL // via g_malloc()
>   sets "rom->fw_file" // if (fw_dir)
>   rom_set_mr()        // if (rom->fw_file && fw_cfg && rom_file_in_ram)
>     sets "rom->mr"
> 
> So I believe this explicit check here is not necessary; it will always
> evaluate to false. (It doesn't hurt of course, for robustness).
> 
> >          if (rom->addr > addr) {
> >              continue;
> >          }
> > @@ -808,6 +847,9 @@ int rom_copy(uint8_t *dest, hwaddr addr, size_t size)
> >          if (rom->fw_file) {
> >              continue;
> >          }
> > +        if (rom->mr) {
> > +            continue;
> > +        }
> >          if (rom->addr + rom->romsize < addr) {
> >              continue;
> >          }
> 
> Ditto.
> 
> > @@ -866,7 +908,13 @@ void do_info_roms(Monitor *mon, const QDict *qdict)
> >      Rom *rom;
> >  
> >      QTAILQ_FOREACH(rom, &roms, next) {
> > -        if (!rom->fw_file) {
> > +        if (rom->mr) {
> > +            monitor_printf(mon, "%s"
> > +                           " size=0x%06zx name=\"%s\"\n",
> > +                           rom->mr->name,
> > +                           rom->romsize,
> > +                           rom->name);
> > +        } else if (!rom->fw_file) {
> >              monitor_printf(mon, "addr=" TARGET_FMT_plx
> >                             " size=0x%06zx mem=%s name=\"%s\"\n",
> >                             rom->addr, rom->romsize,
> 
> "rom->mr" implies "rom->fw_file", which is equivalent to "!rom->fw_file"
> implying "!rom->mr".
> 
> However "!rom->mr" doesn't imply "!rom->fw_file", so the check for the
> latter is needed & correct.
> 
> Not sure what size we should print in the "rom->mr" case. The RAMBlock
> size is a multiple of the target page size. Maybe we could print both sizes.
> 
> What do you think about my comments?
> 
> Thanks,
> Laszlo
> 
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 95c45b8..2a5348e 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -25,6 +25,7 @@
> >  #include <glib.h>
> >  
> >  #include "hw/hw.h"
> > +#include "hw/loader.h"
> >  #include "hw/i386/pc.h"
> >  #include "hw/i386/apic.h"
> >  #include "hw/pci/pci.h"
> > @@ -252,6 +253,7 @@ static void pc_init_pci(QEMUMachineInitArgs *args)
> >  static void pc_init_pci_1_6(QEMUMachineInitArgs *args)
> >  {
> >      has_pci_info = false;
> > +    rom_file_in_ram = false;
> >      pc_init_pci(args);
> >  }
> >  
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index 6bfc2ca..66e7e1b 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -28,6 +28,7 @@
> >   * THE SOFTWARE.
> >   */
> >  #include "hw/hw.h"
> > +#include "hw/loader.h"
> >  #include "sysemu/arch_init.h"
> >  #include "hw/i2c/smbus.h"
> >  #include "hw/boards.h"
> > @@ -220,6 +221,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
> >  static void pc_q35_init_1_6(QEMUMachineInitArgs *args)
> >  {
> >      has_pci_info = false;
> > +    rom_file_in_ram = false;
> >      pc_q35_init(args);
> >  }
> >  
> > diff --git a/include/hw/loader.h b/include/hw/loader.h
> > index eb9c9a3..6145736 100644
> > --- a/include/hw/loader.h
> > +++ b/include/hw/loader.h
> > @@ -36,6 +36,7 @@ void pstrcpy_targphys(const char *name,
> >                        hwaddr dest, int buf_size,
> >                        const char *source);
> >  
> > +extern bool rom_file_in_ram;
> >  
> >  int rom_add_file(const char *file, const char *fw_dir,
> >                   hwaddr addr, int32_t bootindex);
> > 

Re: [Qemu-devel] [PATCH v3 2/2] loader: put FW CFG ROM files into RAM

[Laszlo Ersek]

On 08/19/13 13:06, Laszlo Ersek wrote:
> On 08/13/13 00:43, Michael S. Tsirkin wrote:

>> @@ -646,6 +669,7 @@ int rom_add_file(const char *file, const char *fw_dir,
>>      if (rom->fw_file && fw_cfg) {
>>          const char *basename;
>>          char fw_file_name[56];
>> +        void *data;
>>  
>>          basename = strrchr(rom->fw_file, '/');
>>          if (basename) {
>> @@ -655,8 +679,15 @@ int rom_add_file(const char *file, const char *fw_dir,
>>          }
>>          snprintf(fw_file_name, sizeof(fw_file_name), "%s/%s", rom->fw_dir,
>>                   basename);
>> -        fw_cfg_add_file(fw_cfg, fw_file_name, rom->data, rom->romsize);
>>          snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
>> +
>> +        if (rom_file_in_ram) {
>> +            data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
>> +        } else {
>> +            data = rom->data;
>> +        }
>> +
>> +        fw_cfg_add_file(fw_cfg, fw_file_name, data, rom->romsize);
> 
> This seems OK, but if "rom_file_in_ram" is nonzero, then we'll store the
> ROM contents in the qemu process twice -- once in "rom->data" (allocated
> just a bit higher up, not shown in context), and in the new RAMBlock.
> 
> This is no bug of course, I'm just wondering if we could drop/repoint
> "rom->data" in this case.
> 
>>      } else {
>>          snprintf(devpath, sizeof(devpath), "/rom@" TARGET_FMT_plx, addr);
>>      }
>> @@ -731,7 +762,12 @@ static void rom_reset(void *unused)
>>          if (rom->data == NULL) {
>>              continue;
>>          }
>> -        cpu_physical_memory_write_rom(rom->addr, rom->data, rom->datasize);
>> +        if (rom->mr) {
>> +            void *host = memory_region_get_ram_ptr(rom->mr);
>> +            memcpy(host, rom->data, rom->datasize);
>> +        } else {
>> +            cpu_physical_memory_write_rom(rom->addr, rom->data, rom->datasize);
>> +        }
> 
> Hmmm. Why is this (ie. the pre-patch resetting) necessary at all?
> 
> Is this due to the writeability of fw_cfg files via the ioport
> (fw_cfg_write())? I think that modifies "rom->data" unconditionally
> (which is currently kept separate from the RAMBlock, see above).
> 
> So, regarding the patched version:
> - not sure if the RAMBlock can change at all -- it is neither mapped
> into guest-phys address space, nor does fw_cfg_write() touch it,
> - *if* the guest modifies the contents under "rom->addr", via
> fw_cfg_write(), then the hva-space memcpy() is insufficient.

Sorry, I'm wrong here. The patched rom_add_file() ensures that
fw_cfg_write() modifies the correct backing store. Also, we need to keep
"rom->data" around even if "rom_file_in_ram" is set, because that's
where we restore the RAMBlock contents from, in case of a reset.

Laszlo

Re: [Qemu-devel] [PATCH v3 1/2] memory: export migration page size

[Michael S. Tsirkin]

On Mon, Aug 19, 2013 at 01:09:36PM +0200, Laszlo Ersek wrote:
> On 08/19/13 12:21, Peter Maydell wrote:
> > On 19 August 2013 10:59, Laszlo Ersek <lersek@redhat.com> wrote:
> >> On 08/13/13 00:43, Michael S. Tsirkin wrote:
> >>> Migration code assumes that each RAM block is a multiple of target page
> >>> size.
> >>
> >> Isn't that a valid assumption, considering the TARGET_PAGE_ALIGN() macro
> >> call in qemu_ram_alloc_from_ptr() [exec.c]?
> > 
> > That macro only makes the size we store in the ramblock data
> > structure be a multiple of the page size -- it does nothing to ensure
> > that the actual memory that was passed in by the caller is the
> > right size. (It will have the right effect where qemu_ram_alloc_from_ptr
> > is allocating the memory itself, obviously.)
> 
> Which is the case for 2/2, see my comments there:
> 
> memory_region_init_ram()
>   qemu_ram_alloc()
>     qemu_ram_alloc_from_ptr()  <---- host==NULL
>       TARGET_PAGE_ALIGN()
> 
> Laszlo

The issue this addresses is not the size of RAM allocated.
The issue is the size of the MR.
Migration code assumes the size of the MR
is a multiple of TARGET_PAGE_SIZE.


-- 
MST

Re: [Qemu-devel] [PATCH v3 2/2] loader: put FW CFG ROM files into RAM

[Michael S. Tsirkin]

On Mon, Aug 19, 2013 at 01:15:36PM +0200, Laszlo Ersek wrote:
> On 08/19/13 13:06, Laszlo Ersek wrote:
> > On 08/13/13 00:43, Michael S. Tsirkin wrote:
> 
> >> @@ -646,6 +669,7 @@ int rom_add_file(const char *file, const char *fw_dir,
> >>      if (rom->fw_file && fw_cfg) {
> >>          const char *basename;
> >>          char fw_file_name[56];
> >> +        void *data;
> >>  
> >>          basename = strrchr(rom->fw_file, '/');
> >>          if (basename) {
> >> @@ -655,8 +679,15 @@ int rom_add_file(const char *file, const char *fw_dir,
> >>          }
> >>          snprintf(fw_file_name, sizeof(fw_file_name), "%s/%s", rom->fw_dir,
> >>                   basename);
> >> -        fw_cfg_add_file(fw_cfg, fw_file_name, rom->data, rom->romsize);
> >>          snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
> >> +
> >> +        if (rom_file_in_ram) {
> >> +            data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
> >> +        } else {
> >> +            data = rom->data;
> >> +        }
> >> +
> >> +        fw_cfg_add_file(fw_cfg, fw_file_name, data, rom->romsize);
> > 
> > This seems OK, but if "rom_file_in_ram" is nonzero, then we'll store the
> > ROM contents in the qemu process twice -- once in "rom->data" (allocated
> > just a bit higher up, not shown in context), and in the new RAMBlock.
> > 
> > This is no bug of course, I'm just wondering if we could drop/repoint
> > "rom->data" in this case.
> > 
> >>      } else {
> >>          snprintf(devpath, sizeof(devpath), "/rom@" TARGET_FMT_plx, addr);
> >>      }
> >> @@ -731,7 +762,12 @@ static void rom_reset(void *unused)
> >>          if (rom->data == NULL) {
> >>              continue;
> >>          }
> >> -        cpu_physical_memory_write_rom(rom->addr, rom->data, rom->datasize);
> >> +        if (rom->mr) {
> >> +            void *host = memory_region_get_ram_ptr(rom->mr);
> >> +            memcpy(host, rom->data, rom->datasize);
> >> +        } else {
> >> +            cpu_physical_memory_write_rom(rom->addr, rom->data, rom->datasize);
> >> +        }
> > 
> > Hmmm. Why is this (ie. the pre-patch resetting) necessary at all?
> > 
> > Is this due to the writeability of fw_cfg files via the ioport
> > (fw_cfg_write())? I think that modifies "rom->data" unconditionally
> > (which is currently kept separate from the RAMBlock, see above).
> > 
> > So, regarding the patched version:
> > - not sure if the RAMBlock can change at all -- it is neither mapped
> > into guest-phys address space, nor does fw_cfg_write() touch it,
> > - *if* the guest modifies the contents under "rom->addr", via
> > fw_cfg_write(), then the hva-space memcpy() is insufficient.
> 
> Sorry, I'm wrong here. The patched rom_add_file() ensures that
> fw_cfg_write() modifies the correct backing store. Also, we need to keep
> "rom->data" around even if "rom_file_in_ram" is set, because that's
> where we restore the RAMBlock contents from, in case of a reset.
> 
> Laszlo

Exactly.

Re: [Qemu-devel] [PATCH v3 1/2] memory: export migration page size

[Laszlo Ersek]

On 08/19/13 13:18, Michael S. Tsirkin wrote:
> On Mon, Aug 19, 2013 at 01:09:36PM +0200, Laszlo Ersek wrote:
>> On 08/19/13 12:21, Peter Maydell wrote:
>>> On 19 August 2013 10:59, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> On 08/13/13 00:43, Michael S. Tsirkin wrote:
>>>>> Migration code assumes that each RAM block is a multiple of target page
>>>>> size.
>>>>
>>>> Isn't that a valid assumption, considering the TARGET_PAGE_ALIGN() macro
>>>> call in qemu_ram_alloc_from_ptr() [exec.c]?
>>>
>>> That macro only makes the size we store in the ramblock data
>>> structure be a multiple of the page size -- it does nothing to ensure
>>> that the actual memory that was passed in by the caller is the
>>> right size. (It will have the right effect where qemu_ram_alloc_from_ptr
>>> is allocating the memory itself, obviously.)
>>
>> Which is the case for 2/2, see my comments there:
>>
>> memory_region_init_ram()
>>   qemu_ram_alloc()
>>     qemu_ram_alloc_from_ptr()  <---- host==NULL
>>       TARGET_PAGE_ALIGN()
>>
>> Laszlo
> 
> The issue this addresses is not the size of RAM allocated.
> The issue is the size of the MR.
> Migration code assumes the size of the MR
> is a multiple of TARGET_PAGE_SIZE.

You're right. Thanks.
Laszlo

Re: [Qemu-devel] [PATCH v3 0/2] future proof rom loading for cross versiom migration

[Laszlo Ersek]

On 08/13/13 00:43, Michael S. Tsirkin wrote:
> Changes from v2: address comments on v2 by Peter Maydell
>     - switch from global constant to function
>     - use memory_region_init_ram instead of _ram_ptr
>     - disable for 1.6
> 
> Changes from v1: address comments by Peter Maydell
>     - drop useless data=data line
>     - rename target_page_size to migration_page_size to make use clear
>     Peter, you also suggested somehow hiding this within memory core.
>     I don't see a clean way to do this without lots of code
>     changes, I think what I propose here is acceptable for now
>     and we can always rework APIs without wire format changes.
> 
> Please review, and consider for merging.
> 
> Original cover letter below.
> 
> 
> ROM files that are put in FW CFG are copied to guest ram, by BIOS, but
> they are not backed by RAM so they don't get migrated.
> 
> Each time we'll change at least two bytes in such a ROM this will break
> cross-version migration: since we can migrate after BIOS has read the first
> byte but before it has read the second one, getting an inconsistent state.
> 
> This patchset makes QEMU future-proof against such changes.
> 
> Naturally, this only helps for -M 1.6 and up, older machine types
> will still have the cross-version migration bug.
> 
> I think this should be applied for 1.6, this way we won't
> have this problem from 1.7 and on.
> 
> Michael S. Tsirkin (2):
>   memory: export migration page size
>   loader: put FW CFG ROM files into RAM
> 
>  arch_init.c           |  6 ++++++
>  hw/core/loader.c      | 54 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  hw/i386/pc_piix.c     |  2 ++
>  hw/i386/pc_q35.c      |  2 ++
>  include/exec/memory.h |  1 +
>  include/hw/loader.h   |  1 +
>  6 files changed, 63 insertions(+), 3 deletions(-)
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>