pc: fix up pc initialization
PATCH 1/1 pc: fix up pc initialization
commit 7f3e341a008c585deed174eaf1f826c88c67948a
hw/misc: don't create pvpanic device by default
was mismerged: as a result, pvpanic is enabled in 1.6
Fix this up, clean up a trivial code duplication and add a comment explaining why we special-case 1.5 with respect to pvpanic.
--- hw/i386/pc_piix.c | 6 +++--- hw/i386/pc_q35.c | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-)
hw/i386/pc_piix.c +3 -3
@@ -252,12 +252,12 @@ static void pc_init_pci(QEMUMachineInitArgs *args)static void pc_init_pci_1_6 (QEMUMachineInitArgs *args) { has_pci_info = false; - has_pvpanic = true;pc_init_pci (args); }static void pc_init_pci_1_5 (QEMUMachineInitArgs *args) { + has_pvpanic = true;pc_init_pci_1_6 (args); }
@@ -265,8 +265,8 @@ static void pc_init_pci_1_4(QEMUMachineInitArgs *args) {x86_cpu_compat_set_features ("n270" ,FEAT_1_ECX ,0 ,CPUID_EXT_MOVBE );x86_cpu_compat_set_features ("Westmere" ,FEAT_1_ECX ,0 ,CPUID_EXT_PCLMULQDQ ); - has_pci_info = false; -pc_init_pci (args); +/* 1.5 was special as it has pvpanic as a builtin */ +pc_init_pci_1_6 (args); }static void pc_init_pci_1_3 (QEMUMachineInitArgs *args)
hw/i386/pc_q35.c +3 -3
@@ -221,12 +221,12 @@ static void pc_q35_init(QEMUMachineInitArgs *args)static void pc_q35_init_1_6 (QEMUMachineInitArgs *args) { has_pci_info = false; - has_pvpanic = true;pc_q35_init (args); }static void pc_q35_init_1_5 (QEMUMachineInitArgs *args) { + has_pvpanic = true;pc_q35_init_1_6 (args); }
@@ -234,8 +234,8 @@ static void pc_q35_init_1_4(QEMUMachineInitArgs *args) {x86_cpu_compat_set_features ("n270" ,FEAT_1_ECX ,0 ,CPUID_EXT_MOVBE );x86_cpu_compat_set_features ("Westmere" ,FEAT_1_ECX ,0 ,CPUID_EXT_PCLMULQDQ ); - has_pci_info = false; -pc_q35_init (args); +/* 1.5 was special as it has pvpanic as a builtin */ +pc_q35_init_1_6 (args); }static QEMUMachine pc_q35_machine_v1_6 = {
Signature · 1 line
MST
Quoted 8 lines · Michael S. Tsirkin
commit
7f3e341a008c585deed174eaf1f826c88c67948a
hw/misc: don't create pvpanic device by default
was mismerged: as a result, pvpanic is enabled in 1.6Fix this up, clean up a trivial code duplication and add a comment explaining why we special-case 1.5 with respect to pvpanic.
Quoted 6 lines
Reported-by Markus Armbruster <[email protected]>Signed-off-by Michael S. Tsirkin <[email protected]>--- hw/i386/pc_piix.c | 6 +++--- hw/i386/pc_q35.c | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Quoted 10 lines · Michael S. Tsirkin
commit
7f3e341a008c585deed174eaf1f826c88c67948a
hw/misc: don't create pvpanic device by default
was mismerged: as a result, pvpanic is enabled in 1.6Fix this up, clean up a trivial code duplication and add a comment explaining why we special-case 1.5 with respect to pvpanic.
Reported-by Markus Armbruster <[email protected]>Signed-off-by Michael S. Tsirkin <[email protected]>
Thanks for catching this. I'm a little disturbed by this. I use git-am --3way specifically to avoid problems from fuzzing but I guess merge artifacts are possible.
Quoted 12 lines
--- hw/i386/pc_piix.c | 6 +++--- hw/i386/pc_q35.c | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-)hw/i386/pc_piix.c +0
-0@@ -252,12 +252,12 @@ static void pc_init_pci(QEMUMachineInitArgs *args)static void pc_init_pci_1_6 (QEMUMachineInitArgs *args) {
hw/i386/pc_piix.c static void pc_init_pci(QEMUMachineInitArgs *args) · line 262
Quoted 10 lines
hw/i386/pc_piix.c +1
-1has_pci_info = false; - has_pvpanic = true;pc_init_pci (args); }static void pc_init_pci_1_5 (QEMUMachineInitArgs *args) { + has_pvpanic = true;pc_init_pci_1_6 (args); }
I'd prefer we stick to the minimal fix. If you want to refactor the code lets do it separately. I just sent a patch fixing the merge problem.
Regards,
Anthony Liguori
Quoted 43 lines
hw/i386/pc_piix.c +2
-2@@ -265,8 +265,8 @@ static void pc_init_pci_1_4(QEMUMachineInitArgs *args) {x86_cpu_compat_set_features ("n270" ,FEAT_1_ECX ,0 ,CPUID_EXT_MOVBE );x86_cpu_compat_set_features ("Westmere" ,FEAT_1_ECX ,0 ,CPUID_EXT_PCLMULQDQ ); - has_pci_info = false; -pc_init_pci (args); +/* 1.5 was special as it has pvpanic as a builtin */ +pc_init_pci_1_6 (args); }static void pc_init_pci_1_3 (QEMUMachineInitArgs *args)hw/i386/pc_q35.c +3
-3@@ -221,12 +221,12 @@ static void pc_q35_init(QEMUMachineInitArgs *args)static void pc_q35_init_1_6 (QEMUMachineInitArgs *args) { has_pci_info = false; - has_pvpanic = true;pc_q35_init (args); }static void pc_q35_init_1_5 (QEMUMachineInitArgs *args) { + has_pvpanic = true;pc_q35_init_1_6 (args); }@@ -234,8 +234,8 @@ static void pc_q35_init_1_4(QEMUMachineInitArgs *args) {x86_cpu_compat_set_features ("n270" ,FEAT_1_ECX ,0 ,CPUID_EXT_MOVBE );x86_cpu_compat_set_features ("Westmere" ,FEAT_1_ECX ,0 ,CPUID_EXT_PCLMULQDQ ); - has_pci_info = false; -pc_q35_init (args); +/* 1.5 was special as it has pvpanic as a builtin */ +pc_q35_init_1_6 (args); }static QEMUMachine pc_q35_machine_v1_6 = {Signature · 1 line
MST
Il 13/08/2013 16:11, Anthony Liguori ha scritto:
Quoted 10 lines
Quoted 6 lines
Quoted 6 lines
Fix this up, clean up a trivial code duplication and add a comment explaining why we special-case 1.5 with respect to pvpanic.
Reported-by Markus Armbruster <[email protected]>Signed-off-by Michael S. Tsirkin <[email protected]>Thanks for catching this. I'm a little disturbed by this. I use git-am --3way specifically to avoid problems from fuzzing but I guess merge artifacts are possible.
I wonder if we shouldn't disable pvpanic in 1.5 too, one-off behavior is ugly and likely no one will notice.
Paolo
Am 13.08.2013 16:54, schrieb Paolo Bonzini:
Quoted 7 lines
Il 13/08/2013 16:11, Anthony Liguori ha scritto:
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Fix this up, clean up a trivial code duplication and add a comment explaining why we special-case 1.5 with respect to pvpanic.
Reported-by Markus Armbruster <[email protected]>Signed-off-by Michael S. Tsirkin <[email protected]>
Quoted 7 lines
Quoted 4 lines
Thanks for catching this. I'm a little disturbed by this. I use git-am --3way specifically to avoid problems from fuzzing but I guess merge artifacts are possible.
I wonder if we shouldn't disable pvpanic in 1.5 too, one-off behavior is ugly and likely no one will notice.
I had rejected the previous attempt to completely disable pvpanic device because it looked to me as if this compatibility aspect had been forgotten. I didn't imagine the resulting code to look as ugly though, with us "skipping" _1_5 to not have 1.5 overwrite has_pvpanic for 1.6+.
mst suggested to patch stable-1.5 to disable it there, too. I am not against but have doubts as to how well that works with migration, since 1.5.3 is still a bit off and I would expect 1.5.2 -> 1.6.0 migration to work without guest-visible changes... We could argue that having to use -M pc-i440fx-1.5 we can also expect users to add -device pvpanic; question would be how to convey that knowledge of if-you-use-pc-x.y-then-you-also-need-to-do-Z to users, which compat_props usually handle under the hood. We could misuse pvpanic.ioport=0 for that purpose until we have a better solution.
Regards,
Andreas
Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Quoted 31 lines · Andreas Färber
Am 13.08.2013 16:54, schrieb Paolo Bonzini:
Quoted 7 lines
Il 13/08/2013 16:11, Anthony Liguori ha scritto:
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Fix this up, clean up a trivial code duplication and add a comment explaining why we special-case 1.5 with respect to pvpanic.
Reported-by Markus Armbruster <[email protected]>Signed-off-by Michael S. Tsirkin <[email protected]>Quoted 7 lines
Quoted 4 lines
Thanks for catching this. I'm a little disturbed by this. I use git-am --3way specifically to avoid problems from fuzzing but I guess merge artifacts are possible.
I wonder if we shouldn't disable pvpanic in 1.5 too, one-off behavior is ugly and likely no one will notice.
I had rejected the previous attempt to completely disable pvpanic device because it looked to me as if this compatibility aspect had been forgotten. I didn't imagine the resulting code to look as ugly though, with us "skipping" _1_5 to not have 1.5 overwrite
has_pvpanicfor 1.6+.mst suggested to patch stable-1.5 to disable it there, too. I am not against but have doubts as to how well that works with migration, since 1.5.3 is still a bit off and I would expect 1.5.2 -> 1.6.0 migration to work without guest-visible changes... We could argue that having to use -M pc-i440fx-1.5 we can also expect users to add -device pvpanic; question would be how to convey that knowledge of if-you-use-pc-x.y-then-you-also-need-to-do-Z to users, which
compat_propsusually handle under the hood. We could misuse pvpanic.ioport=0 for that purpose until we have a better solution.
Quoted 2 lines
Regards,
Andreas
Well, in this case then this boils down to 1.5.2 migration being buggy - having a small race (migration during reset) which we can then close with 1.5.3 .
I don't think anyone ever triggered this race in practice so I'm not sure whether we should be too worried about this.
Quoted 3 lines
Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Il 13/08/2013 17:11, Andreas Färber ha scritto:
Quoted 10 lines
mst suggested to patch stable-1.5 to disable it there, too. I am not against but have doubts as to how well that works with migration, since 1.5.3 is still a bit off and I would expect 1.5.2 -> 1.6.0 migration to work without guest-visible changes... We could argue that having to use -M pc-i440fx-1.5 we can also expect users to add -device pvpanic; question would be how to convey that knowledge of if-you-use-pc-x.y-then-you-also-need-to-do-Z to users, which
compat_propsusually handle under the hood. We could misuse pvpanic.ioport=0 for that purpose until we have a better solution.
pvpanic has no vmstate though, has it? So it won't break migration (needs testing of course).
All that should happen is that after migration you will not get panic notifications on the destination.
Paolo
Am 13.08.2013 17:27, schrieb Paolo Bonzini:
Quoted 12 lines
Il 13/08/2013 17:11, Andreas Färber ha scritto:
Quoted 10 lines
mst suggested to patch stable-1.5 to disable it there, too. I am not against but have doubts as to how well that works with migration, since 1.5.3 is still a bit off and I would expect 1.5.2 -> 1.6.0 migration to work without guest-visible changes... We could argue that having to use -M pc-i440fx-1.5 we can also expect users to add -device pvpanic; question would be how to convey that knowledge of if-you-use-pc-x.y-then-you-also-need-to-do-Z to users, which
compat_propsusually handle under the hood. We could misuse pvpanic.ioport=0 for that purpose until we have a better solution.
Quoted 2 lines
pvpanic has no vmstate though, has it? So it won't break migration (needs testing of course).
Just checked, you're right, it doesn't. I somehow assumed it would.
Quoted 2 lines
All that should happen is that after migration you will not get panic notifications on the destination.
Well, how does the Linux driver cope with pvpanic device present on boot but not present on panic? ISA PIO is not usually expected to be hot-unplugged. ;)
Andreas
Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Quoted 16 lines · Andreas Färber
Am 13.08.2013 17:27, schrieb Paolo Bonzini:
Quoted 12 lines
Il 13/08/2013 17:11, Andreas Färber ha scritto:
Quoted 10 lines
mst suggested to patch stable-1.5 to disable it there, too. I am not against but have doubts as to how well that works with migration, since 1.5.3 is still a bit off and I would expect 1.5.2 -> 1.6.0 migration to work without guest-visible changes... We could argue that having to use -M pc-i440fx-1.5 we can also expect users to add -device pvpanic; question would be how to convey that knowledge of if-you-use-pc-x.y-then-you-also-need-to-do-Z to users, which
compat_propsusually handle under the hood. We could misuse pvpanic.ioport=0 for that purpose until we have a better solution.Quoted 2 lines
pvpanic has no vmstate though, has it? So it won't break migration (needs testing of course).
Quoted 8 lines
Just checked, you're right, it doesn't. I somehow assumed it would.
Quoted 2 lines
All that should happen is that after migration you will not get panic notifications on the destination.
Well, how does the Linux driver cope with pvpanic device present on boot but not present on panic? ISA PIO is not usually expected to be hot-unplugged. ;)
Yeah, disappearing doesn't seem acceptable to me.
We made a mistake with 1.5, now we have to live with it.
Regards,
Anthony Liguori
Quoted 6 lines
Andreas
Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Il 13/08/2013 17:57, Andreas Färber ha scritto:
Quoted 7 lines
Am 13.08.2013 17:27, schrieb Paolo Bonzini:
Quoted 2 lines
All that should happen is that after migration you will not get panic notifications on the destination.
Well, how does the Linux driver cope with pvpanic device present on boot but not present on panic? ISA PIO is not usually expected to be hot-unplugged. ;)
On panic, the driver does a single "outb". That outb will simply go to the bitbucket.
Paolo