hw/i386/pc.c | 2 +- hw/i386/pc_piix.c | 1 + hw/i386/pc_q35.c | 1 + include/hw/i386/pc.h | 6 ++---- 4 files changed, 5 insertions(+), 5 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>
---
hw/i386/pc.c | 2 +-
hw/i386/pc_piix.c | 1 +
hw/i386/pc_q35.c | 1 +
include/hw/i386/pc.h | 6 ++----
4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f3b372a18f..3f2d96da64 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1047,7 +1047,7 @@ 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)) {
+ if (!pcmc->linuxboot_dma_disabled && fw_cfg_dma_enabled(fw_cfg)) {
option_rom[nb_option_roms].name = "linuxboot_dma.bin";
option_rom[nb_option_roms].bootindex = 0;
} else {
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 9f102aa388..dd3a2bb02a 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_disabled = true;
SET_MACHINE_COMPAT(m, PC_COMPAT_2_6);
}
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index dd792a8547..9988ecc578 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_disabled = true;
SET_MACHINE_COMPAT(m, PC_COMPAT_2_6);
}
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index f278b3ae89..ff6f13b61b 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -151,6 +151,8 @@ struct PCMachineClass {
bool save_tsc_khz;
/* generate legacy CPU hotplug AML */
bool legacy_cpu_hotplug;
+
+ bool linuxboot_dma_disabled;
};
#define TYPE_PC_MACHINE "generic-pc-machine"
@@ -432,10 +434,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 Mon, Apr 24, 2017 at 08:58:17PM +0200, Igor Mammedov wrote:
[...]
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f3b372a18f..3f2d96da64 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1047,7 +1047,7 @@ 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)) {
> + if (!pcmc->linuxboot_dma_disabled && fw_cfg_dma_enabled(fw_cfg)) {
Why not name the flag just "linuxboot_dma", set it to true by
default at pc_machine_class_init(), and avoid the double
negative?
[...]
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index f278b3ae89..ff6f13b61b 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -151,6 +151,8 @@ struct PCMachineClass {
> bool save_tsc_khz;
> /* generate legacy CPU hotplug AML */
> bool legacy_cpu_hotplug;
> +
A one-line description of the consequences of setting/clearing
the flag would be nice.
> + bool linuxboot_dma_disabled;
> };
>
> #define TYPE_PC_MACHINE "generic-pc-machine"
> @@ -432,10 +434,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)
>
--
Eduardo
On Mon, 24 Apr 2017 16:13:17 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Mon, Apr 24, 2017 at 08:58:17PM +0200, Igor Mammedov wrote:
> [...]
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index f3b372a18f..3f2d96da64 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1047,7 +1047,7 @@ 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)) {
> > + if (!pcmc->linuxboot_dma_disabled && fw_cfg_dma_enabled(fw_cfg)) {
>
> Why not name the flag just "linuxboot_dma", set it to true by
> default at pc_machine_class_init(), and avoid the double
> negative?
to avoid setting it to true somewhere else, so less thing could go wrong
but is you prefer *_enable variant I can switch to it.
>
> [...]
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index f278b3ae89..ff6f13b61b 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -151,6 +151,8 @@ struct PCMachineClass {
> > bool save_tsc_khz;
> > /* generate legacy CPU hotplug AML */
> > bool legacy_cpu_hotplug;
> > +
>
> A one-line description of the consequences of setting/clearing
> the flag would be nice.
will fix in v2
>
> > + bool linuxboot_dma_disabled;
> > };
> >
> > #define TYPE_PC_MACHINE "generic-pc-machine"
> > @@ -432,10 +434,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 Mon, Apr 24, 2017 at 09:32:33PM +0200, Igor Mammedov wrote:
> On Mon, 24 Apr 2017 16:13:17 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > On Mon, Apr 24, 2017 at 08:58:17PM +0200, Igor Mammedov wrote:
> > [...]
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index f3b372a18f..3f2d96da64 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -1047,7 +1047,7 @@ 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)) {
> > > + if (!pcmc->linuxboot_dma_disabled && fw_cfg_dma_enabled(fw_cfg)) {
> >
> > Why not name the flag just "linuxboot_dma", set it to true by
> > default at pc_machine_class_init(), and avoid the double
> > negative?
> to avoid setting it to true somewhere else, so less thing could go wrong
> but is you prefer *_enable variant I can switch to it.
I would prefer to. We already have other compat flags initialized
inside pc_machine_class_init(), so this would fit nicely there.
>
> >
> > [...]
> > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > index f278b3ae89..ff6f13b61b 100644
> > > --- a/include/hw/i386/pc.h
> > > +++ b/include/hw/i386/pc.h
> > > @@ -151,6 +151,8 @@ struct PCMachineClass {
> > > bool save_tsc_khz;
> > > /* generate legacy CPU hotplug AML */
> > > bool legacy_cpu_hotplug;
> > > +
> >
> > A one-line description of the consequences of setting/clearing
> > the flag would be nice.
> will fix in v2
Thanks!
>
> >
> > > + bool linuxboot_dma_disabled;
> > > };
> > >
> > > #define TYPE_PC_MACHINE "generic-pc-machine"
> > > @@ -432,10 +434,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)
> > >
> >
>
--
Eduardo
On Mon, 24 Apr 2017 16:37:31 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Mon, Apr 24, 2017 at 09:32:33PM +0200, Igor Mammedov wrote:
> > On Mon, 24 Apr 2017 16:13:17 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> > > On Mon, Apr 24, 2017 at 08:58:17PM +0200, Igor Mammedov wrote:
> > > [...]
> > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > > index f3b372a18f..3f2d96da64 100644
> > > > --- a/hw/i386/pc.c
> > > > +++ b/hw/i386/pc.c
> > > > @@ -1047,7 +1047,7 @@ 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)) {
> > > > + if (!pcmc->linuxboot_dma_disabled && fw_cfg_dma_enabled(fw_cfg)) {
> > >
> > > Why not name the flag just "linuxboot_dma", set it to true by
> > > default at pc_machine_class_init(), and avoid the double
> > > negative?
> > to avoid setting it to true somewhere else, so less thing could go wrong
> > but is you prefer *_enable variant I can switch to it.
>
> I would prefer to. We already have other compat flags initialized
> inside pc_machine_class_init(), so this would fit nicely there.
how about 'use_linuxboot_mmio' instead, it will remove negation
and let me not to touch pc_machine_class_init()?
>
> >
> > >
> > > [...]
> > > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > > index f278b3ae89..ff6f13b61b 100644
> > > > --- a/include/hw/i386/pc.h
> > > > +++ b/include/hw/i386/pc.h
> > > > @@ -151,6 +151,8 @@ struct PCMachineClass {
> > > > bool save_tsc_khz;
> > > > /* generate legacy CPU hotplug AML */
> > > > bool legacy_cpu_hotplug;
> > > > +
> > >
> > > A one-line description of the consequences of setting/clearing
> > > the flag would be nice.
> > will fix in v2
>
> Thanks!
>
> >
> > >
> > > > + bool linuxboot_dma_disabled;
> > > > };
> > > >
> > > > #define TYPE_PC_MACHINE "generic-pc-machine"
> > > > @@ -432,10 +434,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 Mon, Apr 24, 2017 at 09:56:04PM +0200, Igor Mammedov wrote:
> On Mon, 24 Apr 2017 16:37:31 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > On Mon, Apr 24, 2017 at 09:32:33PM +0200, Igor Mammedov wrote:
> > > On Mon, 24 Apr 2017 16:13:17 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >
> > > > On Mon, Apr 24, 2017 at 08:58:17PM +0200, Igor Mammedov wrote:
> > > > [...]
> > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > > > index f3b372a18f..3f2d96da64 100644
> > > > > --- a/hw/i386/pc.c
> > > > > +++ b/hw/i386/pc.c
> > > > > @@ -1047,7 +1047,7 @@ 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)) {
> > > > > + if (!pcmc->linuxboot_dma_disabled && fw_cfg_dma_enabled(fw_cfg)) {
> > > >
> > > > Why not name the flag just "linuxboot_dma", set it to true by
> > > > default at pc_machine_class_init(), and avoid the double
> > > > negative?
> > > to avoid setting it to true somewhere else, so less thing could go wrong
> > > but is you prefer *_enable variant I can switch to it.
> >
> > I would prefer to. We already have other compat flags initialized
> > inside pc_machine_class_init(), so this would fit nicely there.
> how about 'use_linuxboot_mmio' instead, it will remove negation
> and let me not to touch pc_machine_class_init()?
Sounds good to me.
--
Eduardo
© 2016 - 2024 Red Hat, Inc.