[Qemu-devel] [PATCH for-1.6] pc: fix up pc initialization

[Qemu-devel] [PATCH for-1.6] pc: fix up pc initialization

[Michael S. Tsirkin]

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(-)

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 750 bytes --]

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>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH for-1.6] pc: fix up pc initialization

[Anthony Liguori]

"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

Re: [Qemu-devel] [PATCH for-1.6] pc: fix up pc initialization

[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.

Paolo

Re: [Qemu-devel] [PATCH for-1.6] pc: fix up pc initialization

[Andreas Färber]

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

-- 
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

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH for-1.6] pc: fix up pc initialization

[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).

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

[Andreas Färber]

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

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH for-1.6] pc: fix up pc initialization

[Paolo Bonzini]

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