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

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

[Michael S. Tsirkin]

Changes from v3:
    - drop new API, all changes are now internal to memory subsystem
      Patch 1/2 is new. Patch 2/2 had minor changes.
      At laszlo's request (offline), here's the diff of patch 2/2
      from v3 to v4:
        diff --git a/hw/core/loader.c b/hw/core/loader.c
        index 32d807a..7b3d3ee 100644
        --- a/hw/core/loader.c
        +++ b/hw/core/loader.c
        @@ -610,15 +610,10 @@ static void rom_insert(Rom *rom)
         
         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_init_ram(rom->mr, owner, name, rom->datasize);
             memory_region_set_readonly(rom->mr, true);
             vmstate_register_ram_global(rom->mr);

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.7 and up, older machine types
will still have the cross-version migration bug.

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

Note: compatibility code depends on patch
'pc: cleanup 1.4 compat support' to work correctly
for machine types 1.5 and back.

Michael S. Tsirkin (2):
  arch_init: align MR size to target page size
  loader: store FW CFG ROM files in RAM

 arch_init.c         |  3 ++-
 hw/core/loader.c    | 49 ++++++++++++++++++++++++++++++++++++++++++++++---
 hw/i386/pc_piix.c   |  2 ++
 hw/i386/pc_q35.c    |  2 ++
 include/hw/loader.h |  1 +
 5 files changed, 53 insertions(+), 4 deletions(-)

-- 
MST

[Qemu-devel] [PATCH v4 1/2] arch_init: align MR size to target page size

[Michael S. Tsirkin]

Migration code assumes that each MR is a multiple of TARGET_PAGE_SIZE:
MR size is divided by TARGET_PAGE_SIZE, so if it isn't migration
never completes.
But this isn't really required for regions set up with
memory_region_init_ram, since that calls qemu_ram_alloc
which aligns size up using TARGET_PAGE_ALIGN.

Align MR size up to full target page sizes, this way
migration completes even if we create a RAM MR
which is not a full target page size.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 arch_init.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch_init.c b/arch_init.c
index 68a7ab7..ac8eb59 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -342,7 +342,8 @@ ram_addr_t migration_bitmap_find_and_reset_dirty(MemoryRegion *mr,
 {
     unsigned long base = mr->ram_addr >> TARGET_PAGE_BITS;
     unsigned long nr = base + (start >> TARGET_PAGE_BITS);
-    unsigned long size = base + (int128_get64(mr->size) >> TARGET_PAGE_BITS);
+    uint64_t mr_size = TARGET_PAGE_ALIGN(memory_region_size(mr));
+    unsigned long size = base + (mr_size >> TARGET_PAGE_BITS);
 
     unsigned long next;
 
-- 
MST

Re: [Qemu-devel] [PATCH v4 1/2] arch_init: align MR size to target page size

[Laszlo Ersek]

On 08/19/13 16:26, Michael S. Tsirkin wrote:
> Migration code assumes that each MR is a multiple of TARGET_PAGE_SIZE:
> MR size is divided by TARGET_PAGE_SIZE, so if it isn't migration
> never completes.
> But this isn't really required for regions set up with
> memory_region_init_ram, since that calls qemu_ram_alloc
> which aligns size up using TARGET_PAGE_ALIGN.
> 
> Align MR size up to full target page sizes, this way
> migration completes even if we create a RAM MR
> which is not a full target page size.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  arch_init.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 68a7ab7..ac8eb59 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -342,7 +342,8 @@ ram_addr_t migration_bitmap_find_and_reset_dirty(MemoryRegion *mr,
>  {
>      unsigned long base = mr->ram_addr >> TARGET_PAGE_BITS;
>      unsigned long nr = base + (start >> TARGET_PAGE_BITS);
> -    unsigned long size = base + (int128_get64(mr->size) >> TARGET_PAGE_BITS);
> +    uint64_t mr_size = TARGET_PAGE_ALIGN(memory_region_size(mr));
> +    unsigned long size = base + (mr_size >> TARGET_PAGE_BITS);
>  
>      unsigned long next;
>  
> 

(1) The patch (and the update to 2/2) seem correct to me.

(2) But is this patch complete?

Long version:

(1) The "only" danger in migration_bitmap_find_and_reset_dirty(),
AFAICS, is over-subscripting "migration_bitmap" with find_next_bit().

However, ram_save_setup() seems to initialize "migration_bitmap" for
"ram_pages" bits, and "ram_pages" comes from last_ram_offset().

last_ram_offset() in turn finds the highest offset any RAMBlock has.

The RAMBlock backing the fw_cfg file has already rounded-up size, so I
think "migration_bitmap" will have a bit allocated for the last
(possibly not fully populated) page of any fw_cfg RAMBlock. So this
patch should be correct.


(2) Regarding completeness, are we sure that nothing else depends on
mr->size being an integer multiple of TARGET_PAGE_SIZE?

I think v3 is perhaps less intrusive (as in, it doesn't raise (2)).


((3) memory_region_size() is slightly different from
int128_get64(mr->size); it has a special case for int128_2_64() -- and I
don't understand that. int128_2_64() represents 2 raised to the power of
64. It seems to be the replacement for UINT64_MAX.)

Thanks
Laszlo

Re: [Qemu-devel] [PATCH v4 1/2] arch_init: align MR size to target page size

[Peter Maydell]

On 19 August 2013 18:37, Laszlo Ersek <lersek@redhat.com> wrote:
> ((3) memory_region_size() is slightly different from
> int128_get64(mr->size); it has a special case for int128_2_64() -- and I
> don't understand that.

The special case is because valid memory region sizes range
from 0 to 2^64, *inclusive*. [2^64-sized regions tend to be
containers representing address spaces like PCI or the system
memory space.] Inside memory.c we store the size
in an int128 in the way you'd expect. However some of the
memory region APIs take or return the size of the region
as a uint64_t, with the convention that UINT64_MAX means
"actually 2^64" (with a size of really 2^64 - 1 not being
valid). So the special case here is doing the conversion
from an int128 representation of the size to the uint64_t
representation. You can see the same thing going the other
way in memory_region_init(), where we take the size as a
uint64_t and convert it to an int128 with

    mr->size = int128_make64(size);
    if (size == UINT64_MAX) {
        mr->size = int128_2_64();
    }

(Note that int128_get64() of an int128 which == 2^64 or
more will assert, so int128_get64(mr->size) is a bit of a
code smell; it happens to be OK here because we know that a
RAM-backed MR will never be a 2^64-sized region.)

-- PMM

Re: [Qemu-devel] [PATCH v4 1/2] arch_init: align MR size to target page size

[Michael S. Tsirkin]

On Mon, Aug 19, 2013 at 06:45:10PM +0100, Peter Maydell wrote:
> On 19 August 2013 18:37, Laszlo Ersek <lersek@redhat.com> wrote:
> > ((3) memory_region_size() is slightly different from
> > int128_get64(mr->size); it has a special case for int128_2_64() -- and I
> > don't understand that.
> 
> The special case is because valid memory region sizes range
> from 0 to 2^64, *inclusive*. [2^64-sized regions tend to be
> containers representing address spaces like PCI or the system
> memory space.] Inside memory.c we store the size
> in an int128 in the way you'd expect. However some of the
> memory region APIs take or return the size of the region
> as a uint64_t, with the convention that UINT64_MAX means
> "actually 2^64" (with a size of really 2^64 - 1 not being
> valid). So the special case here is doing the conversion
> from an int128 representation of the size to the uint64_t
> representation. You can see the same thing going the other
> way in memory_region_init(), where we take the size as a
> uint64_t and convert it to an int128 with
> 
>     mr->size = int128_make64(size);
>     if (size == UINT64_MAX) {
>         mr->size = int128_2_64();
>     }
> 
> (Note that int128_get64() of an int128 which == 2^64 or
> more will assert, so int128_get64(mr->size) is a bit of a
> code smell; it happens to be OK here because we know that a
> RAM-backed MR will never be a 2^64-sized region.)
> 
> -- PMM

Note code smell is in the original code -
this patch uses the API.
We could add memory_region_get_target_pages
as that will fit in uint64_t.
But other migration code shifts it right back by target page size,
so I don't think it's worth it.

Re: [Qemu-devel] [PATCH v4 1/2] arch_init: align MR size to target page size

[Michael S. Tsirkin]

On Mon, Aug 19, 2013 at 07:37:44PM +0200, Laszlo Ersek wrote:
> On 08/19/13 16:26, Michael S. Tsirkin wrote:
> > Migration code assumes that each MR is a multiple of TARGET_PAGE_SIZE:
> > MR size is divided by TARGET_PAGE_SIZE, so if it isn't migration
> > never completes.
> > But this isn't really required for regions set up with
> > memory_region_init_ram, since that calls qemu_ram_alloc
> > which aligns size up using TARGET_PAGE_ALIGN.
> > 
> > Align MR size up to full target page sizes, this way
> > migration completes even if we create a RAM MR
> > which is not a full target page size.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  arch_init.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch_init.c b/arch_init.c
> > index 68a7ab7..ac8eb59 100644
> > --- a/arch_init.c
> > +++ b/arch_init.c
> > @@ -342,7 +342,8 @@ ram_addr_t migration_bitmap_find_and_reset_dirty(MemoryRegion *mr,
> >  {
> >      unsigned long base = mr->ram_addr >> TARGET_PAGE_BITS;
> >      unsigned long nr = base + (start >> TARGET_PAGE_BITS);
> > -    unsigned long size = base + (int128_get64(mr->size) >> TARGET_PAGE_BITS);
> > +    uint64_t mr_size = TARGET_PAGE_ALIGN(memory_region_size(mr));
> > +    unsigned long size = base + (mr_size >> TARGET_PAGE_BITS);
> >  
> >      unsigned long next;
> >  
> > 
> 
> (1) The patch (and the update to 2/2) seem correct to me.
> 
> (2) But is this patch complete?
> 
> Long version:
> 
> (1) The "only" danger in migration_bitmap_find_and_reset_dirty(),
> AFAICS, is over-subscripting "migration_bitmap" with find_next_bit().
> 
> However, ram_save_setup() seems to initialize "migration_bitmap" for
> "ram_pages" bits, and "ram_pages" comes from last_ram_offset().
> 
> last_ram_offset() in turn finds the highest offset any RAMBlock has.
> 
> The RAMBlock backing the fw_cfg file has already rounded-up size, so I
> think "migration_bitmap" will have a bit allocated for the last
> (possibly not fully populated) page of any fw_cfg RAMBlock. So this
> patch should be correct.
> 
> 
> (2) Regarding completeness, are we sure that nothing else depends on
> mr->size being an integer multiple of TARGET_PAGE_SIZE?

There's no requirement that mr->size is a multiple of TARGET_PAGE_SIZE.
The only requirement is for a RAM mr size, and that
comes from migration. Even that is simply a bug.


> I think v3 is perhaps less intrusive (as in, it doesn't raise (2)).

Yes but it's early days in the 1.7 cycle so I think it makes
sense to opt for a cleaner/smaller API even if this might trigger
some latent bugs.

> 
> ((3) memory_region_size() is slightly different from
> int128_get64(mr->size); it has a special case for int128_2_64() -- and I
> don't understand that. int128_2_64() represents 2 raised to the power of
> 64. It seems to be the replacement for UINT64_MAX.)
> 
> Thanks
> Laszlo

I think this is to represent things like PCI regions which can
in theory cover the whole 64 bit range.
You can't represent size of the whole 64 bit range in a 64 bit
integer.
We can't migrate RAM that large so no real issue.


-- 
MST

Re: [Qemu-devel] [PATCH v4 1/2] arch_init: align MR size to target page size

[Laszlo Ersek]

On 08/19/13 19:48, Michael S. Tsirkin wrote:
> On Mon, Aug 19, 2013 at 07:37:44PM +0200, Laszlo Ersek wrote:
>> On 08/19/13 16:26, Michael S. Tsirkin wrote:
>>> Migration code assumes that each MR is a multiple of TARGET_PAGE_SIZE:
>>> MR size is divided by TARGET_PAGE_SIZE, so if it isn't migration
>>> never completes.
>>> But this isn't really required for regions set up with
>>> memory_region_init_ram, since that calls qemu_ram_alloc
>>> which aligns size up using TARGET_PAGE_ALIGN.
>>>
>>> Align MR size up to full target page sizes, this way
>>> migration completes even if we create a RAM MR
>>> which is not a full target page size.
>>>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>>  arch_init.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch_init.c b/arch_init.c
>>> index 68a7ab7..ac8eb59 100644
>>> --- a/arch_init.c
>>> +++ b/arch_init.c
>>> @@ -342,7 +342,8 @@ ram_addr_t migration_bitmap_find_and_reset_dirty(MemoryRegion *mr,
>>>  {
>>>      unsigned long base = mr->ram_addr >> TARGET_PAGE_BITS;
>>>      unsigned long nr = base + (start >> TARGET_PAGE_BITS);
>>> -    unsigned long size = base + (int128_get64(mr->size) >> TARGET_PAGE_BITS);
>>> +    uint64_t mr_size = TARGET_PAGE_ALIGN(memory_region_size(mr));
>>> +    unsigned long size = base + (mr_size >> TARGET_PAGE_BITS);
>>>  
>>>      unsigned long next;
>>>  
>>>
>>
>> (1) The patch (and the update to 2/2) seem correct to me.
>>
>> (2) But is this patch complete?
>>
>> Long version:
>>
>> (1) The "only" danger in migration_bitmap_find_and_reset_dirty(),
>> AFAICS, is over-subscripting "migration_bitmap" with find_next_bit().
>>
>> However, ram_save_setup() seems to initialize "migration_bitmap" for
>> "ram_pages" bits, and "ram_pages" comes from last_ram_offset().
>>
>> last_ram_offset() in turn finds the highest offset any RAMBlock has.
>>
>> The RAMBlock backing the fw_cfg file has already rounded-up size, so I
>> think "migration_bitmap" will have a bit allocated for the last
>> (possibly not fully populated) page of any fw_cfg RAMBlock. So this
>> patch should be correct.
>>
>>
>> (2) Regarding completeness, are we sure that nothing else depends on
>> mr->size being an integer multiple of TARGET_PAGE_SIZE?
> 
> There's no requirement that mr->size is a multiple of TARGET_PAGE_SIZE.
> The only requirement is for a RAM mr size, and that
> comes from migration. Even that is simply a bug.
> 
> 
>> I think v3 is perhaps less intrusive (as in, it doesn't raise (2)).
> 
> Yes but it's early days in the 1.7 cycle so I think it makes
> sense to opt for a cleaner/smaller API even if this might trigger
> some latent bugs.
> 
>>
>> ((3) memory_region_size() is slightly different from
>> int128_get64(mr->size); it has a special case for int128_2_64() -- and I
>> don't understand that. int128_2_64() represents 2 raised to the power of
>> 64. It seems to be the replacement for UINT64_MAX.)
>>
>> Thanks
>> Laszlo
> 
> I think this is to represent things like PCI regions which can
> in theory cover the whole 64 bit range.
> You can't represent size of the whole 64 bit range in a 64 bit
> integer.
> We can't migrate RAM that large so no real issue.

Okay then.

By now both Peter and Paolo should be willing to R-b the v4 series;
here's mine:

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

[Qemu-devel] [PATCH v4 2/2] loader: store FW CFG ROM files in 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    | 49 ++++++++++++++++++++++++++++++++++++++++++++++---
 hw/i386/pc_piix.c   |  2 ++
 hw/i386/pc_q35.c    |  2 ++
 include/hw/loader.h |  1 +
 4 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 6875b7e..7b3d3ee 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,21 @@ static void rom_insert(Rom *rom)
     QTAILQ_INSERT_TAIL(&roms, rom, next);
 }
 
+static void *rom_set_mr(Rom *rom, Object *owner, const char *name)
+{
+    void *data;
+
+    rom->mr = g_malloc(sizeof(*rom->mr));
+    memory_region_init_ram(rom->mr, owner, name, rom->datasize);
+    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 +664,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 +674,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 +757,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 +812,9 @@ static Rom *find_rom(hwaddr addr)
         if (rom->fw_file) {
             continue;
         }
+        if (rom->mr) {
+            continue;
+        }
         if (rom->addr > addr) {
             continue;
         }
@@ -808,6 +842,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 +903,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 v4 2/2] loader: store FW CFG ROM files in RAM

[Paolo Bonzini]

Il 19/08/2013 16:26, Michael S. Tsirkin ha scritto:
> 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>

This doesn't really help much unless we support migration of
different-sized RAM regions, does it?  In particular, what happens if
the region is smaller on the destination?  Do we get garbage written out
of a malloc-ed region's bounds?

The patches look good, but I'm not sure they are an improvement yet.

Paolo

Re: [Qemu-devel] [PATCH v4 2/2] loader: store FW CFG ROM files in RAM

[Michael S. Tsirkin]

On Mon, Aug 19, 2013 at 07:28:12PM +0200, Paolo Bonzini wrote:
> Il 19/08/2013 16:26, Michael S. Tsirkin ha scritto:
> > 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>
> 
> This doesn't really help much unless we support migration of
> different-sized RAM regions, does it?

It does because RAM blocks are multiples of target pages, so the size
won't change all that much. This is more or less similar to what happens
e.g. with BIOS (aligned to power of two).
It worked for BIOS for a while.


> In particular, what happens if
> the region is smaller on the destination?  Do we get garbage written out
> of a malloc-ed region's bounds?

No, migration fails if block size does not match.

> The patches look good, but I'm not sure they are an improvement yet.
> 
> Paolo

Well they protect us against minor changes which are
IMO more likely than major changes.

We can add code to allow ram block size changes on top.
I'm not sure it's a requirement right now.

-- 
MST

Re: [Qemu-devel] [PATCH v4 2/2] loader: store FW CFG ROM files in RAM

[Paolo Bonzini]

Il 19/08/2013 19:40, Michael S. Tsirkin ha scritto:
> On Mon, Aug 19, 2013 at 07:28:12PM +0200, Paolo Bonzini wrote:
>> Il 19/08/2013 16:26, Michael S. Tsirkin ha scritto:
>>> 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>
>>
>> This doesn't really help much unless we support migration of
>> different-sized RAM regions, does it?
> 
> It does because RAM blocks are multiples of target pages, so the size
> won't change all that much. This is more or less similar to what happens
> e.g. with BIOS (aligned to power of two).
> It worked for BIOS for a while.

Yeah, it should work.

On one hand there are many different files, so many different points of
failures.  On the other hand most of the files are small, so the
almost-4 KiB legroom is not bad.

> No, migration fails if block size does not match.

Good.

> Well they protect us against minor changes which are
> IMO more likely than major changes.
> 
> We can add code to allow ram block size changes on top.
> I'm not sure it's a requirement right now.

No, it's not---thanks for the clarifications.

Paolo

Re: [Qemu-devel] [PATCH v4 2/2] loader: store FW CFG ROM files in RAM

[Laszlo Ersek]

On 08/19/13 16:26, 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.
> 
> 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    | 49 ++++++++++++++++++++++++++++++++++++++++++++++---
>  hw/i386/pc_piix.c   |  2 ++
>  hw/i386/pc_q35.c    |  2 ++
>  include/hw/loader.h |  1 +
>  4 files changed, 51 insertions(+), 3 deletions(-)

checked the v3->v4 diff in v4 0/2.

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

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

[Peter Maydell]

On 19 August 2013 15:26, Michael S. Tsirkin <mst@redhat.com> wrote:
> Changes from v3:
>     - drop new API, all changes are now internal to memory subsystem
>       Patch 1/2 is new. Patch 2/2 had minor changes.

I'm happy with these from a memory system api point of view,
though I don't know enough about migration to review the
other aspects of the patch.

thanks
-- PMM