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

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

[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'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 target page size
  loader: put FW CFG ROM files into RAM

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

-- 
MST

[Qemu-devel] [PATCH for-1.6 1/2] memory: export target page size

[Michael S. Tsirkin]

Add symbol to make it possible to use target page size
in target-independent code.

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

diff --git a/exec.c b/exec.c
index 3ca9381..965076e 100644
--- a/exec.c
+++ b/exec.c
@@ -2659,6 +2659,8 @@ bool virtio_is_big_endian(void)
 
 #endif
 
+uint64_t qemu_target_page_size = TARGET_PAGE_SIZE;
+
 #ifndef CONFIG_USER_ONLY
 bool cpu_physical_memory_is_io(hwaddr phys_addr)
 {
diff --git a/include/exec/memory.h b/include/exec/memory.h
index ebe0d24..3ed747c 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1056,6 +1056,8 @@ void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
                          int is_write, hwaddr access_len);
 
 
+extern uint64_t qemu_target_page_size;
+
 #endif
 
 #endif
-- 
MST

Re: [Qemu-devel] [PATCH for-1.6 1/2] memory: export target page size

[Peter Maydell]

On 12 August 2013 09:49, Michael S. Tsirkin <mst@redhat.com> wrote:
> Add symbol to make it possible to use target page size
> in target-independent code.

Given that TARGET_PAGE_SIZE is "smallest page size the
TCG implementation currently supports for this core"
(ie it is a TCG internal implementation detail as much
as a property of the target CPU and it may well not
match the actual page size being used by the guest)
I would be very suspicious of any target-independent
code that uses it.

-- PMM

Re: [Qemu-devel] [PATCH for-1.6 1/2] memory: export target page size

[Michael S. Tsirkin]

On Mon, Aug 12, 2013 at 10:17:46AM +0100, Peter Maydell wrote:
> On 12 August 2013 09:49, Michael S. Tsirkin <mst@redhat.com> wrote:
> > Add symbol to make it possible to use target page size
> > in target-independent code.
> 
> Given that TARGET_PAGE_SIZE is "smallest page size the
> TCG implementation currently supports for this core"
> (ie it is a TCG internal implementation detail as much
> as a property of the target CPU and it may well not
> match the actual page size being used by the guest)
> I would be very suspicious of any target-independent
> code that uses it.
> 
> -- PMM

Look at the next patch and the comment justifying its use please.
Point is migration works in units of TARGET_PAGE_SIZE
and expects all RAM blocks to be a multiple of that.

What if we rename this to qemu_migration_page_size ?
Will this address your comment?

-- 
MST

[Qemu-devel] [PATCH for-1.6 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.6 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.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Laszlo Ersek <lersek@redhat.com>
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..6e99750 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 target pages.
+     * Round MR size up to make this work.
+     */
+    unsigned size = ROUND_UP(rom->datasize, qemu_target_page_size);
+    void *data = g_malloc0(size);
+
+    memcpy(data, rom->data, rom->datasize);
+
+    rom->mr = g_malloc(sizeof(*rom->mr));
+    data = data;
+    memory_region_init_ram_ptr(rom->mr, owner, name, size, data);
+    memory_region_set_readonly(rom->mr, true);
+    vmstate_register_ram_global(rom->mr);
+
+    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..f654977 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"
@@ -257,6 +258,7 @@ static void pc_init_pci_1_6(QEMUMachineInitArgs *args)
 
 static void pc_init_pci_1_5(QEMUMachineInitArgs *args)
 {
+    rom_file_in_ram = false;
     pc_init_pci_1_6(args);
 }
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 6bfc2ca..6848c7b 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"
@@ -225,6 +226,7 @@ static void pc_q35_init_1_6(QEMUMachineInitArgs *args)
 
 static void pc_q35_init_1_5(QEMUMachineInitArgs *args)
 {
+    rom_file_in_ram = false;
     pc_q35_init_1_6(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 for-1.6 2/2] loader: put FW CFG ROM files into RAM

[Peter Maydell]

On 12 August 2013 09:49, Michael S. Tsirkin <mst@redhat.com> wrote:
> +static void *rom_set_mr(Rom *rom, Object *owner, const char *name)
> +{
> +    /*
> +     * Migration code expects that all RAM blocks are full target pages.
> +     * Round MR size up to make this work.
> +     */
> +    unsigned size = ROUND_UP(rom->datasize, qemu_target_page_size);
> +    void *data = g_malloc0(size);

If we don't really care where the data lives (ie we are just
allocating a block for it here) it would be better to get the
memory subsystem to do the allocation, because then the information
about the constraints on the size of the region would be confined
to the memory system.

> +    data = data;

Huh?

-- PMM

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

[Michael S. Tsirkin]

On Mon, Aug 12, 2013 at 10:32:40AM +0100, Peter Maydell wrote:
> On 12 August 2013 09:49, Michael S. Tsirkin <mst@redhat.com> wrote:
> > +static void *rom_set_mr(Rom *rom, Object *owner, const char *name)
> > +{
> > +    /*
> > +     * Migration code expects that all RAM blocks are full target pages.
> > +     * Round MR size up to make this work.
> > +     */
> > +    unsigned size = ROUND_UP(rom->datasize, qemu_target_page_size);
> > +    void *data = g_malloc0(size);
> 
> If we don't really care where the data lives (ie we are just
> allocating a block for it here) it would be better to get the
> memory subsystem to do the allocation, because then the information
> about the constraints on the size of the region would be confined
> to the memory system.

OK but that looks like a bigger patch. I'm looking
for a small fix that we can make for 1.6.
APIs are easy to tweak migration on-wire format is
set in stone on release.

OK

> > +    data = data;
> 
> Huh?
> 
> -- PMM

I'll drop this in v2 but let's figure out what's acceptable.

-- 
MST

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

[Anthony Liguori]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> 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.

This is not for 1.6.  It's far too late to make a change like this.

Regards,

Anthony Liguori

>
> Michael S. Tsirkin (2):
>   memory: export target page size
>   loader: put FW CFG ROM files into RAM
>
>  exec.c                |  2 ++
>  hw/core/loader.c      | 54 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  hw/i386/pc_piix.c     |  2 ++
>  hw/i386/pc_q35.c      |  2 ++
>  include/exec/memory.h |  2 ++
>  include/hw/loader.h   |  1 +
>  6 files changed, 60 insertions(+), 3 deletions(-)
>
> -- 
> MST