qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH for-2.1 1/2] migration: load smaller RAMBlock to


From: Igor Mammedov
Subject: Re: [Qemu-devel] [PATCH for-2.1 1/2] migration: load smaller RAMBlock to a bigger one if permitted
Date: Mon, 28 Jul 2014 09:40:22 +0200

On Fri, 25 Jul 2014 19:56:40 +0200
Laszlo Ersek <address@hidden> 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 <address@hidden>
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> >  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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]