Re: [Qemu-devel] [PATCH v2] pc/fwcfg: unbreak migration from qemu-2.5 and qemu-2.6 during firmware boot

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 25 Apr 2017 17:01:21 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 04/24/17 22:45, Igor Mammedov wrote:
> > Since 2.7 commit (b2a575a Add optionrom compatible with fw_cfg DMA version)
> > regressed migration during firmware exection time by
> > abusing fwcfg.dma_enabled property to decide loading
> > dma version of option rom AND by mistake disabling DMA
> > for 2.6 and earlier globally instead of only for option rom.
> > 
> > so 2.6 machine type guest is broken when it already runs
> > firmware in DMA mode but migrated to qemu-2.7(pc-2.6)
> > at that time;
> > 
> > a) qemu-2.6:pc2.6 (fwcfg.dma=on,firmware=dma,oprom=mmio)
> > b) qemu-2.7:pc2.6 (fwcfg.dma=off,firmware=mmio,oprom=mmio)
> > 
> >   to:   a     b
> > from
> > a       OK   FAIL
> > b       OK   OK
> > 
> > So we currently have broken forward migration from
> > qemu-2.6 to qemu-2.[789] that however could be fixed
> > for 2.10 by re-enabling DMA for 2.[56] machine types
> > and allowing dma capable option rom only since 2.7.
> > As result qemu should end up with:
> > 
> > c) qemu-2.10:pc2.6 (fwcfg.dma=on,firmware=dma,oprom=mmio)
> > 
> >    to:  a     b    c
> > from
> > a      OK   FAIL  OK
> > b      OK   OK    OK
> > c      OK   FAIL  OK
> > 
> > where forward migration from qemu-2.6 to qemu-2.10 should
> > work again leaving only qemu-2.[789]:pc-2.6 broken.
> > 
> > Patch should also help downstream to maintain migration
> > the way it used to be since dma cable option rom
> > is managed by new
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v2:
> >   (Eduardo Habkost <ehabkost@redhat.com>)
> >     * s/linuxboot_dma_disabled/linuxboot_dma_enabled/
> >     * add comment to linuxboot_dma_enabled field
> > ---
> >  hw/i386/pc.c         | 9 ++++-----
> >  hw/i386/pc_piix.c    | 1 +
> >  hw/i386/pc_q35.c     | 1 +
> >  include/hw/i386/pc.h | 7 +++----
> >  4 files changed, 9 insertions(+), 9 deletions(-)
> 
> Two suggestions for the commit message:
> 
> - The last paragraph contains a typo (s/cable/capable/), plus it
> generally looks unfinished. Please clean up that paragraph, or drop it.
I'll drop it

> 
> - Since this is PC and on PC we use IO port mapped fw_cfg in the absence
> of DMA (and not MMIO mapped fw_cfg), I suggest to replace all
> occurrences of "mmio" with "ioport" in the commit message. Not
> "critical" of course, just a bit more precise IMO.
will fix in v3

> 
> With those (or without, as you see fit):
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> In addition, if you and Eduardo agree, I would like to record that I too
> worked quite a bit on the original bug analysis, so please consider adding:
> 
> Reported-by: Eduardo Habkost <ehabkost@redhat.com>
> Analyzed-by: Laszlo Ersek <lersek@redhat.com>
sure, I'll add it on repost

> 
> Thank you very much for the patch.
> Laszlo
> 
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index f3b372a18f..8063241140 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1047,12 +1047,10 @@ static void load_linux(PCMachineState *pcms,
> >      fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_SIZE, setup_size);
> >      fw_cfg_add_bytes(fw_cfg, FW_CFG_SETUP_DATA, setup, setup_size);
> >  
> > -    if (fw_cfg_dma_enabled(fw_cfg)) {
> > +    option_rom[nb_option_roms].bootindex = 0;
> > +    option_rom[nb_option_roms].name = "linuxboot.bin";
> > +    if (pcmc->linuxboot_dma_enabled && fw_cfg_dma_enabled(fw_cfg)) {
> >          option_rom[nb_option_roms].name = "linuxboot_dma.bin";
> > -        option_rom[nb_option_roms].bootindex = 0;
> > -    } else {
> > -        option_rom[nb_option_roms].name = "linuxboot.bin";
> > -        option_rom[nb_option_roms].bootindex = 0;
> >      }
> >      nb_option_roms++;
> >  }
> > @@ -2321,6 +2319,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
> >       * to be used at the moment, 32K should be enough for a while.  */
> >      pcmc->acpi_data_size = 0x20000 + 0x8000;
> >      pcmc->save_tsc_khz = true;
> > +    pcmc->linuxboot_dma_enabled = true;
> >      mc->get_hotplug_handler = pc_get_hotpug_handler;
> >      mc->cpu_index_to_socket_id = pc_cpu_index_to_socket_id;
> >      mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids;
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 9f102aa388..a11190be46 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -474,6 +474,7 @@ static void pc_i440fx_2_6_machine_options(MachineClass *m)
> >      PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> >      pc_i440fx_2_7_machine_options(m);
> >      pcmc->legacy_cpu_hotplug = true;
> > +    pcmc->linuxboot_dma_enabled = false;
> >      SET_MACHINE_COMPAT(m, PC_COMPAT_2_6);
> >  }
> >  
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index dd792a8547..0a61a2070c 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -335,6 +335,7 @@ static void pc_q35_2_6_machine_options(MachineClass *m)
> >      PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> >      pc_q35_2_7_machine_options(m);
> >      pcmc->legacy_cpu_hotplug = true;
> > +    pcmc->linuxboot_dma_enabled = false;
> >      SET_MACHINE_COMPAT(m, PC_COMPAT_2_6);
> >  }
> >  
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index f278b3ae89..a57c607a8c 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -151,6 +151,9 @@ struct PCMachineClass {
> >      bool save_tsc_khz;
> >      /* generate legacy CPU hotplug AML */
> >      bool legacy_cpu_hotplug;
> > +
> > +    /* use DMA capable linuxboot option rom */
> > +    bool linuxboot_dma_enabled;
> >  };
> >  
> >  #define TYPE_PC_MACHINE "generic-pc-machine"
> > @@ -432,10 +435,6 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
> >  #define PC_COMPAT_2_6 \
> >      HW_COMPAT_2_6 \
> >      {\
> > -        .driver   = "fw_cfg_io",\
> > -        .property = "dma_enabled",\
> > -        .value    = "off",\
> > -    },{\
> >          .driver   = TYPE_X86_CPU,\
> >          .property = "cpuid-0xb",\
> >          .value    = "off",\
> > 
>