[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] pc/fwcfg: unbreak migration from qemu-2.5 an
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH v2] pc/fwcfg: unbreak migration from qemu-2.5 and qemu-2.6 during firmware boot |
Date: |
Tue, 25 Apr 2017 17:24:10 +0200 |
On Tue, 25 Apr 2017 17:01:21 +0200
Laszlo Ersek <address@hidden> 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 <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.
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 <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>
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",\
> >
>