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

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

[Michael S. Tsirkin]

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 1.6
    and we can rework APIs in time for 1.7.

Please review, and consider for 1.6.

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           |  3 +++
 hw/core/loader.c      | 53 ++++++++++++++++++++++++++++++++++++++++++++++++---
 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, 59 insertions(+), 3 deletions(-)

-- 
MST

[Qemu-devel] [PATCH v2 for-1.6 v2 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, for 1.6 the simplest way is
possibly exporting the required page size so callers can make regions
large enough.

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

diff --git a/arch_init.c b/arch_init.c
index 68a7ab7..61937e2 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -150,6 +150,9 @@ int qemu_read_default_config_files(bool userconfig)
     return 0;
 }
 
+/* Smallest page size for migrated RAM. */
+uint64_t qemu_migration_page_size = 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..42450fe 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;
 
 #endif
 
-- 
MST

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

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

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 6875b7e..9f83aa8 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,25 @@ 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 = g_malloc0(size);
+
+    memcpy(data, rom->data, rom->datasize);
+
+    rom->mr = g_malloc(sizeof(*rom->mr));
+    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 +668,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 +678,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 +761,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 +816,9 @@ static Rom *find_rom(hwaddr addr)
         if (rom->fw_file) {
             continue;
         }
+        if (rom->mr) {
+            continue;
+        }
         if (rom->addr > addr) {
             continue;
         }
@@ -808,6 +846,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 +907,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 v2 for-1.6 0/2] future proof rom loading for cross versiom migration

[Michael S. Tsirkin]

On Mon, Aug 12, 2013 at 09:15:55PM +0300, Michael S. Tsirkin wrote:
> 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 1.6
>     and we can rework APIs in time for 1.7.
> 
> Please review, and consider for 1.6.

This was stuck in my outbound mail queue - wrote this
before I saw Anthony nack this change for 1.6.

So pls ignore the 1.6 tag and review for 1.7.

Thanks, and sorry about the noise.

> 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           |  3 +++
>  hw/core/loader.c      | 53 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  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, 59 insertions(+), 3 deletions(-)
> 
> -- 
> MST
> 

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

[Peter Maydell]

On 12 August 2013 19:16, 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 pages.
> +     * Round MR size up to satisfy this condition.
> +     */
> +    unsigned size = ROUND_UP(rom->datasize, qemu_migration_page_size);
> +    void *data = g_malloc0(size);
> +
> +    memcpy(data, rom->data, rom->datasize);
> +
> +    rom->mr = g_malloc(sizeof(*rom->mr));
> +    memory_region_init_ram_ptr(rom->mr, owner, name, size, data);
> +    memory_region_set_readonly(rom->mr, true);
> +    vmstate_register_ram_global(rom->mr);

So having thought about this a little I think the right answer
here is "don't use memory_region_init_ram_ptr()". At the moment
in-tree we have five users of this function:

hw/display/g364fb.c
hw/i386/kvm/pci-assign.c
hw/misc/ivshmem.c
hw/misc/vfio.c
target-ppc/kvm.c

The last four of these all absolutely have to have the guest
use a specific host pointer, typically the result of mmap()ing
something [shared file, PCI device, KVM_ALLOCATE_RMA fd, etc].
The first one I think should be converted to use
memory_region_init_ram() instead, because it doesn't need
to use a particular buffer.

Similarly, what you're trying to do here doesn't require
that the guest sees any specific host pointer, so you should
just use memory_region_init_ram().

We should add an assert to the _init_ram_ptr functions that
checks that the size is OK, as well.

I seem to recall having a conversation with Paolo along these
lines a few months back (we fixed the exynos devices which
were incorrectly using the _ram_ptr function); he can correct
me if I'm off-base here.

-- PMM

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

[Michael S. Tsirkin]

On Mon, Aug 12, 2013 at 07:37:21PM +0100, Peter Maydell wrote:
> On 12 August 2013 19:16, 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 pages.
> > +     * Round MR size up to satisfy this condition.
> > +     */
> > +    unsigned size = ROUND_UP(rom->datasize, qemu_migration_page_size);
> > +    void *data = g_malloc0(size);
> > +
> > +    memcpy(data, rom->data, rom->datasize);
> > +
> > +    rom->mr = g_malloc(sizeof(*rom->mr));
> > +    memory_region_init_ram_ptr(rom->mr, owner, name, size, data);
> > +    memory_region_set_readonly(rom->mr, true);
> > +    vmstate_register_ram_global(rom->mr);
> 
> So having thought about this a little I think the right answer
> here is "don't use memory_region_init_ram_ptr()". At the moment
> in-tree we have five users of this function:
> 
> hw/display/g364fb.c
> hw/i386/kvm/pci-assign.c
> hw/misc/ivshmem.c
> hw/misc/vfio.c
> target-ppc/kvm.c
> 
> The last four of these all absolutely have to have the guest
> use a specific host pointer, typically the result of mmap()ing
> something [shared file, PCI device, KVM_ALLOCATE_RMA fd, etc].
> The first one I think should be converted to use
> memory_region_init_ram() instead, because it doesn't need
> to use a particular buffer.
> 
> Similarly, what you're trying to do here doesn't require
> that the guest sees any specific host pointer, so you should
> just use memory_region_init_ram().

I was concerned that we are wasting resources here.
In particular, huge page memory might get allocated
and there's no need for it as it's never mapped
into guest.

Still if Paolo is OK with this too, I'll switch, and do
+ memory_region_init_ram_ptr(...)
+ data = memory_region_get_ram_ptr(rom->mr);

Paolo could you please confirm?


> 
> We should add an assert to the _init_ram_ptr functions that
> checks that the size is OK, as well.

At least for pci-assign and vfio it's the wrong thing
to do - they block migration so we don't need
the ram to be a multiple of migration page size.

> I seem to recall having a conversation with Paolo along these
> lines a few months back (we fixed the exynos devices which
> were incorrectly using the _ram_ptr function); he can correct
> me if I'm off-base here.
> 
> -- PMM

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

[Paolo Bonzini]

Il 12/08/2013 20:37, Peter Maydell ha scritto:
> 
> I seem to recall having a conversation with Paolo along these
> lines a few months back (we fixed the exynos devices which
> were incorrectly using the _ram_ptr function); he can correct
> me if I'm off-base here.

Yeah, I think you're right.

Paolo

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

[Paolo Bonzini]

Il 12/08/2013 20:56, Michael S. Tsirkin ha scritto:
>> > Similarly, what you're trying to do here doesn't require
>> > that the guest sees any specific host pointer, so you should
>> > just use memory_region_init_ram().
> I was concerned that we are wasting resources here.
> In particular, huge page memory might get allocated
> and there's no need for it as it's never mapped
> into guest.

You are also right, but I think this issue should be fixed separately
(e.g. only use huge pages for main RAM).

Paolo

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

[Michael S. Tsirkin]

On Tue, Aug 13, 2013 at 04:47:00PM +0200, Paolo Bonzini wrote:
> Il 12/08/2013 20:56, Michael S. Tsirkin ha scritto:
> >> > Similarly, what you're trying to do here doesn't require
> >> > that the guest sees any specific host pointer, so you should
> >> > just use memory_region_init_ram().
> > I was concerned that we are wasting resources here.
> > In particular, huge page memory might get allocated
> > and there's no need for it as it's never mapped
> > into guest.
> 
> You are also right, but I think this issue should be fixed separately
> (e.g. only use huge pages for main RAM).
> 
> Paolo

OK.
I switched to memory_region_init_ram for v3.