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

[Andreas Färber]

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.

It would've been nice had you joined our KVM call prompted by _your_
suggestions (which again you could've discussed at KVM Forum where I had
a session specifically for discussing this topic), but you didn't.
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, 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.

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

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)