On Wed, Aug 28, 2019 at 08:52:28AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabk...@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 <ehabk...@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 <ehabk...@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
