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 <bhar...@linux.vnet.ibm.com>
> >>> ---
> >>> 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 <bhar...@linux.vnet.ibm.com>
> >>> + *
> >>> + * 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
