Re: [Qemu-devel] [PATCH for-1.6] pc: fix up pc initialization
Il 13/08/2013 17:57, Andreas Färber ha scritto: Am 13.08.2013 17:27, schrieb Paolo Bonzini: 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
[Qemu-devel] [PATCH for-1.6] 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.
Reported-by: Markus Armbruster arm...@redhat.com
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
hw/i386/pc_piix.c | 6 +++---
hw/i386/pc_q35.c | 6 +++---
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 55c24f2..c58f0f4 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -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)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index bd25071..968b22b 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -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 = {
--
MST
Re: [Qemu-devel] [PATCH for-1.6] pc: fix up pc initialization
On 08/13/2013 06:53 AM, Michael S. Tsirkin wrote: 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. Reported-by: Markus Armbruster arm...@redhat.com Signed-off-by: Michael S. Tsirkin m...@redhat.com --- hw/i386/pc_piix.c | 6 +++--- hw/i386/pc_q35.c | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) Reviewed-by: Eric Blake ebl...@redhat.com -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH for-1.6] pc: fix up pc initialization
Michael S. Tsirkin m...@redhat.com writes:
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.
Reported-by: Markus Armbruster arm...@redhat.com
Signed-off-by: Michael S. Tsirkin m...@redhat.com
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.
---
hw/i386/pc_piix.c | 6 +++---
hw/i386/pc_q35.c | 6 +++---
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 55c24f2..c58f0f4 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -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);
}
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
@@ -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)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index bd25071..968b22b 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -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 = {
--
MST
Re: [Qemu-devel] [PATCH for-1.6] pc: fix up pc initialization
Il 13/08/2013 16:11, Anthony Liguori ha scritto: 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 arm...@redhat.com Signed-off-by: Michael S. Tsirkin m...@redhat.com 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
Re: [Qemu-devel] [PATCH for-1.6] pc: fix up pc initialization
Am 13.08.2013 16:54, schrieb Paolo Bonzini: Il 13/08/2013 16:11, Anthony Liguori ha scritto: 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 arm...@redhat.com Signed-off-by: Michael S. Tsirkin m...@redhat.com 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 -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH for-1.6] pc: fix up pc initialization
On Tue, Aug 13, 2013 at 05:11:08PM +0200, Andreas Färber wrote: Am 13.08.2013 16:54, schrieb Paolo Bonzini: Il 13/08/2013 16:11, Anthony Liguori ha scritto: 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 arm...@redhat.com Signed-off-by: Michael S. Tsirkin m...@redhat.com 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 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. -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH for-1.6] pc: fix up pc initialization
Il 13/08/2013 17:11, Andreas Färber ha scritto: 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. 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
Re: [Qemu-devel] [PATCH for-1.6] pc: fix up pc initialization
Am 13.08.2013 17:27, schrieb Paolo Bonzini: Il 13/08/2013 17:11, Andreas Färber ha scritto: 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. 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. 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 -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH for-1.6] pc: fix up pc initialization
Andreas Färber afaer...@suse.de writes: Am 13.08.2013 17:27, schrieb Paolo Bonzini: Il 13/08/2013 17:11, Andreas Färber ha scritto: 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. 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. 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 Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
