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

[Laszlo Ersek]

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 <address@hidden>
> ---
> v2:
>   (Eduardo Habkost <address@hidden>)
>     * 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.

- 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.

With those (or without, as you see fit):

Reviewed-by: Laszlo Ersek <address@hidden>

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 <address@hidden>
Analyzed-by: Laszlo Ersek <address@hidden>

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",\
>