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 <imamm...@redhat.com>
> ---
> v2:
> (Eduardo Habkost <ehabk...@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.
- 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 <ler...@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 <ehabk...@redhat.com>
Analyzed-by: Laszlo Ersek <ler...@redhat.com>
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",\
>
