[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [qemu-s390x] [Qemu-devel] [PATCH v2] pc: Don't make die-id mandatory
From: |
Igor Mammedov |
Subject: |
Re: [qemu-s390x] [Qemu-devel] [PATCH v2] pc: Don't make die-id mandatory unless necessary |
Date: |
Wed, 28 Aug 2019 18:57:18 +0200 |
On Wed, 28 Aug 2019 11:35:24 -0300
Eduardo Habkost <address@hidden> wrote:
> On Wed, Aug 28, 2019 at 08:52:28AM +0200, Markus Armbruster wrote:
> > Eduardo Habkost <address@hidden> 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 <address@hidden>
> > > ---
> > > 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 <address@hidden>
> > > +#
> > > +# 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.
- Re: [qemu-s390x] [Qemu-devel] [PATCH v2] pc: Don't make die-id mandatory unless necessary,
Igor Mammedov <=