Re: [Qemu-devel] [RFC PATCH v0 4/8] spapr: Introduce CPU core device

[David Gibson]

On Mon, Feb 22, 2016 at 04:58:46PM +0100, Andreas Färber wrote:
> Am 22.02.2016 um 08:47 schrieb David Gibson:
> > On Mon, Feb 22, 2016 at 07:44:40AM +0100, Andreas Färber wrote:
> >> Am 22.02.2016 um 06:01 schrieb Bharata B Rao:
> >>> sPAPR CPU core device is a container of CPU thread devices. CPU hotplug is
> >>> performed in the granularity of CPU core device by setting the "realized"
> >>> property of this device to "true". When hotplugged, CPU core creates CPU
> >>> thread devices.
> >>>
> >>> TODO: Right now allows for only homogeneous configurations as we depend
> >>> on global smp_threads and machine->cpu_model.
> >>>
> >>> Signed-off-by: Bharata B Rao <address@hidden>
> >>> ---
> >>>  hw/ppc/Makefile.objs               |  1 +
> >>>  hw/ppc/spapr_cpu_package.c         | 50 
> >>> ++++++++++++++++++++++++++++++++++++++
> >>>  include/hw/ppc/spapr_cpu_package.h | 26 ++++++++++++++++++++
> >>>  3 files changed, 77 insertions(+)
> >>>  create mode 100644 hw/ppc/spapr_cpu_package.c
> >>>  create mode 100644 include/hw/ppc/spapr_cpu_package.h
> >>>
> >>> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> >>> index c1ffc77..3000982 100644
> >>> --- a/hw/ppc/Makefile.objs
> >>> +++ b/hw/ppc/Makefile.objs
> >>> @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
> >>>  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
> >>>  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> >>>  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> >>> +obj-$(CONFIG_PSERIES) += spapr_cpu_package.o
> >>>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> >>>  obj-y += spapr_pci_vfio.o
> >>>  endif
> >>> diff --git a/hw/ppc/spapr_cpu_package.c b/hw/ppc/spapr_cpu_package.c
> >>> new file mode 100644
> >>> index 0000000..3120a16
> >>> --- /dev/null
> >>> +++ b/hw/ppc/spapr_cpu_package.c
> >>> @@ -0,0 +1,50 @@
> >>> +/*
> >>> + * sPAPR CPU package device, acts as container of CPU thread devices.
> >>> + *
> >>> + * Copyright (C) 2016 Bharata B Rao <address@hidden>
> >>> + *
> >>> + * This work is licensed under the terms of the GNU GPL, version 2 or 
> >>> later.
> >>> + * See the COPYING file in the top-level directory.
> >>> + */
> >>> +#include "hw/cpu/package.h"
> >>> +#include "hw/ppc/spapr_cpu_package.h"
> >>> +#include "hw/boards.h"
> >>> +#include <sysemu/cpus.h>
> >>> +#include "qemu/error-report.h"
> >>> +
> >>> +static void spapr_cpu_package_instance_init(Object *obj)
> >>> +{
> >>> +    int i;
> >>> +    CPUState *cpu;
> >>> +    MachineState *machine = MACHINE(qdev_get_machine());
> >>> +    sPAPRCPUPackage *package = SPAPR_CPU_PACKAGE(obj);
> >>> +
> >>> +    /* Create as many CPU threads as specified in the topology */
> >>> +    for (i = 0; i < smp_threads; i++) {
> >>> +        cpu = cpu_generic_init(machine->cpu_type, machine->cpu_model);
> >>
> >> No, no, no. This is horribly violating QOM design.
> > 
> > Ok.. why?  There does not, to me, seem to be any remotely easily
> > discoverable means of finding out what QOM design principles are.
> > 
> > It also would have been nice if you weighed in on my RFC this code is
> > based on.

So, first, apologies for my grumpy tone.  This is just one of a number
of frustrations I've had in the last few days, putting me in an
uncharitable frame of mind.

> It would've been nice had you joined our KVM call prompted by _your_
> suggestions

Ok, a) I was not aware that this call was prompted by my suggestions.
I got some vague second hand notices about it; as I do about KVM calls
from time to time, which I ignored, as usual, because b) the KVM call
is at stupid o'clock for me.

> (which again you could've discussed at KVM Forum where I had
> a session specifically for discussing this topic), but you didn't.

At KVM Forum, cpu hotplug wasn't yet on my radar.

> I did discuss several aspects on the call and in the recorded session
> and will not write it by email again. Feel free to put it on the call
> agenda again.
> 
> I told Bharata that you can't have a base type that suddenly mutates its
> topology,

Ok, I'm not entirely sure what you mean by that.  Do you mean the fact
that the core object constructs its own sub-objects?

> therefore we discussed the possibility of having a QOM
> _interface_, not a base type as apparently done here. We deliberately do
> not have multi-inheritence in QOM; there's not just ppc in the world.

So, in other discussions I've realised the package needs to be an
interface, rather than a type.  But the patch your objecting to here
implements the spapr-core subtype.  If that were implementing rather
than inheriting "cpu-package" I don't see that it would really change
the logic here.

Like you (I think) I do dislike the use of machine->cpu_type and
machine->cpu_model.  I just haven't figured out enough QOM to know how
to avoid them.  What is the QOM equivalent of, essentially, arguments
to a constructor?

> My time for reading list mails is limited. Make it easy for me to
> understand what the difference is between your package and my core and
> why instead of reusing my posted cpu-core type you need to propose your

Because my time for reading the list is also limited, so I haven't
seen your cpu-core posting.  I did see Bharata's earlier core based
stuff which didn't seem to go over well either.

> own cpu-package type. In one point you are right, we keep going in
> circles and sometimes backwards rather than forward here.

But, also, from what I can tell of those parts of the discussion I
have seen, locking the hotplug at core level doesn't seem to work.  It
seems to create an awful backwards compatibility tangle for x86 which
has existing thread-level hotplug, and I expect it would cause
problems when we encounter a platform with socket level hotplug only
(this seems very plausible for something modelling physical hotplug).

The idea of cpu-package was to abstract away the granularity of cpu
hotplug from a fixed level of the socket/core/thread heirarchy, while
still making that granularity easy to discover for management.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature