| Message ID | 20130813125349.GA28317@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <armbru@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@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 <eblake@redhat.com>
"Michael S. Tsirkin" <mst@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 <armbru@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@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
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 <armbru@redhat.com> >> > Signed-off-by: Michael S. Tsirkin <mst@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
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 <armbru@redhat.com> >>>> Signed-off-by: Michael S. Tsirkin <mst@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
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 <armbru@redhat.com> > >>>> Signed-off-by: Michael S. Tsirkin <mst@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
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
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
Andreas Färber <afaerber@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
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
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 = {
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 <armbru@redhat.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- hw/i386/pc_piix.c | 6 +++--- hw/i386/pc_q35.c | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-)