[Qemu-devel] [PATCH for-2.1 0/2] Fix migration failure due to ACPI tables size changes

[Qemu-devel] [PATCH for-2.1 0/2] Fix migration failure due to ACPI tables size changes

[Igor Mammedov]

Changing the ACPI table size causes migration to break, and the memory
hotplug work opened our eyes on how horribly we were breaking things in
2.0 already.

To trigger issue start
  QEMU-1.7 with -M pc-i440fx-1.7 -device pci-bridge,chassis_nr=1
and try to migrate to QEMU-2.1 or QEMU-2.0 as result target will fail with:
  qemu-system-x86_64: Length mismatch: /rom@etc/acpi/tables: 2000 in != 3000

This fix allows target QEMU to load smaller RAMBlock into a bigger one
and fixes regression which was introduced since 2.0, allowing
forward migration from 1.7/2.0 to 2.1
Fix is also suitable for stable-2.0.

Igor Mammedov (2):
  migration: load smaller RAMBlock to a bigger one if permitted
  acpi: mark ACPI tables ROM blob as extend-able on migration

 arch_init.c             | 19 ++++++++++++++-----
 exec.c                  | 15 +++++++++------
 hw/core/loader.c        |  6 +++++-
 hw/i386/acpi-build.c    |  2 +-
 include/exec/cpu-all.h  | 15 ++++++++++++++-
 include/exec/memory.h   | 10 ++++++++++
 include/exec/ram_addr.h |  1 +
 include/hw/loader.h     |  5 +++--
 memory.c                |  5 +++++
 9 files changed, 62 insertions(+), 16 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH for-2.1 1/2] migration: load smaller RAMBlock to a bigger one if permitted

[Igor Mammedov]

Add API to mark memory region as extend-able on migration,
to allow migration code to load smaller RAMBlock into
a bigger one on destination QEMU instance.

This will allow to fix broken migration from QEMU 1.7/2.0 to
QEMU 2.1 due to  ACPI tables size changes across 1.7/2.0/2.1
versions by marking ACPI tables ROM blob as extend-able.
So that smaller tables from previos version could be always
migrated to a bigger rom blob on new version.

Credits-for-idea: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 arch_init.c             | 19 ++++++++++++++-----
 exec.c                  | 15 +++++++++------
 include/exec/cpu-all.h  | 15 ++++++++++++++-
 include/exec/memory.h   | 10 ++++++++++
 include/exec/ram_addr.h |  1 +
 memory.c                |  5 +++++
 6 files changed, 53 insertions(+), 12 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 8ddaf35..812f8b5 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -1071,11 +1071,20 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
 
                 QTAILQ_FOREACH(block, &ram_list.blocks, next) {
                     if (!strncmp(id, block->idstr, sizeof(id))) {
-                        if (block->length != length) {
-                            error_report("Length mismatch: %s: " RAM_ADDR_FMT
-                                         " in != " RAM_ADDR_FMT, id, length,
-                                         block->length);
-                            ret =  -EINVAL;
+                        if (block->flags & RAM_EXTENDABLE_ON_MIGRATE) {
+                            if (block->length < length) {
+                                error_report("Length too big: %s: "RAM_ADDR_FMT
+                                             " > " RAM_ADDR_FMT, id, length,
+                                             block->length);
+                                ret =  -EINVAL;
+                            }
+                        } else {
+                            if (block->length != length) {
+                                error_report("Length mismatch: %s: "RAM_ADDR_FMT
+                                             " in != " RAM_ADDR_FMT, id, length,
+                                             block->length);
+                                ret =  -EINVAL;
+                            }
                         }
                         break;
                     }
diff --git a/exec.c b/exec.c
index 765bd94..755b1d3 100644
--- a/exec.c
+++ b/exec.c
@@ -69,12 +69,6 @@ AddressSpace address_space_memory;
 MemoryRegion io_mem_rom, io_mem_notdirty;
 static MemoryRegion io_mem_unassigned;
 
-/* RAM is pre-allocated and passed into qemu_ram_alloc_from_ptr */
-#define RAM_PREALLOC   (1 << 0)
-
-/* RAM is mmap-ed with MAP_SHARED */
-#define RAM_SHARED     (1 << 1)
-
 #endif
 
 struct CPUTailQ cpus = QTAILQ_HEAD_INITIALIZER(cpus);
@@ -1214,6 +1208,15 @@ void qemu_ram_unset_idstr(ram_addr_t addr)
     }
 }
 
+void qemu_ram_set_extendable_on_migration(ram_addr_t addr)
+{
+    RAMBlock *block = find_ram_block(addr);
+
+    if (block) {
+        block->flags |= RAM_EXTENDABLE_ON_MIGRATE;
+    }
+}
+
 static int memory_try_enable_merging(void *addr, size_t len)
 {
     if (!qemu_opt_get_bool(qemu_get_machine_opts(), "mem-merge", true)) {
diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index f91581f..2b9aea3 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -296,13 +296,26 @@ CPUArchState *cpu_copy(CPUArchState *env);
 #if !defined(CONFIG_USER_ONLY)
 
 /* memory API */
+typedef enum {
+    /* RAM is pre-allocated and passed into qemu_ram_alloc_from_ptr */
+    RAM_PREALLOC = 1 << 0,
+    /* RAM is mmap-ed with MAP_SHARED */
+    RAM_SHARED = 1 << 1,
+    /*
+     * Allow at migration time to load RAMBlock of smaller size than
+     * destination RAMBlock is
+     */
+    RAM_EXTENDABLE_ON_MIGRATE = 1 << 2,
+    RAM_FLAGS_END = 1 << 31
+} RAMBlockFlags;
+
 
 typedef struct RAMBlock {
     struct MemoryRegion *mr;
     uint8_t *host;
     ram_addr_t offset;
     ram_addr_t length;
-    uint32_t flags;
+    RAMBlockFlags flags;
     char idstr[256];
     /* Reads can take either the iothread or the ramlist lock.
      * Writes must take both locks.
diff --git a/include/exec/memory.h b/include/exec/memory.h
index e2c8e3e..6c03f70 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -894,6 +894,16 @@ bool memory_region_present(MemoryRegion *container, hwaddr addr);
 bool memory_region_is_mapped(MemoryRegion *mr);
 
 /**
+ * memory_region_permit_extendable_migration: allows to mark
+ * #MemoryRegion as extendable on migrartion, which permits to
+ * load source memory block of smaller size than destination memory block
+ * at migration time
+ *
+ * @mr: a #MemoryRegion which should be marked as extendable on migration
+ */
+void memory_region_permit_extendable_migration(MemoryRegion *mr);
+
+/**
  * memory_region_find: translate an address/size relative to a
  * MemoryRegion into a #MemoryRegionSection.
  *
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 6593be1..248c075 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -33,6 +33,7 @@ void *qemu_get_ram_block_host_ptr(ram_addr_t addr);
 void *qemu_get_ram_ptr(ram_addr_t addr);
 void qemu_ram_free(ram_addr_t addr);
 void qemu_ram_free_from_ptr(ram_addr_t addr);
+void qemu_ram_set_extendable_on_migration(ram_addr_t addr);
 
 static inline bool cpu_physical_memory_get_dirty(ram_addr_t start,
                                                  ram_addr_t length,
diff --git a/memory.c b/memory.c
index 64d7176..744c746 100644
--- a/memory.c
+++ b/memory.c
@@ -1791,6 +1791,11 @@ bool memory_region_is_mapped(MemoryRegion *mr)
     return mr->container ? true : false;
 }
 
+void memory_region_permit_extendable_migration(MemoryRegion *mr)
+{
+    qemu_ram_set_extendable_on_migration(mr->ram_addr);
+}
+
 MemoryRegionSection memory_region_find(MemoryRegion *mr,
                                        hwaddr addr, uint64_t size)
 {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH for-2.1 1/2] migration: load smaller RAMBlock to a bigger one if permitted

[Laszlo Ersek]

On 07/25/14 17:48, Igor Mammedov wrote:

> Add API to mark memory region as extend-able on migration,
> to allow migration code to load smaller RAMBlock into
> a bigger one on destination QEMU instance.
>
> This will allow to fix broken migration from QEMU 1.7/2.0 to
> QEMU 2.1 due to  ACPI tables size changes across 1.7/2.0/2.1
> versions by marking ACPI tables ROM blob as extend-able.
> So that smaller tables from previos version could be always

("previous")

> migrated to a bigger rom blob on new version.
>
> Credits-for-idea: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  arch_init.c             | 19 ++++++++++++++-----
>  exec.c                  | 15 +++++++++------
>  include/exec/cpu-all.h  | 15 ++++++++++++++-
>  include/exec/memory.h   | 10 ++++++++++
>  include/exec/ram_addr.h |  1 +
>  memory.c                |  5 +++++
>  6 files changed, 53 insertions(+), 12 deletions(-)

(Please pass the -O flag to git-format-patch, with an order file that
moves header files (*.h) to the front. Header hunks should lead in a
patch. Thanks.)

> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 6593be1..248c075 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -33,6 +33,7 @@ void *qemu_get_ram_block_host_ptr(ram_addr_t addr);
>  void *qemu_get_ram_ptr(ram_addr_t addr);
>  void qemu_ram_free(ram_addr_t addr);
>  void qemu_ram_free_from_ptr(ram_addr_t addr);
> +void qemu_ram_set_extendable_on_migration(ram_addr_t addr);
>
>  static inline bool cpu_physical_memory_get_dirty(ram_addr_t start,
>                                                   ram_addr_t length,

(Ugh. The declarations (prototypes) of qemu_ram_*() functions are
scattered between "ram_addr.h" and "cpu-common.h" (both under
include/exec). I can't see why that is a good thing (the function
definitions all seem to be in exec.c).)

> diff --git a/exec.c b/exec.c
> index 765bd94..755b1d3 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -69,12 +69,6 @@ AddressSpace address_space_memory;
>  MemoryRegion io_mem_rom, io_mem_notdirty;
>  static MemoryRegion io_mem_unassigned;
>
> -/* RAM is pre-allocated and passed into qemu_ram_alloc_from_ptr */
> -#define RAM_PREALLOC   (1 << 0)
> -
> -/* RAM is mmap-ed with MAP_SHARED */
> -#define RAM_SHARED     (1 << 1)
> -
>  #endif

I'm not sure these macros should be replaced with an enum; the new flag
could be introduced just following the existent pattern.

>
>  struct CPUTailQ cpus = QTAILQ_HEAD_INITIALIZER(cpus);
> @@ -1214,6 +1208,15 @@ void qemu_ram_unset_idstr(ram_addr_t addr)
>      }
>  }
>
> +void qemu_ram_set_extendable_on_migration(ram_addr_t addr)
> +{
> +    RAMBlock *block = find_ram_block(addr);
> +
> +    if (block) {
> +        block->flags |= RAM_EXTENDABLE_ON_MIGRATE;
> +    }
> +}
> +

Let's see how some oher qemu_ram_*() functions search for their blocks.
We can form two classes; the first class takes a ram_addr_t into some
RAMBlock, the second class only accepts/finds a ram_addr_t only at the
beginning of some RAMBlock.

(1) containment:

  qemu_get_ram_fd():           calls qemu_get_ram_block()
  qemu_get_ram_block_host_ptr: calls qemu_get_ram_block()
  qemu_get_ram_ptr():          calls qemu_get_ram_block()
  qemu_ram_remap():            needs containment

(2) exact block start match:

  qemu_ram_free():             needs (addr == block->offset)
  qemu_ram_free_from_ptr():    needs (addr == block->offset)
  qemu_ram_set_idstr():        calls find_ram_block()
  qemu_ram_unset_idstr():      calls find_ram_block()

Your function will belong to the 2nd class. The definition is close to
that of qemu_ram_unset_idstr(), another class member, OK. The
declaration is close to qemu_ram_free_from_ptr(), which is another class
member. OK.

I'd throw in an assert(), rather than an "if", just like
qemu_ram_set_idstr() does.

>  static int memory_try_enable_merging(void *addr, size_t len)
>  {
>      if (!qemu_opt_get_bool(qemu_get_machine_opts(), "mem-merge", true)) {

Let's see the caller:

> diff --git a/memory.c b/memory.c
> index 64d7176..744c746 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1791,6 +1791,11 @@ bool memory_region_is_mapped(MemoryRegion *mr)
>      return mr->container ? true : false;
>  }
>
> +void memory_region_permit_extendable_migration(MemoryRegion *mr)
> +{
> +    qemu_ram_set_extendable_on_migration(mr->ram_addr);
> +}
> +
>  MemoryRegionSection memory_region_find(MemoryRegion *mr,
>                                         hwaddr addr, uint64_t size)
>  {
>

Looks matching, and consistent with other callers of the 2nd family
functions. OK.

> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index e2c8e3e..6c03f70 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -894,6 +894,16 @@ bool memory_region_present(MemoryRegion *container, hwaddr addr);
>  bool memory_region_is_mapped(MemoryRegion *mr);
>
>  /**
> + * memory_region_permit_extendable_migration: allows to mark

Please refer to

commit 9d85d557326df69fe0570e7de84b2f57e133c7e7
Author: Michael Tokarev <mjt@tls.msk.ru>
Date:   Mon Apr 7 13:34:58 2014 +0400

    doc: grammify "allows to"

> + * #MemoryRegion as extendable on migrartion, which permits to

"migrartion": garbled spelling

"permits to": I guess it's similar to "allows to"

> + * load source memory block of smaller size than destination memory block
> + * at migration time
> + *
> + * @mr: a #MemoryRegion which should be marked as extendable on migration
> + */

("whose #RAMBlock should be marked as"...)

> +void memory_region_permit_extendable_migration(MemoryRegion *mr);
> +
> +/**
>   * memory_region_find: translate an address/size relative to a
>   * MemoryRegion into a #MemoryRegionSection.
>   *

Then, as I said,

> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index f91581f..2b9aea3 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -296,13 +296,26 @@ CPUArchState *cpu_copy(CPUArchState *env);
>  #if !defined(CONFIG_USER_ONLY)
>
>  /* memory API */
> +typedef enum {
> +    /* RAM is pre-allocated and passed into qemu_ram_alloc_from_ptr */
> +    RAM_PREALLOC = 1 << 0,
> +    /* RAM is mmap-ed with MAP_SHARED */
> +    RAM_SHARED = 1 << 1,
> +    /*
> +     * Allow at migration time to load RAMBlock of smaller size than
> +     * destination RAMBlock is
> +     */
> +    RAM_EXTENDABLE_ON_MIGRATE = 1 << 2,
> +    RAM_FLAGS_END = 1 << 31
> +} RAMBlockFlags;
> +

... I kind of disagree with the enumification of these flags.

There's another use of "allow [] to".

RAM_FLAGS_END is wrong. It shifts a signed 32-bit integer with value 1
to the left by 31 bits, shifting the set 0th bit into the sign bit.
Undefined behavior, Peter had a number of patches exterminating such
shifts (IIRC). You need to say 1U << 31.

However, that won't work either, because enumeration constants have type
"int" (and 1U << 31 is not representable as "int"). The C99 spec says
this (6.4.4.3p2), and then separately it also says "The expression that
defines the value of an enumeration constant shall be an integer
constant expression that has a value representable as an int."
(6.7.2.2p2).

So, I'd say just drop the enumification, and if you still want a
RAM_FLAGS_END macro, define it as (1U << 31).

>
>  typedef struct RAMBlock {
>      struct MemoryRegion *mr;
>      uint8_t *host;
>      ram_addr_t offset;
>      ram_addr_t length;
> -    uint32_t flags;
> +    RAMBlockFlags flags;
>      char idstr[256];
>      /* Reads can take either the iothread or the ramlist lock.
>       * Writes must take both locks.

uint32_t is just fine here. IMHO.

>
> diff --git a/arch_init.c b/arch_init.c
> index 8ddaf35..812f8b5 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -1071,11 +1071,20 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>
>                  QTAILQ_FOREACH(block, &ram_list.blocks, next) {
>                      if (!strncmp(id, block->idstr, sizeof(id))) {
> -                        if (block->length != length) {
> -                            error_report("Length mismatch: %s: " RAM_ADDR_FMT
> -                                         " in != " RAM_ADDR_FMT, id, length,
> -                                         block->length);
> -                            ret =  -EINVAL;
> +                        if (block->flags & RAM_EXTENDABLE_ON_MIGRATE) {
> +                            if (block->length < length) {
> +                                error_report("Length too big: %s: "RAM_ADDR_FMT

missing space between '"' and RAM_ADDR_FMT (for readability)


> +                                             " > " RAM_ADDR_FMT, id, length,

you dropped the important word "in" here (cf. "in !=" above). That word
explains it to the user that the incoming size (stored in "length") is
too big.

> +                                             block->length);
> +                                ret =  -EINVAL;
> +                            }
> +                        } else {
> +                            if (block->length != length) {
> +                                error_report("Length mismatch: %s: "RAM_ADDR_FMT

missing space between '"' and RAM_ADDR_FMT (for readability)

> +                                             " in != " RAM_ADDR_FMT, id, length,
> +                                             block->length);
> +                                ret =  -EINVAL;
> +                            }
>                          }
>                          break;
>                      }

Can you please explain where it is ensured that the non-overwritten part
of a greater RAMBlock is zero-filled? As far as I can see:

main() [vl.c]
  qemu_run_machine_init_done_notifiers() [vl.c]
    notifier_list_notify() [util/notify.c]
      pc_guest_info_machine_done() [hw/i386/pc.c]
        acpi_setup() [hw/i386/acpi-build.c]
          acpi_add_rom_blob() [hw/i386/acpi-build.c]
            rom_add_blob() [hw/core/loader.c]
              rom_set_mr() [hw/core/loader.c]
                memory_region_init_ram() [memory.c]
                  qemu_ram_alloc() [exec.c]
                memcpy()      <-------------------------- populates it
              fw_cfg_add_file_callback() [hw/nvram/fw_cfg.c]
                fw_cfg_add_bytes_read_callback() [hw/nvram/fw_cfg.c] <----- stores "len"
  qemu_start_incoming_migration() [migration.c]

I currently cannot see where the trailing portion of the bigger RAMBlock
would be overwritten with zeroes.

I also don't see why that portion of the RAMBlock / memory region would
not be exposed to the guest over fw_cfg. The size of the fw_cfg entry is
stored while the target (incoming) qemu process is setting up, in
fw_cfg_add_bytes_read_callback(), and that "len" value will reflect the
greater length when reading too. See fw_cfg_read().

If that portion contains nonzero garbage (leftovers from the original
ACPI stuff, generated on the incoming host), then OVMF's ACPI payload
parser will choke on it (if you migrate such a VM before OVMF does its
ACPI parsing). It can handle trailing zeros, but not trailing garbage.

Summary, roughly in order of growing importance:
- various typos and grammar errors, not too important to fix,
- please consider assert()ing the success of find_ram_block() in
  qemu_ram_set_extendable_on_migration(),
- please fix whitespace errors near RAM_ADDR_FMT,
- please keep the "in" word in the new error message there,
- please do something about RAM_FLAGS_END; preferably, keep the macros
  and drop the enum,
- please prove (or enforce) that the trailing portion of oversized
  RAMBlocks are filled with zeroes (a memset() might suffice near the
  new (block->length < length) check, in ram_load()).

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH for-2.1 1/2] migration: load smaller RAMBlock to a bigger one if permitted

[Igor Mammedov]

On Fri, 25 Jul 2014 19:56:40 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 07/25/14 17:48, Igor Mammedov wrote:
> 
> > Add API to mark memory region as extend-able on migration,
> > to allow migration code to load smaller RAMBlock into
> > a bigger one on destination QEMU instance.
> >
> > This will allow to fix broken migration from QEMU 1.7/2.0 to
> > QEMU 2.1 due to  ACPI tables size changes across 1.7/2.0/2.1
> > versions by marking ACPI tables ROM blob as extend-able.
> > So that smaller tables from previos version could be always
> 
> ("previous")
> 
> > migrated to a bigger rom blob on new version.
> >
> > Credits-for-idea: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  arch_init.c             | 19 ++++++++++++++-----
> >  exec.c                  | 15 +++++++++------
> >  include/exec/cpu-all.h  | 15 ++++++++++++++-
> >  include/exec/memory.h   | 10 ++++++++++
> >  include/exec/ram_addr.h |  1 +
> >  memory.c                |  5 +++++
> >  6 files changed, 53 insertions(+), 12 deletions(-)
> 
> (Please pass the -O flag to git-format-patch, with an order file that
> moves header files (*.h) to the front. Header hunks should lead in a
> patch. Thanks.)
> 
> > diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> > index 6593be1..248c075 100644
> > --- a/include/exec/ram_addr.h
> > +++ b/include/exec/ram_addr.h
> > @@ -33,6 +33,7 @@ void *qemu_get_ram_block_host_ptr(ram_addr_t addr);
> >  void *qemu_get_ram_ptr(ram_addr_t addr);
> >  void qemu_ram_free(ram_addr_t addr);
> >  void qemu_ram_free_from_ptr(ram_addr_t addr);
> > +void qemu_ram_set_extendable_on_migration(ram_addr_t addr);
> >
> >  static inline bool cpu_physical_memory_get_dirty(ram_addr_t start,
> >                                                   ram_addr_t length,
> 
> (Ugh. The declarations (prototypes) of qemu_ram_*() functions are
> scattered between "ram_addr.h" and "cpu-common.h" (both under
> include/exec). I can't see why that is a good thing (the function
> definitions all seem to be in exec.c).)
Trying to avoid putting stuff to global cpu-common.h, I've put
definition in ram_addr.h. The rest should be moved from cpu-common.h
to ram_addr.h when 2.2 is open.

[...]

> >
> > diff --git a/arch_init.c b/arch_init.c
> > index 8ddaf35..812f8b5 100644
> > --- a/arch_init.c
> > +++ b/arch_init.c
> > @@ -1071,11 +1071,20 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
> >
> >                  QTAILQ_FOREACH(block, &ram_list.blocks, next) {
> >                      if (!strncmp(id, block->idstr, sizeof(id))) {
> > -                        if (block->length != length) {
> > -                            error_report("Length mismatch: %s: " RAM_ADDR_FMT
> > -                                         " in != " RAM_ADDR_FMT, id, length,
> > -                                         block->length);
> > -                            ret =  -EINVAL;
> > +                        if (block->flags & RAM_EXTENDABLE_ON_MIGRATE) {
> > +                            if (block->length < length) {
> > +                                error_report("Length too big: %s: "RAM_ADDR_FMT
> 
> missing space between '"' and RAM_ADDR_FMT (for readability)
> 
> 
> > +                                             " > " RAM_ADDR_FMT, id, length,
> 
> you dropped the important word "in" here (cf. "in !=" above). That word
> explains it to the user that the incoming size (stored in "length") is
> too big.
> 
> > +                                             block->length);
> > +                                ret =  -EINVAL;
> > +                            }
> > +                        } else {
> > +                            if (block->length != length) {
> > +                                error_report("Length mismatch: %s: "RAM_ADDR_FMT
> 
> missing space between '"' and RAM_ADDR_FMT (for readability)
> 
> > +                                             " in != " RAM_ADDR_FMT, id, length,
> > +                                             block->length);
> > +                                ret =  -EINVAL;
> > +                            }
> >                          }
> >                          break;
> >                      }
> 
> Can you please explain where it is ensured that the non-overwritten part
> of a greater RAMBlock is zero-filled? As far as I can see:
> 
> main() [vl.c]
>   qemu_run_machine_init_done_notifiers() [vl.c]
>     notifier_list_notify() [util/notify.c]
>       pc_guest_info_machine_done() [hw/i386/pc.c]
>         acpi_setup() [hw/i386/acpi-build.c]
>           acpi_add_rom_blob() [hw/i386/acpi-build.c]
>             rom_add_blob() [hw/core/loader.c]
>               rom_set_mr() [hw/core/loader.c]
>                 memory_region_init_ram() [memory.c]
>                   qemu_ram_alloc() [exec.c]
>                 memcpy()      <-------------------------- populates it
>               fw_cfg_add_file_callback() [hw/nvram/fw_cfg.c]
>                 fw_cfg_add_bytes_read_callback() [hw/nvram/fw_cfg.c] <----- stores "len"
>   qemu_start_incoming_migration() [migration.c]
> 
> I currently cannot see where the trailing portion of the bigger RAMBlock
> would be overwritten with zeroes.
> 
> I also don't see why that portion of the RAMBlock / memory region would
> not be exposed to the guest over fw_cfg. The size of the fw_cfg entry is
> stored while the target (incoming) qemu process is setting up, in
> fw_cfg_add_bytes_read_callback(), and that "len" value will reflect the
> greater length when reading too. See fw_cfg_read().
>
> If that portion contains nonzero garbage (leftovers from the original
> ACPI stuff, generated on the incoming host), then OVMF's ACPI payload
> parser will choke on it (if you migrate such a VM before OVMF does its
> ACPI parsing). It can handle trailing zeros, but not trailing garbage.
I've 'fixed' it as suggested by zeroing it out.
But if OVMF chokes on garbage after ACPI tables, then it probably does
something wrong. I'd say we shouldn't care about garbage after ACPI tables,
guest still should be able locate RSDP and then read the rest of the tables
referenced by RSDT (tested RHEL6 and rather capricious Windows2003 all of them
boot and read correct tables).


[...] 
> Thanks,
> Laszlo

Re: [Qemu-devel] [PATCH for-2.1 1/2] migration: load smaller RAMBlock to a bigger one if permitted

[Laszlo Ersek]

On 07/28/14 09:40, Igor Mammedov wrote:
> On Fri, 25 Jul 2014 19:56:40 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 07/25/14 17:48, Igor Mammedov wrote:
>>
>>> Add API to mark memory region as extend-able on migration,
>>> to allow migration code to load smaller RAMBlock into
>>> a bigger one on destination QEMU instance.
>>>
>>> This will allow to fix broken migration from QEMU 1.7/2.0 to
>>> QEMU 2.1 due to  ACPI tables size changes across 1.7/2.0/2.1
>>> versions by marking ACPI tables ROM blob as extend-able.
>>> So that smaller tables from previos version could be always
>>
>> ("previous")
>>
>>> migrated to a bigger rom blob on new version.
>>>
>>> Credits-for-idea: Michael S. Tsirkin <mst@redhat.com>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>>  arch_init.c             | 19 ++++++++++++++-----
>>>  exec.c                  | 15 +++++++++------
>>>  include/exec/cpu-all.h  | 15 ++++++++++++++-
>>>  include/exec/memory.h   | 10 ++++++++++
>>>  include/exec/ram_addr.h |  1 +
>>>  memory.c                |  5 +++++
>>>  6 files changed, 53 insertions(+), 12 deletions(-)
>>
>> (Please pass the -O flag to git-format-patch, with an order file that
>> moves header files (*.h) to the front. Header hunks should lead in a
>> patch. Thanks.)
>>
>>> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
>>> index 6593be1..248c075 100644
>>> --- a/include/exec/ram_addr.h
>>> +++ b/include/exec/ram_addr.h
>>> @@ -33,6 +33,7 @@ void *qemu_get_ram_block_host_ptr(ram_addr_t addr);
>>>  void *qemu_get_ram_ptr(ram_addr_t addr);
>>>  void qemu_ram_free(ram_addr_t addr);
>>>  void qemu_ram_free_from_ptr(ram_addr_t addr);
>>> +void qemu_ram_set_extendable_on_migration(ram_addr_t addr);
>>>
>>>  static inline bool cpu_physical_memory_get_dirty(ram_addr_t start,
>>>                                                   ram_addr_t length,
>>
>> (Ugh. The declarations (prototypes) of qemu_ram_*() functions are
>> scattered between "ram_addr.h" and "cpu-common.h" (both under
>> include/exec). I can't see why that is a good thing (the function
>> definitions all seem to be in exec.c).)
> Trying to avoid putting stuff to global cpu-common.h, I've put
> definition in ram_addr.h. The rest should be moved from cpu-common.h
> to ram_addr.h when 2.2 is open.
> 
> [...]
> 
>>>
>>> diff --git a/arch_init.c b/arch_init.c
>>> index 8ddaf35..812f8b5 100644
>>> --- a/arch_init.c
>>> +++ b/arch_init.c
>>> @@ -1071,11 +1071,20 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>>>
>>>                  QTAILQ_FOREACH(block, &ram_list.blocks, next) {
>>>                      if (!strncmp(id, block->idstr, sizeof(id))) {
>>> -                        if (block->length != length) {
>>> -                            error_report("Length mismatch: %s: " RAM_ADDR_FMT
>>> -                                         " in != " RAM_ADDR_FMT, id, length,
>>> -                                         block->length);
>>> -                            ret =  -EINVAL;
>>> +                        if (block->flags & RAM_EXTENDABLE_ON_MIGRATE) {
>>> +                            if (block->length < length) {
>>> +                                error_report("Length too big: %s: "RAM_ADDR_FMT
>>
>> missing space between '"' and RAM_ADDR_FMT (for readability)
>>
>>
>>> +                                             " > " RAM_ADDR_FMT, id, length,
>>
>> you dropped the important word "in" here (cf. "in !=" above). That word
>> explains it to the user that the incoming size (stored in "length") is
>> too big.
>>
>>> +                                             block->length);
>>> +                                ret =  -EINVAL;
>>> +                            }
>>> +                        } else {
>>> +                            if (block->length != length) {
>>> +                                error_report("Length mismatch: %s: "RAM_ADDR_FMT
>>
>> missing space between '"' and RAM_ADDR_FMT (for readability)
>>
>>> +                                             " in != " RAM_ADDR_FMT, id, length,
>>> +                                             block->length);
>>> +                                ret =  -EINVAL;
>>> +                            }
>>>                          }
>>>                          break;
>>>                      }
>>
>> Can you please explain where it is ensured that the non-overwritten part
>> of a greater RAMBlock is zero-filled? As far as I can see:
>>
>> main() [vl.c]
>>   qemu_run_machine_init_done_notifiers() [vl.c]
>>     notifier_list_notify() [util/notify.c]
>>       pc_guest_info_machine_done() [hw/i386/pc.c]
>>         acpi_setup() [hw/i386/acpi-build.c]
>>           acpi_add_rom_blob() [hw/i386/acpi-build.c]
>>             rom_add_blob() [hw/core/loader.c]
>>               rom_set_mr() [hw/core/loader.c]
>>                 memory_region_init_ram() [memory.c]
>>                   qemu_ram_alloc() [exec.c]
>>                 memcpy()      <-------------------------- populates it
>>               fw_cfg_add_file_callback() [hw/nvram/fw_cfg.c]
>>                 fw_cfg_add_bytes_read_callback() [hw/nvram/fw_cfg.c] <----- stores "len"
>>   qemu_start_incoming_migration() [migration.c]
>>
>> I currently cannot see where the trailing portion of the bigger RAMBlock
>> would be overwritten with zeroes.
>>
>> I also don't see why that portion of the RAMBlock / memory region would
>> not be exposed to the guest over fw_cfg. The size of the fw_cfg entry is
>> stored while the target (incoming) qemu process is setting up, in
>> fw_cfg_add_bytes_read_callback(), and that "len" value will reflect the
>> greater length when reading too. See fw_cfg_read().
>>
>> If that portion contains nonzero garbage (leftovers from the original
>> ACPI stuff, generated on the incoming host), then OVMF's ACPI payload
>> parser will choke on it (if you migrate such a VM before OVMF does its
>> ACPI parsing). It can handle trailing zeros, but not trailing garbage.

> I've 'fixed' it as suggested by zeroing it out.

Thanks!

> But if OVMF chokes on garbage after ACPI tables, then it probably does
> something wrong. I'd say we shouldn't care about garbage after ACPI tables,
> guest still should be able locate RSDP and then read the rest of the tables
> referenced by RSDT (tested RHEL6 and rather capricious Windows2003 all of them
> boot and read correct tables).

OVMF works differently from SeaBIOS. OVMF itself has to locate the
individual tables in the fw_cfg blob(s). If you leave garbage at the end
of some blob(s), OVMF's (simple) ACPI table header parser will reject
the payload as invalid, and drop everything else too (on purpose).

This has been all discussed earlier (on edk2-devel), including the
"why". Here's the link:

http://sourceforge.net/p/edk2/mailman/edk2-devel/thread/1400626211-28593-1-git-send-email-lersek@redhat.com/

Thanks
Laszlo

[Qemu-devel] [PATCH for-2.1 2/2] acpi: mark ACPI tables ROM blob as extend-able on migration

[Igor Mammedov]

It fixes migration failure for machine type pc-i440fx-1.7 from
QEMU 1.7/2.0 to QEMU 2.1

Migration fails due to ACPI tables size grows across 1.7-2.1
versions. That causes ACPI tables ROM blob to change its size
differently for the same configurations on different QEMU versions.
As result migration code bails out when attempting to load
smaller ROM blob into a bigger one on a newer QEMU.
To trigger failure it's enough to add pci-bridge device to QEMU CLI

Marking ACPI tables ROM blob as extend-able on migration allows
QEMU to load smaller ACPI tables from QEMU 1.7/2.0, fixing
forward migration failure introduced since 2.0 which affects
only configurations that cause ACPI ROM blob size change.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/core/loader.c     | 6 +++++-
 hw/i386/acpi-build.c | 2 +-
 include/hw/loader.h  | 5 +++--
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 2bf6b8f..d09b894 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -722,7 +722,8 @@ err:
 
 void *rom_add_blob(const char *name, const void *blob, size_t len,
                    hwaddr addr, const char *fw_file_name,
-                   FWCfgReadCallback fw_callback, void *callback_opaque)
+                   FWCfgReadCallback fw_callback, void *callback_opaque,
+                   bool extendable)
 {
     Rom *rom;
     void *data = NULL;
@@ -742,6 +743,9 @@ void *rom_add_blob(const char *name, const void *blob, size_t len,
 
         if (rom_file_has_mr) {
             data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
+            if (extendable) {
+                memory_region_permit_extendable_migration(rom->mr);
+            }
         } else {
             data = rom->data;
         }
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index ebc5f03..651c06b 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1558,7 +1558,7 @@ static void *acpi_add_rom_blob(AcpiBuildState *build_state, GArray *blob,
                                const char *name)
 {
     return rom_add_blob(name, blob->data, acpi_data_len(blob), -1, name,
-                        acpi_build_update, build_state);
+                        acpi_build_update, build_state, true);
 }
 
 static const VMStateDescription vmstate_acpi_build = {
diff --git a/include/hw/loader.h b/include/hw/loader.h
index 796cbf9..e5a24ac 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -57,7 +57,8 @@ int rom_add_file(const char *file, const char *fw_dir,
                  bool option_rom);
 void *rom_add_blob(const char *name, const void *blob, size_t len,
                    hwaddr addr, const char *fw_file_name,
-                   FWCfgReadCallback fw_callback, void *callback_opaque);
+                   FWCfgReadCallback fw_callback, void *callback_opaque,
+                   bool extendable);
 int rom_add_elf_program(const char *name, void *data, size_t datasize,
                         size_t romsize, hwaddr addr);
 int rom_load_all(void);
@@ -70,7 +71,7 @@ void do_info_roms(Monitor *mon, const QDict *qdict);
 #define rom_add_file_fixed(_f, _a, _i)          \
     rom_add_file(_f, NULL, _a, _i, false)
 #define rom_add_blob_fixed(_f, _b, _l, _a)      \
-    rom_add_blob(_f, _b, _l, _a, NULL, NULL, NULL)
+    rom_add_blob(_f, _b, _l, _a, NULL, NULL, NULL, false)
 
 #define PC_ROM_MIN_VGA     0xc0000
 #define PC_ROM_MIN_OPTION  0xc8000
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH for-2.1 2/2] acpi: mark ACPI tables ROM blob as extend-able on migration

[Paolo Bonzini]

Il 25/07/2014 17:48, Igor Mammedov ha scritto:
> It fixes migration failure for machine type pc-i440fx-1.7 from
> QEMU 1.7/2.0 to QEMU 2.1
> 
> Migration fails due to ACPI tables size grows across 1.7-2.1
> versions. That causes ACPI tables ROM blob to change its size
> differently for the same configurations on different QEMU versions.
> As result migration code bails out when attempting to load
> smaller ROM blob into a bigger one on a newer QEMU.
> To trigger failure it's enough to add pci-bridge device to QEMU CLI
> 
> Marking ACPI tables ROM blob as extend-able on migration allows
> QEMU to load smaller ACPI tables from QEMU 1.7/2.0, fixing
> forward migration failure introduced since 2.0 which affects
> only configurations that cause ACPI ROM blob size change.

This works in this case, and it is more friendly to downstreams indeed.
 It also is simpler, at least on the surface.  I think the ramifications
could be a bit larger than with my own patches, but still I guess it's
more appropriate at this point of the release cycle.

It doesn't handle the case of ACPI tables that shrink, which can happen
as well.  I guess if this ever happens we can just hard-code the table
size of the "old" versions to something big enough (64K?) and keep using
fine-grained sizing.

I'd like a day or two to mull about it, but I have it even if the
patches are applied.  Peter, feel free to go ahead with Igor's patches.

Paolo

Re: [Qemu-devel] [PATCH for-2.1 2/2] acpi: mark ACPI tables ROM blob as extend-able on migration

[Igor Mammedov]

On Fri, 25 Jul 2014 19:37:41 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 25/07/2014 17:48, Igor Mammedov ha scritto:
> > It fixes migration failure for machine type pc-i440fx-1.7 from
> > QEMU 1.7/2.0 to QEMU 2.1
> > 
> > Migration fails due to ACPI tables size grows across 1.7-2.1
> > versions. That causes ACPI tables ROM blob to change its size
> > differently for the same configurations on different QEMU versions.
> > As result migration code bails out when attempting to load
> > smaller ROM blob into a bigger one on a newer QEMU.
> > To trigger failure it's enough to add pci-bridge device to QEMU CLI
> > 
> > Marking ACPI tables ROM blob as extend-able on migration allows
> > QEMU to load smaller ACPI tables from QEMU 1.7/2.0, fixing
> > forward migration failure introduced since 2.0 which affects
> > only configurations that cause ACPI ROM blob size change.
> 
> This works in this case, and it is more friendly to downstreams indeed.
>  It also is simpler, at least on the surface.  I think the ramifications
> could be a bit larger than with my own patches, but still I guess it's
> more appropriate at this point of the release cycle.
> 
> It doesn't handle the case of ACPI tables that shrink, which can happen
> as well.  I guess if this ever happens we can just hard-code the table
> size of the "old" versions to something big enough (64K?) and keep using
> fine-grained sizing.
That was intentionally omitted in here so far size only goes up from 1.7 to
2.1. My though was that can enforce minimum size later during 2.2 cycle
Anyway, I'll think more about it, and maybe post additional patch
on top of this to set minimum size if I find a reason for it to be in 2.1.


> 
> I'd like a day or two to mull about it, but I have it even if the
> patches are applied.  Peter, feel free to go ahead with Igor's patches.
> 
> Paolo
> 
> 

Re: [Qemu-devel] [PATCH for-2.1 2/2] acpi: mark ACPI tables ROM blob as extend-able on migration

[Paolo Bonzini]

Il 28/07/2014 09:56, Igor Mammedov ha scritto:
>> > It doesn't handle the case of ACPI tables that shrink, which can happen
>> > as well.  I guess if this ever happens we can just hard-code the table
>> > size of the "old" versions to something big enough (64K?) and keep using
>> > fine-grained sizing.
> That was intentionally omitted in here so far size only goes up from 1.7 to
> 2.1. My though was that can enforce minimum size later during 2.2 cycle
> Anyway, I'll think more about it, and maybe post additional patch
> on top of this to set minimum size if I find a reason for it to be in 2.1.

No, there's no need for it in 2.1.

Paolo

Re: [Qemu-devel] [PATCH for-2.1 2/2] acpi: mark ACPI tables ROM blob as extend-able on migration

[Laszlo Ersek]

On 07/25/14 17:48, Igor Mammedov wrote:
> It fixes migration failure for machine type pc-i440fx-1.7 from
> QEMU 1.7/2.0 to QEMU 2.1
> 
> Migration fails due to ACPI tables size grows across 1.7-2.1
> versions. That causes ACPI tables ROM blob to change its size
> differently for the same configurations on different QEMU versions.
> As result migration code bails out when attempting to load
> smaller ROM blob into a bigger one on a newer QEMU.
> To trigger failure it's enough to add pci-bridge device to QEMU CLI
> 
> Marking ACPI tables ROM blob as extend-able on migration allows
> QEMU to load smaller ACPI tables from QEMU 1.7/2.0, fixing
> forward migration failure introduced since 2.0 which affects
> only configurations that cause ACPI ROM blob size change.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/core/loader.c     | 6 +++++-
>  hw/i386/acpi-build.c | 2 +-
>  include/hw/loader.h  | 5 +++--
>  3 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 2bf6b8f..d09b894 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -722,7 +722,8 @@ err:
>  
>  void *rom_add_blob(const char *name, const void *blob, size_t len,
>                     hwaddr addr, const char *fw_file_name,
> -                   FWCfgReadCallback fw_callback, void *callback_opaque)
> +                   FWCfgReadCallback fw_callback, void *callback_opaque,
> +                   bool extendable)
>  {
>      Rom *rom;
>      void *data = NULL;
> @@ -742,6 +743,9 @@ void *rom_add_blob(const char *name, const void *blob, size_t len,
>  
>          if (rom_file_has_mr) {
>              data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
> +            if (extendable) {
> +                memory_region_permit_extendable_migration(rom->mr);
> +            }
>          } else {
>              data = rom->data;
>          }
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index ebc5f03..651c06b 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1558,7 +1558,7 @@ static void *acpi_add_rom_blob(AcpiBuildState *build_state, GArray *blob,
>                                 const char *name)
>  {
>      return rom_add_blob(name, blob->data, acpi_data_len(blob), -1, name,
> -                        acpi_build_update, build_state);
> +                        acpi_build_update, build_state, true);
>  }
>  
>  static const VMStateDescription vmstate_acpi_build = {
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 796cbf9..e5a24ac 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -57,7 +57,8 @@ int rom_add_file(const char *file, const char *fw_dir,
>                   bool option_rom);
>  void *rom_add_blob(const char *name, const void *blob, size_t len,
>                     hwaddr addr, const char *fw_file_name,
> -                   FWCfgReadCallback fw_callback, void *callback_opaque);
> +                   FWCfgReadCallback fw_callback, void *callback_opaque,
> +                   bool extendable);
>  int rom_add_elf_program(const char *name, void *data, size_t datasize,
>                          size_t romsize, hwaddr addr);
>  int rom_load_all(void);
> @@ -70,7 +71,7 @@ void do_info_roms(Monitor *mon, const QDict *qdict);
>  #define rom_add_file_fixed(_f, _a, _i)          \
>      rom_add_file(_f, NULL, _a, _i, false)
>  #define rom_add_blob_fixed(_f, _b, _l, _a)      \
> -    rom_add_blob(_f, _b, _l, _a, NULL, NULL, NULL)
> +    rom_add_blob(_f, _b, _l, _a, NULL, NULL, NULL, false)
>  
>  #define PC_ROM_MIN_VGA     0xc0000
>  #define PC_ROM_MIN_OPTION  0xc8000
> 

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

Re: [Qemu-devel] [PATCH for-2.1 2/2] acpi: mark ACPI tables ROM blob as extend-able on migration

[Dr. David Alan Gilbert]

* Igor Mammedov (imammedo@redhat.com) wrote:
> It fixes migration failure for machine type pc-i440fx-1.7 from
> QEMU 1.7/2.0 to QEMU 2.1
> 
> Migration fails due to ACPI tables size grows across 1.7-2.1
> versions. That causes ACPI tables ROM blob to change its size
> differently for the same configurations on different QEMU versions.
> As result migration code bails out when attempting to load
> smaller ROM blob into a bigger one on a newer QEMU.
> To trigger failure it's enough to add pci-bridge device to QEMU CLI
>
> Marking ACPI tables ROM blob as extend-able on migration allows
> QEMU to load smaller ACPI tables from QEMU 1.7/2.0, fixing
> forward migration failure introduced since 2.0 which affects
> only configurations that cause ACPI ROM blob size change.

(To follow up from an irc thread earlier today)
I only vaguely understand this case but my understanding is:

  1) It's a block of data that's never mapped into the guests address
space
  2) It can change, but only at guest reset
  3) Worst case is it can get upto about 2MB in size

it's pretty marginal whether this thing should be a RAMBlock,
it doesn't feel like normal RAM or ROM in most ways; but there
again 2MB is getting a bit large for the device state; hmm.

Dave

> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/core/loader.c     | 6 +++++-
>  hw/i386/acpi-build.c | 2 +-
>  include/hw/loader.h  | 5 +++--
>  3 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 2bf6b8f..d09b894 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -722,7 +722,8 @@ err:
>  
>  void *rom_add_blob(const char *name, const void *blob, size_t len,
>                     hwaddr addr, const char *fw_file_name,
> -                   FWCfgReadCallback fw_callback, void *callback_opaque)
> +                   FWCfgReadCallback fw_callback, void *callback_opaque,
> +                   bool extendable)
>  {
>      Rom *rom;
>      void *data = NULL;
> @@ -742,6 +743,9 @@ void *rom_add_blob(const char *name, const void *blob, size_t len,
>  
>          if (rom_file_has_mr) {
>              data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
> +            if (extendable) {
> +                memory_region_permit_extendable_migration(rom->mr);
> +            }
>          } else {
>              data = rom->data;
>          }
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index ebc5f03..651c06b 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1558,7 +1558,7 @@ static void *acpi_add_rom_blob(AcpiBuildState *build_state, GArray *blob,
>                                 const char *name)
>  {
>      return rom_add_blob(name, blob->data, acpi_data_len(blob), -1, name,
> -                        acpi_build_update, build_state);
> +                        acpi_build_update, build_state, true);
>  }
>  
>  static const VMStateDescription vmstate_acpi_build = {
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 796cbf9..e5a24ac 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -57,7 +57,8 @@ int rom_add_file(const char *file, const char *fw_dir,
>                   bool option_rom);
>  void *rom_add_blob(const char *name, const void *blob, size_t len,
>                     hwaddr addr, const char *fw_file_name,
> -                   FWCfgReadCallback fw_callback, void *callback_opaque);
> +                   FWCfgReadCallback fw_callback, void *callback_opaque,
> +                   bool extendable);
>  int rom_add_elf_program(const char *name, void *data, size_t datasize,
>                          size_t romsize, hwaddr addr);
>  int rom_load_all(void);
> @@ -70,7 +71,7 @@ void do_info_roms(Monitor *mon, const QDict *qdict);
>  #define rom_add_file_fixed(_f, _a, _i)          \
>      rom_add_file(_f, NULL, _a, _i, false)
>  #define rom_add_blob_fixed(_f, _b, _l, _a)      \
> -    rom_add_blob(_f, _b, _l, _a, NULL, NULL, NULL)
> +    rom_add_blob(_f, _b, _l, _a, NULL, NULL, NULL, false)
>  
>  #define PC_ROM_MIN_VGA     0xc0000
>  #define PC_ROM_MIN_OPTION  0xc8000
> -- 
> 1.8.3.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH for-2.1 2/2] acpi: mark ACPI tables ROM blob as extend-able on migration

[Paolo Bonzini]

On 03/11/2014 18:36, Dr. David Alan Gilbert wrote:
>   1) It's a block of data that's never mapped into the guests address
> space
>   2) It can change, but only at guest reset
>   3) Worst case is it can get upto about 2MB in size
> 
> it's pretty marginal whether this thing should be a RAMBlock,
> it doesn't feel like normal RAM or ROM in most ways; but there
> again 2MB is getting a bit large for the device state; hmm.

And also I think changing migration format gratuitously is bad.  We
decided to make these RAMs, which has some advantages and turned out to
have some possible disadvantages, but it's not a big deal.  They are
some kind of EPROM if you wish.

The important point is that we can (and arguably _should_ since it keeps
us honest!) make these ACPI tables RAMBlocks fixed-size per machine
type.  See the patches I posted around late September/early October.
There is no need to support auto-fixing of the RAMBlock's sizes.

Paolo

Re: [Qemu-devel] [PATCH for-2.1 2/2] acpi: mark ACPI tables ROM blob as extend-able on migration

[Michael S. Tsirkin]

On Tue, Nov 04, 2014 at 05:30:14PM +0100, Paolo Bonzini wrote:
> On 03/11/2014 18:36, Dr. David Alan Gilbert wrote:
> >   1) It's a block of data that's never mapped into the guests address
> > space
> >   2) It can change, but only at guest reset
> >   3) Worst case is it can get upto about 2MB in size
> > 
> > it's pretty marginal whether this thing should be a RAMBlock,
> > it doesn't feel like normal RAM or ROM in most ways; but there
> > again 2MB is getting a bit large for the device state; hmm.
> 
> And also I think changing migration format gratuitously is bad.  We
> decided to make these RAMs, which has some advantages and turned out to
> have some possible disadvantages, but it's not a big deal.  They are
> some kind of EPROM if you wish.
> 
> The important point is that we can (and arguably _should_ since it keeps
> us honest!) make these ACPI tables RAMBlocks fixed-size per machine
> type.  See the patches I posted around late September/early October.
> There is no need to support auto-fixing of the RAMBlock's sizes.
> 
> Paolo

I'm not sure I buy that we should. ACPI bytecode can express
identical interfaces in different ways. Even just recompiling
ACPI from source can give you a different binary,
same is true for a minor change in ACPI code.
Migrating between two almost identical builds from qemu seems a
very reasonable thing to do.


-- 
MST

Re: [Qemu-devel] [PATCH for-2.1 2/2] acpi: mark ACPI tables ROM blob as extend-able on migration

[Paolo Bonzini]

On 04/11/2014 17:46, Michael S. Tsirkin wrote:
> On Tue, Nov 04, 2014 at 05:30:14PM +0100, Paolo Bonzini wrote:
>> On 03/11/2014 18:36, Dr. David Alan Gilbert wrote:
>>>   1) It's a block of data that's never mapped into the guests address
>>> space
>>>   2) It can change, but only at guest reset
>>>   3) Worst case is it can get upto about 2MB in size
>>>
>>> it's pretty marginal whether this thing should be a RAMBlock,
>>> it doesn't feel like normal RAM or ROM in most ways; but there
>>> again 2MB is getting a bit large for the device state; hmm.
>>
>> And also I think changing migration format gratuitously is bad.  We
>> decided to make these RAMs, which has some advantages and turned out to
>> have some possible disadvantages, but it's not a big deal.  They are
>> some kind of EPROM if you wish.
>>
>> The important point is that we can (and arguably _should_ since it keeps
>> us honest!) make these ACPI tables RAMBlocks fixed-size per machine
>> type.  See the patches I posted around late September/early October.
>> There is no need to support auto-fixing of the RAMBlock's sizes.
> 
> I'm not sure I buy that we should. ACPI bytecode can express
> identical interfaces in different ways. Even just recompiling
> ACPI from source can give you a different binary,
> same is true for a minor change in ACPI code.
> Migrating between two almost identical builds from qemu seems a
> very reasonable thing to do.

Yes, identical ACPI blocks are just a sufficient condition, not a
necessary one.  But it's very easy to enforce it, and it's what the
acpi-tables-test already checks.  It makes sense to me to stick with it.

(Regarding recompilation with a different iasl version, SSDT blocks are
simple enough that I think we can just build them in C code.  We're
already doing it for the much more complicated PCI bridge hotplug
interface.  BTW, can you pick up at least the patch to move the memory
hotplug device from SSDT to DSDT?).

Paolo

Re: [Qemu-devel] [PATCH for-2.1 2/2] acpi: mark ACPI tables ROM blob as extend-able on migration

[Michael S. Tsirkin]

On Tue, Nov 04, 2014 at 05:54:26PM +0100, Paolo Bonzini wrote:
> On 04/11/2014 17:46, Michael S. Tsirkin wrote:
> > On Tue, Nov 04, 2014 at 05:30:14PM +0100, Paolo Bonzini wrote:
> >> On 03/11/2014 18:36, Dr. David Alan Gilbert wrote:
> >>>   1) It's a block of data that's never mapped into the guests address
> >>> space
> >>>   2) It can change, but only at guest reset
> >>>   3) Worst case is it can get upto about 2MB in size
> >>>
> >>> it's pretty marginal whether this thing should be a RAMBlock,
> >>> it doesn't feel like normal RAM or ROM in most ways; but there
> >>> again 2MB is getting a bit large for the device state; hmm.
> >>
> >> And also I think changing migration format gratuitously is bad.  We
> >> decided to make these RAMs, which has some advantages and turned out to
> >> have some possible disadvantages, but it's not a big deal.  They are
> >> some kind of EPROM if you wish.
> >>
> >> The important point is that we can (and arguably _should_ since it keeps
> >> us honest!) make these ACPI tables RAMBlocks fixed-size per machine
> >> type.  See the patches I posted around late September/early October.
> >> There is no need to support auto-fixing of the RAMBlock's sizes.
> > 
> > I'm not sure I buy that we should. ACPI bytecode can express
> > identical interfaces in different ways. Even just recompiling
> > ACPI from source can give you a different binary,
> > same is true for a minor change in ACPI code.
> > Migrating between two almost identical builds from qemu seems a
> > very reasonable thing to do.
> 
> Yes, identical ACPI blocks are just a sufficient condition, not a
> necessary one.  But it's very easy to enforce it, and it's what the
> acpi-tables-test already checks.  It makes sense to me to stick with it.
> 
> (Regarding recompilation with a different iasl version, SSDT blocks are
> simple enough that I think we can just build them in C code.  We're
> already doing it for the much more complicated PCI bridge hotplug
> interface.  BTW, can you pick up at least the patch to move the memory
> hotplug device from SSDT to DSDT?).
> 
> Paolo

I'm not against that - does this have value by itself?

-- 
MST

Re: [Qemu-devel] [PATCH for-2.1 2/2] acpi: mark ACPI tables ROM blob as extend-able on migration

[Paolo Bonzini]

On 04/11/2014 18:25, Michael S. Tsirkin wrote:
>> (Regarding recompilation with a different iasl version, SSDT blocks are
>> simple enough that I think we can just build them in C code.  We're
>> already doing it for the much more complicated PCI bridge hotplug
>> interface.  BTW, can you pick up at least the patch to move the memory
>> hotplug device from SSDT to DSDT?).
>>
>> Paolo
> 
> I'm not against that - does this have value by itself?

It matches more closely what we do for processors, and is more faithful
to the idea that the SSDTs express supplementary functionality.

Paolo

Re: [Qemu-devel] [PATCH for-2.1 2/2] acpi: mark ACPI tables ROM blob as extend-able on migration

[Michael S. Tsirkin]

On Tue, Nov 04, 2014 at 06:35:50PM +0100, Paolo Bonzini wrote:
> 
> 
> On 04/11/2014 18:25, Michael S. Tsirkin wrote:
> >> (Regarding recompilation with a different iasl version, SSDT blocks are
> >> simple enough that I think we can just build them in C code.  We're
> >> already doing it for the much more complicated PCI bridge hotplug
> >> interface.  BTW, can you pick up at least the patch to move the memory
> >> hotplug device from SSDT to DSDT?).
> >>
> >> Paolo
> > 
> > I'm not against that - does this have value by itself?
> 
> It matches more closely what we do for processors, and is more faithful
> to the idea that the SSDTs express supplementary functionality.
> 
> Paolo

OK - I'll review that separately.
Thanks!

-- 
MST

Re: [Qemu-devel] [PATCH for-2.1 0/2] Fix migration failure due to ACPI tables size changes

[Laszlo Ersek]

On 07/25/14 17:48, Igor Mammedov wrote:
> Changing the ACPI table size causes migration to break, and the memory
> hotplug work opened our eyes on how horribly we were breaking things in
> 2.0 already.
> 
> To trigger issue start
>   QEMU-1.7 with -M pc-i440fx-1.7 -device pci-bridge,chassis_nr=1
> and try to migrate to QEMU-2.1 or QEMU-2.0 as result target will fail with:
>   qemu-system-x86_64: Length mismatch: /rom@etc/acpi/tables: 2000 in != 3000
> 
> This fix allows target QEMU to load smaller RAMBlock into a bigger one
> and fixes regression which was introduced since 2.0, allowing
> forward migration from 1.7/2.0 to 2.1
> Fix is also suitable for stable-2.0.
> 
> Igor Mammedov (2):
>   migration: load smaller RAMBlock to a bigger one if permitted
>   acpi: mark ACPI tables ROM blob as extend-able on migration
> 
>  arch_init.c             | 19 ++++++++++++++-----
>  exec.c                  | 15 +++++++++------
>  hw/core/loader.c        |  6 +++++-
>  hw/i386/acpi-build.c    |  2 +-
>  include/exec/cpu-all.h  | 15 ++++++++++++++-
>  include/exec/memory.h   | 10 ++++++++++
>  include/exec/ram_addr.h |  1 +
>  include/hw/loader.h     |  5 +++--
>  memory.c                |  5 +++++
>  9 files changed, 62 insertions(+), 16 deletions(-)

I can name things in favor of both approaches, Paolo's and yours.

Paolo's approach keeps the hack where the problem was introduced, in the
ACPI generator. The local nature of the migration-related pig sty is
very attractive for me. As I said on IRC, if you fix the migration
problem in the APCI generator by elegant compat flags, or such an
emergency hack as Paolo's, that's an implementation detail; it's
important that it stay local to the ACPI generator. Your patchset leaks
the problem into RAMBlocks.

In favor of your idea OTOH, as you explained, it would work for all
possible ACPI configurations, and not break for a random few ones like
Paolo's approach (talking about patch #2 of that set, the procedural
_PRT is great in any case). You could argue that by customizing the
RAMBlocks as extensible (for ACPI / fw_cfg blob backing only) the
leakage is contained.

I don't know how to decide between these two approaches. The optimal one
would be to reimplement the 2.0 --> 2.1 feature additions from scratch,
introducing compat flags afresh. Won't happen I guess.

In any case I'll try to review this set for the technical things.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH for-2.1 0/2] Fix migration failure due to ACPI tables size changes

[Paolo Bonzini]

Il 25/07/2014 18:17, Laszlo Ersek ha scritto:
> In favor of your idea OTOH, as you explained, it would work for all
> possible ACPI configurations, and not break for a random few ones like
> Paolo's approach (talking about patch #2 of that set, the procedural
> _PRT is great in any case).

Note that, paradoxically, including the procedural _PRT will cause
Igor's approach to fail, and the only way to fix it would be a similar hack.

But actually Igor's patches make the dependency on the precise table
size much smaller; for example a test like "if (version <=
pc-i440fx-2.0) add 1500 bytes of padding to the tables" would work.

Paolo