QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [Qemu-devel] [PATCH v2] pc: Don't make die-id mandatory unless necessary
@ 2019-08-16 17:07 Eduardo Habkost
  2019-08-26 14:05 ` Igor Mammedov
  2019-08-28  6:52 ` Markus Armbruster
  0 siblings, 2 replies; 7+ messages in thread
From: Eduardo Habkost @ 2019-08-16 17:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Krempa, Vanderson Martins do Rosario, Eduardo Habkost,
	Like Xu, Erik Skultety, Michael S. Tsirkin, qemu-stable,
	Markus Armbruster, Igor Mammedov, Paolo Bonzini,
	Richard Henderson

We have this issue reported when using libvirt to hotplug CPUs:
https://bugzilla.redhat.com/show_bug.cgi?id=1741451

Basically, libvirt is not copying die-id from
query-hotpluggable-cpus, but die-id is now mandatory.

We could blame libvirt and say it is not following the documented
interface, because we have this buried in the QAPI schema
documentation:

> Note: currently there are 5 properties that could be present
> but management should be prepared to pass through other
> properties with device_add command to allow for future
> interface extension. This also requires the filed names to be kept in
> sync with the properties passed to -device/device_add.

But I don't think this would be reasonable from us.  We can just
make QEMU more flexible and let die-id to be omitted when there's
no ambiguity.  This will allow us to keep compatibility with
existing libvirt versions.

Test case included to ensure we don't break this again.

Fixes: commit 176d2cda0dee ("i386/cpu: Consolidate die-id validity in smp context")
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
* v1 was "pc: Don't make CPU properties mandatory unless necessary"
* Make only die-id optional (Igor Mammedov)
---
 hw/i386/pc.c                             |  8 ++++++
 tests/acceptance/pc_cpu_hotplug_props.py | 35 ++++++++++++++++++++++++
 2 files changed, 43 insertions(+)
 create mode 100644 tests/acceptance/pc_cpu_hotplug_props.py

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3ab4bcb3ca..9c3f6ae828 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2406,6 +2406,14 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
         int max_socket = (ms->smp.max_cpus - 1) /
                                 smp_threads / smp_cores / pcms->smp_dies;
 
+        /*
+         * die-id was optional in QEMU 4.0 and older, so keep it optional
+         * if there's only one die per socket.
+         */
+        if (cpu->die_id < 0 && pcms->smp_dies == 1) {
+            cpu->die_id = 0;
+        }
+
         if (cpu->socket_id < 0) {
             error_setg(errp, "CPU socket-id is not set");
             return;
diff --git a/tests/acceptance/pc_cpu_hotplug_props.py b/tests/acceptance/pc_cpu_hotplug_props.py
new file mode 100644
index 0000000000..08b7e632c6
--- /dev/null
+++ b/tests/acceptance/pc_cpu_hotplug_props.py
@@ -0,0 +1,35 @@
+#
+# Ensure CPU die-id can be omitted on -device
+#
+#  Copyright (c) 2019 Red Hat Inc
+#
+# Author:
+#  Eduardo Habkost <ehabkost@redhat.com>
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public
+# License as published by the Free Software Foundation; either
+# version 2 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library; if not, see <http://www.gnu.org/licenses/>.
+#
+
+from avocado_qemu import Test
+
+class OmittedCPUProps(Test):
+    """
+    :avocado: tags=arch:x86_64
+    """
+    def test_no_die_id(self):
+        self.vm.add_args('-nodefaults', '-S')
+        self.vm.add_args('-smp', '1,sockets=2,cores=2,threads=2,maxcpus=8')
+        self.vm.add_args('-cpu', 'qemu64')
+        self.vm.add_args('-device', 'qemu64-x86_64-cpu,socket-id=1,core-id=0,thread-id=0')
+        self.vm.launch()
+        self.assertEquals(len(self.vm.command('query-cpus')), 2)
-- 
2.21.0



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH v2] pc: Don't make die-id mandatory unless necessary
  2019-08-16 17:07 [Qemu-devel] [PATCH v2] pc: Don't make die-id mandatory unless necessary Eduardo Habkost
@ 2019-08-26 14:05 ` Igor Mammedov
  2019-08-28  6:52 ` Markus Armbruster
  1 sibling, 0 replies; 7+ messages in thread
From: Igor Mammedov @ 2019-08-26 14:05 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: Peter Krempa, Vanderson Martins do Rosario, Like Xu,
	Erik Skultety, Markus Armbruster, Michael S. Tsirkin, qemu-devel,
	qemu-stable, Paolo Bonzini, Richard Henderson

On Fri, 16 Aug 2019 14:07:50 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> We have this issue reported when using libvirt to hotplug CPUs:
> https://bugzilla.redhat.com/show_bug.cgi?id=1741451
> 
> Basically, libvirt is not copying die-id from
> query-hotpluggable-cpus, but die-id is now mandatory.
> 
> We could blame libvirt and say it is not following the documented
> interface, because we have this buried in the QAPI schema
> documentation:
> 
> > Note: currently there are 5 properties that could be present
> > but management should be prepared to pass through other
> > properties with device_add command to allow for future
> > interface extension. This also requires the filed names to be kept in
> > sync with the properties passed to -device/device_add.  
> 
> But I don't think this would be reasonable from us.  We can just
> make QEMU more flexible and let die-id to be omitted when there's
> no ambiguity.  This will allow us to keep compatibility with
> existing libvirt versions.
> 
> Test case included to ensure we don't break this again.
> 
> Fixes: commit 176d2cda0dee ("i386/cpu: Consolidate die-id validity in smp context")
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
> Changes v1 -> v2:
> * v1 was "pc: Don't make CPU properties mandatory unless necessary"
> * Make only die-id optional (Igor Mammedov)
> ---
>  hw/i386/pc.c                             |  8 ++++++
>  tests/acceptance/pc_cpu_hotplug_props.py | 35 ++++++++++++++++++++++++
>  2 files changed, 43 insertions(+)
>  create mode 100644 tests/acceptance/pc_cpu_hotplug_props.py
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 3ab4bcb3ca..9c3f6ae828 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2406,6 +2406,14 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>          int max_socket = (ms->smp.max_cpus - 1) /
>                                  smp_threads / smp_cores / pcms->smp_dies;
>  
> +        /*
> +         * die-id was optional in QEMU 4.0 and older, so keep it optional
> +         * if there's only one die per socket.
> +         */
> +        if (cpu->die_id < 0 && pcms->smp_dies == 1) {
> +            cpu->die_id = 0;
> +        }
> +
>          if (cpu->socket_id < 0) {
>              error_setg(errp, "CPU socket-id is not set");
>              return;
> diff --git a/tests/acceptance/pc_cpu_hotplug_props.py b/tests/acceptance/pc_cpu_hotplug_props.py
> new file mode 100644
> index 0000000000..08b7e632c6
> --- /dev/null
> +++ b/tests/acceptance/pc_cpu_hotplug_props.py
> @@ -0,0 +1,35 @@
> +#
> +# Ensure CPU die-id can be omitted on -device
> +#
> +#  Copyright (c) 2019 Red Hat Inc
> +#
> +# Author:
> +#  Eduardo Habkost <ehabkost@redhat.com>
> +#
> +# This library is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU Lesser General Public
> +# License as published by the Free Software Foundation; either
> +# version 2 of the License, or (at your option) any later version.
> +#
> +# This library is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +# Lesser General Public License for more details.
> +#
> +# You should have received a copy of the GNU Lesser General Public
> +# License along with this library; if not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +from avocado_qemu import Test
> +
> +class OmittedCPUProps(Test):
> +    """
> +    :avocado: tags=arch:x86_64
> +    """
> +    def test_no_die_id(self):
> +        self.vm.add_args('-nodefaults', '-S')
> +        self.vm.add_args('-smp', '1,sockets=2,cores=2,threads=2,maxcpus=8')
> +        self.vm.add_args('-cpu', 'qemu64')
> +        self.vm.add_args('-device', 'qemu64-x86_64-cpu,socket-id=1,core-id=0,thread-id=0')
> +        self.vm.launch()
> +        self.assertEquals(len(self.vm.command('query-cpus')), 2)



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH v2] pc: Don't make die-id mandatory unless necessary
  2019-08-16 17:07 [Qemu-devel] [PATCH v2] pc: Don't make die-id mandatory unless necessary Eduardo Habkost
  2019-08-26 14:05 ` Igor Mammedov
@ 2019-08-28  6:52 ` Markus Armbruster
  2019-08-28 14:35   ` Eduardo Habkost
  1 sibling, 1 reply; 7+ messages in thread
From: Markus Armbruster @ 2019-08-28  6:52 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: Peter Krempa, Vanderson Martins do Rosario, Like Xu,
	Michael S. Tsirkin, Erik Skultety, qemu-devel, qemu-stable,
	Paolo Bonzini, Igor Mammedov, Richard Henderson

Eduardo Habkost <ehabkost@redhat.com> writes:

> We have this issue reported when using libvirt to hotplug CPUs:
> https://bugzilla.redhat.com/show_bug.cgi?id=1741451
>
> Basically, libvirt is not copying die-id from
> query-hotpluggable-cpus, but die-id is now mandatory.
>
> We could blame libvirt and say it is not following the documented
> interface, because we have this buried in the QAPI schema
> documentation:
>
>> Note: currently there are 5 properties that could be present
>> but management should be prepared to pass through other
>> properties with device_add command to allow for future
>> interface extension. This also requires the filed names to be kept in
>> sync with the properties passed to -device/device_add.
>
> But I don't think this would be reasonable from us.  We can just
> make QEMU more flexible and let die-id to be omitted when there's
> no ambiguity.  This will allow us to keep compatibility with
> existing libvirt versions.
>
> Test case included to ensure we don't break this again.
>
> Fixes: commit 176d2cda0dee ("i386/cpu: Consolidate die-id validity in smp context")
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v1 -> v2:
> * v1 was "pc: Don't make CPU properties mandatory unless necessary"
> * Make only die-id optional (Igor Mammedov)
> ---
>  hw/i386/pc.c                             |  8 ++++++
>  tests/acceptance/pc_cpu_hotplug_props.py | 35 ++++++++++++++++++++++++
>  2 files changed, 43 insertions(+)
>  create mode 100644 tests/acceptance/pc_cpu_hotplug_props.py
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 3ab4bcb3ca..9c3f6ae828 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2406,6 +2406,14 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>          int max_socket = (ms->smp.max_cpus - 1) /
>                                  smp_threads / smp_cores / pcms->smp_dies;
>  
> +        /*
> +         * die-id was optional in QEMU 4.0 and older, so keep it optional
> +         * if there's only one die per socket.
> +         */

Is this comment worth its keep?

> +        if (cpu->die_id < 0 && pcms->smp_dies == 1) {
> +            cpu->die_id = 0;
> +        }
> +
>          if (cpu->socket_id < 0) {
>              error_setg(errp, "CPU socket-id is not set");
>              return;
> diff --git a/tests/acceptance/pc_cpu_hotplug_props.py b/tests/acceptance/pc_cpu_hotplug_props.py
> new file mode 100644
> index 0000000000..08b7e632c6
> --- /dev/null
> +++ b/tests/acceptance/pc_cpu_hotplug_props.py
> @@ -0,0 +1,35 @@
> +#
> +# Ensure CPU die-id can be omitted on -device
> +#
> +#  Copyright (c) 2019 Red Hat Inc
> +#
> +# Author:
> +#  Eduardo Habkost <ehabkost@redhat.com>
> +#
> +# This library is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU Lesser General Public
> +# License as published by the Free Software Foundation; either
> +# version 2 of the License, or (at your option) any later version.
> +#
> +# This library is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +# Lesser General Public License for more details.
> +#
> +# You should have received a copy of the GNU Lesser General Public
> +# License along with this library; if not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +from avocado_qemu import Test
> +
> +class OmittedCPUProps(Test):
> +    """
> +    :avocado: tags=arch:x86_64
> +    """
> +    def test_no_die_id(self):
> +        self.vm.add_args('-nodefaults', '-S')
> +        self.vm.add_args('-smp', '1,sockets=2,cores=2,threads=2,maxcpus=8')
> +        self.vm.add_args('-cpu', 'qemu64')
> +        self.vm.add_args('-device', 'qemu64-x86_64-cpu,socket-id=1,core-id=0,thread-id=0')
> +        self.vm.launch()
> +        self.assertEquals(len(self.vm.command('query-cpus')), 2)

This tests intentionally violates the "a CPU hot-plug with -device must
specify all the properties returned by query-hotpluggable-cpus" rule
laid down in qapi/machine.json, and the change to pc.c permits it.

Missing: a suitable update to the rule.

In addition, I'd like us to either work on making the rule stick in the
future (see my reply to Igor for an idea), or ditch the rule.  But
that's outside the scope of this regression fix.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH v2] pc: Don't make die-id mandatory unless necessary
  2019-08-28  6:52 ` Markus Armbruster
@ 2019-08-28 14:35   ` Eduardo Habkost
  2019-08-28 16:57     ` Igor Mammedov
  0 siblings, 1 reply; 7+ messages in thread
From: Eduardo Habkost @ 2019-08-28 14:35 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Peter Krempa, Vanderson Martins do Rosario, Like Xu,
	Michael S. Tsirkin, Erik Skultety, qemu-devel, qemu-stable,
	Paolo Bonzini, Igor Mammedov, Richard Henderson

On Wed, Aug 28, 2019 at 08:52:28AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > We have this issue reported when using libvirt to hotplug CPUs:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1741451
> >
> > Basically, libvirt is not copying die-id from
> > query-hotpluggable-cpus, but die-id is now mandatory.
> >
> > We could blame libvirt and say it is not following the documented
> > interface, because we have this buried in the QAPI schema
> > documentation:
> >
> >> Note: currently there are 5 properties that could be present
> >> but management should be prepared to pass through other
> >> properties with device_add command to allow for future
> >> interface extension. This also requires the filed names to be kept in
> >> sync with the properties passed to -device/device_add.
> >
> > But I don't think this would be reasonable from us.  We can just
> > make QEMU more flexible and let die-id to be omitted when there's
> > no ambiguity.  This will allow us to keep compatibility with
> > existing libvirt versions.
> >
> > Test case included to ensure we don't break this again.
> >
> > Fixes: commit 176d2cda0dee ("i386/cpu: Consolidate die-id validity in smp context")
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Changes v1 -> v2:
> > * v1 was "pc: Don't make CPU properties mandatory unless necessary"
> > * Make only die-id optional (Igor Mammedov)
> > ---
> >  hw/i386/pc.c                             |  8 ++++++
> >  tests/acceptance/pc_cpu_hotplug_props.py | 35 ++++++++++++++++++++++++
> >  2 files changed, 43 insertions(+)
> >  create mode 100644 tests/acceptance/pc_cpu_hotplug_props.py
> >
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 3ab4bcb3ca..9c3f6ae828 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -2406,6 +2406,14 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> >          int max_socket = (ms->smp.max_cpus - 1) /
> >                                  smp_threads / smp_cores / pcms->smp_dies;
> >  
> > +        /*
> > +         * die-id was optional in QEMU 4.0 and older, so keep it optional
> > +         * if there's only one die per socket.
> > +         */
> 
> Is this comment worth its keep?

It's not obvious from the code why die-id is optional and all the
other properties are not.  The purpose of the comment is to
clarify that.

> 
> > +        if (cpu->die_id < 0 && pcms->smp_dies == 1) {
> > +            cpu->die_id = 0;
> > +        }
> > +
> >          if (cpu->socket_id < 0) {
> >              error_setg(errp, "CPU socket-id is not set");
> >              return;
> > diff --git a/tests/acceptance/pc_cpu_hotplug_props.py b/tests/acceptance/pc_cpu_hotplug_props.py
> > new file mode 100644
> > index 0000000000..08b7e632c6
> > --- /dev/null
> > +++ b/tests/acceptance/pc_cpu_hotplug_props.py
> > @@ -0,0 +1,35 @@
> > +#
> > +# Ensure CPU die-id can be omitted on -device
> > +#
> > +#  Copyright (c) 2019 Red Hat Inc
> > +#
> > +# Author:
> > +#  Eduardo Habkost <ehabkost@redhat.com>
> > +#
> > +# This library is free software; you can redistribute it and/or
> > +# modify it under the terms of the GNU Lesser General Public
> > +# License as published by the Free Software Foundation; either
> > +# version 2 of the License, or (at your option) any later version.
> > +#
> > +# This library is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +# Lesser General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU Lesser General Public
> > +# License along with this library; if not, see <http://www.gnu.org/licenses/>.
> > +#
> > +
> > +from avocado_qemu import Test
> > +
> > +class OmittedCPUProps(Test):
> > +    """
> > +    :avocado: tags=arch:x86_64
> > +    """
> > +    def test_no_die_id(self):
> > +        self.vm.add_args('-nodefaults', '-S')
> > +        self.vm.add_args('-smp', '1,sockets=2,cores=2,threads=2,maxcpus=8')
> > +        self.vm.add_args('-cpu', 'qemu64')
> > +        self.vm.add_args('-device', 'qemu64-x86_64-cpu,socket-id=1,core-id=0,thread-id=0')
> > +        self.vm.launch()
> > +        self.assertEquals(len(self.vm.command('query-cpus')), 2)
> 
> This tests intentionally violates the "a CPU hot-plug with -device must
> specify all the properties returned by query-hotpluggable-cpus" rule
> laid down in qapi/machine.json, and the change to pc.c permits it.
> 
> Missing: a suitable update to the rule.

Intentional, because I don't want decisions about documentation
to block the regression fix from being merged.

> 
> In addition, I'd like us to either work on making the rule stick in the
> future (see my reply to Igor for an idea), or ditch the rule.  But
> that's outside the scope of this regression fix.

I'd prefer to ditch the rule, or at least change it to be a
suggestion instead of a requirement.

-- 
Eduardo


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH v2] pc: Don't make die-id mandatory unless necessary
  2019-08-28 14:35   ` Eduardo Habkost
@ 2019-08-28 16:57     ` Igor Mammedov
  2019-08-29 13:12       ` Peter Krempa
  0 siblings, 1 reply; 7+ messages in thread
From: Igor Mammedov @ 2019-08-28 16:57 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: Peter Krempa, Vanderson Martins do Rosario, Like Xu,
	Erik Skultety, qemu-stable, Michael S. Tsirkin,
	Markus Armbruster, qemu-devel, qemu-s390x, qemu-ppc,
	Cornelia Huck, Paolo Bonzini, David Gibson, Richard Henderson

On Wed, 28 Aug 2019 11:35:24 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Aug 28, 2019 at 08:52:28AM +0200, Markus Armbruster wrote:
> > Eduardo Habkost <ehabkost@redhat.com> writes:
> >   
> > > We have this issue reported when using libvirt to hotplug CPUs:
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1741451
> > >
> > > Basically, libvirt is not copying die-id from
> > > query-hotpluggable-cpus, but die-id is now mandatory.
> > >
> > > We could blame libvirt and say it is not following the documented
> > > interface, because we have this buried in the QAPI schema
> > > documentation:
> > >  
> > >> Note: currently there are 5 properties that could be present
> > >> but management should be prepared to pass through other
> > >> properties with device_add command to allow for future
> > >> interface extension. This also requires the filed names to be kept in
> > >> sync with the properties passed to -device/device_add.  
> > >
> > > But I don't think this would be reasonable from us.  We can just
> > > make QEMU more flexible and let die-id to be omitted when there's
> > > no ambiguity.  This will allow us to keep compatibility with
> > > existing libvirt versions.
> > >
> > > Test case included to ensure we don't break this again.
> > >
> > > Fixes: commit 176d2cda0dee ("i386/cpu: Consolidate die-id validity in smp context")
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > > Changes v1 -> v2:
> > > * v1 was "pc: Don't make CPU properties mandatory unless necessary"
> > > * Make only die-id optional (Igor Mammedov)
> > > ---
> > >  hw/i386/pc.c                             |  8 ++++++
> > >  tests/acceptance/pc_cpu_hotplug_props.py | 35 ++++++++++++++++++++++++
> > >  2 files changed, 43 insertions(+)
> > >  create mode 100644 tests/acceptance/pc_cpu_hotplug_props.py
> > >
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index 3ab4bcb3ca..9c3f6ae828 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -2406,6 +2406,14 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> > >          int max_socket = (ms->smp.max_cpus - 1) /
> > >                                  smp_threads / smp_cores / pcms->smp_dies;
> > >  
> > > +        /*
> > > +         * die-id was optional in QEMU 4.0 and older, so keep it optional
> > > +         * if there's only one die per socket.
> > > +         */  
> > 
> > Is this comment worth its keep?  
> 
> It's not obvious from the code why die-id is optional and all the
> other properties are not.  The purpose of the comment is to
> clarify that.
> 
> >   
> > > +        if (cpu->die_id < 0 && pcms->smp_dies == 1) {
> > > +            cpu->die_id = 0;
> > > +        }
> > > +
> > >          if (cpu->socket_id < 0) {
> > >              error_setg(errp, "CPU socket-id is not set");
> > >              return;
> > > diff --git a/tests/acceptance/pc_cpu_hotplug_props.py b/tests/acceptance/pc_cpu_hotplug_props.py
> > > new file mode 100644
> > > index 0000000000..08b7e632c6
> > > --- /dev/null
> > > +++ b/tests/acceptance/pc_cpu_hotplug_props.py
> > > @@ -0,0 +1,35 @@
> > > +#
> > > +# Ensure CPU die-id can be omitted on -device
> > > +#
> > > +#  Copyright (c) 2019 Red Hat Inc
> > > +#
> > > +# Author:
> > > +#  Eduardo Habkost <ehabkost@redhat.com>
> > > +#
> > > +# This library is free software; you can redistribute it and/or
> > > +# modify it under the terms of the GNU Lesser General Public
> > > +# License as published by the Free Software Foundation; either
> > > +# version 2 of the License, or (at your option) any later version.
> > > +#
> > > +# This library is distributed in the hope that it will be useful,
> > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > > +# Lesser General Public License for more details.
> > > +#
> > > +# You should have received a copy of the GNU Lesser General Public
> > > +# License along with this library; if not, see <http://www.gnu.org/licenses/>.
> > > +#
> > > +
> > > +from avocado_qemu import Test
> > > +
> > > +class OmittedCPUProps(Test):
> > > +    """
> > > +    :avocado: tags=arch:x86_64
> > > +    """
> > > +    def test_no_die_id(self):
> > > +        self.vm.add_args('-nodefaults', '-S')
> > > +        self.vm.add_args('-smp', '1,sockets=2,cores=2,threads=2,maxcpus=8')
> > > +        self.vm.add_args('-cpu', 'qemu64')
> > > +        self.vm.add_args('-device', 'qemu64-x86_64-cpu,socket-id=1,core-id=0,thread-id=0')
> > > +        self.vm.launch()
> > > +        self.assertEquals(len(self.vm.command('query-cpus')), 2)  

test_plug_with_device_add_x86() is supposed to test device_add (currently broken),
so do we really need more heavy avocado tests,
how about adding this test to tests/cpu-plug-test.c
('make check' is used much more often by developers)

> > This tests intentionally violates the "a CPU hot-plug with -device must
> > specify all the properties returned by query-hotpluggable-cpus" rule
> > laid down in qapi/machine.json, and the change to pc.c permits it.
> > 
> > Missing: a suitable update to the rule.  
> 
> Intentional, because I don't want decisions about documentation
> to block the regression fix from being merged.
> 
> > 
> > In addition, I'd like us to either work on making the rule stick in the
> > future (see my reply to Igor for an idea), or ditch the rule.  But
> > that's outside the scope of this regression fix.  
> 
> I'd prefer to ditch the rule, or at least change it to be a
> suggestion instead of a requirement.
Perhaps someone reads docs and uses API as designed (libvirt is not the only user)

I'd prefer to allow implicit die-id in 4.1 and 'stable' as that
ship has already sailed and make it mandatory since 4.2 as it is
supposed to be (+opening bug on libvirt - hoping that API would
be fixed properly this time).


Another related to die-id series bug:
We should hide die-id from query-hotpluggable-cpus output
for 4.0 and older machine types as well, so it won't break
migration for users that implement interface as documented
as it won't be possible to start
 old-qemu-4.0 -device cpufoo,die-id=0,...
since that "-device cpufoo,die-id=0,..." were used on new-qemu source.

PS:
Adding affected targets maintainers to the loop to see if
we can drop restriction.

Even though it works fine for die-id and I don't see immediate problems
with relaxing rule, I reluctant to do it, since instead of simple
 "add all properties you were told to"
implicit rules would evolve into mess similar to smp_parse() over time.

Also if we would need to change implicit values logic down the road
it would be a pain like with any default parameters in QEMU, which is
a good reason against relaxing rule.



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH v2] pc: Don't make die-id mandatory unless necessary
  2019-08-28 16:57     ` Igor Mammedov
@ 2019-08-29 13:12       ` Peter Krempa
  2019-08-29 13:46         ` Peter Krempa
  0 siblings, 1 reply; 7+ messages in thread
From: Peter Krempa @ 2019-08-29 13:12 UTC (permalink / raw)
  To: Igor Mammedov
  Cc: Cornelia Huck, Vanderson Martins do Rosario, Eduardo Habkost,
	Like Xu, Erik Skultety, qemu-stable, Michael S. Tsirkin,
	qemu-devel, Markus Armbruster, qemu-s390x, qemu-ppc,
	Paolo Bonzini, David Gibson, Richard Henderson

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

On Wed, Aug 28, 2019 at 18:57:18 +0200, Igor Mammedov wrote:
> On Wed, 28 Aug 2019 11:35:24 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Wed, Aug 28, 2019 at 08:52:28AM +0200, Markus Armbruster wrote:
> > > Eduardo Habkost <ehabkost@redhat.com> writes:
> > >   
> > > > We have this issue reported when using libvirt to hotplug CPUs:
> > > > https://bugzilla.redhat.com/show_bug.cgi?id=1741451
> > > >
> > > > Basically, libvirt is not copying die-id from
> > > > query-hotpluggable-cpus, but die-id is now mandatory.
> > > >
> > > > We could blame libvirt and say it is not following the documented
> > > > interface, because we have this buried in the QAPI schema
> > > > documentation:
> > > >  
> > > >> Note: currently there are 5 properties that could be present
> > > >> but management should be prepared to pass through other
> > > >> properties with device_add command to allow for future
> > > >> interface extension. This also requires the filed names to be kept in
> > > >> sync with the properties passed to -device/device_add.  
> > > >
> > > > But I don't think this would be reasonable from us.  We can just
> > > > make QEMU more flexible and let die-id to be omitted when there's
> > > > no ambiguity.  This will allow us to keep compatibility with
> > > > existing libvirt versions.
> > > >
> > > > Test case included to ensure we don't break this again.
> > > >
> > > > Fixes: commit 176d2cda0dee ("i386/cpu: Consolidate die-id validity in smp context")
> > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > > ---
> > > > Changes v1 -> v2:
> > > > * v1 was "pc: Don't make CPU properties mandatory unless necessary"
> > > > * Make only die-id optional (Igor Mammedov)
> > > > ---
> > > >  hw/i386/pc.c                             |  8 ++++++
> > > >  tests/acceptance/pc_cpu_hotplug_props.py | 35 ++++++++++++++++++++++++
> > > >  2 files changed, 43 insertions(+)
> > > >  create mode 100644 tests/acceptance/pc_cpu_hotplug_props.py
> > > >
> > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > > index 3ab4bcb3ca..9c3f6ae828 100644
> > > > --- a/hw/i386/pc.c
> > > > +++ b/hw/i386/pc.c
> > > > @@ -2406,6 +2406,14 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> > > >          int max_socket = (ms->smp.max_cpus - 1) /
> > > >                                  smp_threads / smp_cores / pcms->smp_dies;
> > > >  
> > > > +        /*
> > > > +         * die-id was optional in QEMU 4.0 and older, so keep it optional
> > > > +         * if there's only one die per socket.
> > > > +         */  
> > > 
> > > Is this comment worth its keep?  
> > 
> > It's not obvious from the code why die-id is optional and all the
> > other properties are not.  The purpose of the comment is to
> > clarify that.
> > 
> > >   
> > > > +        if (cpu->die_id < 0 && pcms->smp_dies == 1) {
> > > > +            cpu->die_id = 0;
> > > > +        }
> > > > +
> > > >          if (cpu->socket_id < 0) {
> > > >              error_setg(errp, "CPU socket-id is not set");
> > > >              return;
> > > > diff --git a/tests/acceptance/pc_cpu_hotplug_props.py b/tests/acceptance/pc_cpu_hotplug_props.py
> > > > new file mode 100644
> > > > index 0000000000..08b7e632c6
> > > > --- /dev/null
> > > > +++ b/tests/acceptance/pc_cpu_hotplug_props.py
> > > > @@ -0,0 +1,35 @@
> > > > +#
> > > > +# Ensure CPU die-id can be omitted on -device
> > > > +#
> > > > +#  Copyright (c) 2019 Red Hat Inc
> > > > +#
> > > > +# Author:
> > > > +#  Eduardo Habkost <ehabkost@redhat.com>
> > > > +#
> > > > +# This library is free software; you can redistribute it and/or
> > > > +# modify it under the terms of the GNU Lesser General Public
> > > > +# License as published by the Free Software Foundation; either
> > > > +# version 2 of the License, or (at your option) any later version.
> > > > +#
> > > > +# This library is distributed in the hope that it will be useful,
> > > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > > > +# Lesser General Public License for more details.
> > > > +#
> > > > +# You should have received a copy of the GNU Lesser General Public
> > > > +# License along with this library; if not, see <http://www.gnu.org/licenses/>.
> > > > +#
> > > > +
> > > > +from avocado_qemu import Test
> > > > +
> > > > +class OmittedCPUProps(Test):
> > > > +    """
> > > > +    :avocado: tags=arch:x86_64
> > > > +    """
> > > > +    def test_no_die_id(self):
> > > > +        self.vm.add_args('-nodefaults', '-S')
> > > > +        self.vm.add_args('-smp', '1,sockets=2,cores=2,threads=2,maxcpus=8')
> > > > +        self.vm.add_args('-cpu', 'qemu64')
> > > > +        self.vm.add_args('-device', 'qemu64-x86_64-cpu,socket-id=1,core-id=0,thread-id=0')
> > > > +        self.vm.launch()
> > > > +        self.assertEquals(len(self.vm.command('query-cpus')), 2)  
> 
> test_plug_with_device_add_x86() is supposed to test device_add (currently broken),
> so do we really need more heavy avocado tests,
> how about adding this test to tests/cpu-plug-test.c
> ('make check' is used much more often by developers)
> 
> > > This tests intentionally violates the "a CPU hot-plug with -device must
> > > specify all the properties returned by query-hotpluggable-cpus" rule
> > > laid down in qapi/machine.json, and the change to pc.c permits it.
> > > 
> > > Missing: a suitable update to the rule.  
> > 
> > Intentional, because I don't want decisions about documentation
> > to block the regression fix from being merged.
> > 
> > > 
> > > In addition, I'd like us to either work on making the rule stick in the
> > > future (see my reply to Igor for an idea), or ditch the rule.  But
> > > that's outside the scope of this regression fix.  
> > 
> > I'd prefer to ditch the rule, or at least change it to be a
> > suggestion instead of a requirement.
> Perhaps someone reads docs and uses API as designed (libvirt is not the only user)
> 
> I'd prefer to allow implicit die-id in 4.1 and 'stable' as that
> ship has already sailed and make it mandatory since 4.2 as it is
> supposed to be (+opening bug on libvirt - hoping that API would
> be fixed properly this time).
> 
> 
> Another related to die-id series bug:
> We should hide die-id from query-hotpluggable-cpus output
> for 4.0 and older machine types as well, so it won't break
> migration for users that implement interface as documented
> as it won't be possible to start
>  old-qemu-4.0 -device cpufoo,die-id=0,...
> since that "-device cpufoo,die-id=0,..." were used on new-qemu source.
> 
> PS:
> Adding affected targets maintainers to the loop to see if
> we can drop restriction.
> 
> Even though it works fine for die-id and I don't see immediate problems
> with relaxing rule, I reluctant to do it, since instead of simple
>  "add all properties you were told to"
> implicit rules would evolve into mess similar to smp_parse() over time.
> 
> Also if we would need to change implicit values logic down the road
> it would be a pain like with any default parameters in QEMU, which is
> a good reason against relaxing rule.

Hi,

after a discussion with Igor I'm preparing patches which take the
'props' verbatim and use it for hotplug. It's a fairly simple change in
libvirt. Originally I've opted to reconstruct the props from internal
data since it was simpler as the values were already parsed into
internal structures anyways.

With some persuation we could even get it into libvirt during the freeze
so that with new libvirt the released stuff will not break any more.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH v2] pc: Don't make die-id mandatory unless necessary
  2019-08-29 13:12       ` Peter Krempa
@ 2019-08-29 13:46         ` Peter Krempa
  0 siblings, 0 replies; 7+ messages in thread
From: Peter Krempa @ 2019-08-29 13:46 UTC (permalink / raw)
  To: Igor Mammedov
  Cc: Cornelia Huck, Vanderson Martins do Rosario, Eduardo Habkost,
	Like Xu, Erik Skultety, qemu-stable, Michael S. Tsirkin,
	qemu-devel, Markus Armbruster, qemu-s390x, qemu-ppc,
	Paolo Bonzini, David Gibson, Richard Henderson

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

On Thu, Aug 29, 2019 at 15:12:33 +0200, Peter Krempa wrote:
> On Wed, Aug 28, 2019 at 18:57:18 +0200, Igor Mammedov wrote:
> > On Wed, 28 Aug 2019 11:35:24 -0300

[...]

> > Also if we would need to change implicit values logic down the road
> > it would be a pain like with any default parameters in QEMU, which is
> > a good reason against relaxing rule.
> 
> Hi,
> 
> after a discussion with Igor I'm preparing patches which take the
> 'props' verbatim and use it for hotplug. It's a fairly simple change in
> libvirt. Originally I've opted to reconstruct the props from internal
> data since it was simpler as the values were already parsed into
> internal structures anyways.
> 
> With some persuation we could even get it into libvirt during the freeze
> so that with new libvirt the released stuff will not break any more.

https://www.redhat.com/archives/libvir-list/2019-August/msg01322.html


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-16 17:07 [Qemu-devel] [PATCH v2] pc: Don't make die-id mandatory unless necessary Eduardo Habkost
2019-08-26 14:05 ` Igor Mammedov
2019-08-28  6:52 ` Markus Armbruster
2019-08-28 14:35   ` Eduardo Habkost
2019-08-28 16:57     ` Igor Mammedov
2019-08-29 13:12       ` Peter Krempa
2019-08-29 13:46         ` Peter Krempa

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git