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(-)
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(-)
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",\
--
2.11.0 (Apple Git-81)
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.
- 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 <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>
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",\
>
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",\
> >
>
© 2016 - 2024 Red Hat, Inc.