Patchwork [RFC,8/8] qom: make CPU a child of DeviceState

login
register
mail settings
Submitter Eduardo Habkost
Date Dec. 4, 2012, 1:19 p.m.
Message ID <1354627180-25704-9-git-send-email-ehabkost@redhat.com>
Download mbox | patch
Permalink /patch/203623/
State New
Headers show

Comments

Eduardo Habkost - Dec. 4, 2012, 1:19 p.m.
From: Igor Mammedov <imammedo@redhat.com>

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
[ehabkost: change CPU type declaration to hae TYPE_DEVICE as parent]
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Yes, there is "changelog" data before the "---" mark, but I believe that
in this case they are important to indicate authorship and the scope of
the Signed-off-by lines (so they need to get into the git commit
message).
---
 include/qemu/cpu.h | 6 +++---
 qom/cpu.c          | 3 ++-
 2 files changed, 5 insertions(+), 4 deletions(-)
Andreas Färber - Dec. 5, 2012, 2:48 p.m.
Am 04.12.2012 14:19, schrieb Eduardo Habkost:
> From: Igor Mammedov <imammedo@redhat.com>
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> [ehabkost: change CPU type declaration to hae TYPE_DEVICE as parent]
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Yes, there is "changelog" data before the "---" mark, but I believe that
> in this case they are important to indicate authorship and the scope of
> the Signed-off-by lines (so they need to get into the git commit
> message).
> ---
>  include/qemu/cpu.h | 6 +++---
>  qom/cpu.c          | 3 ++-
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/include/qemu/cpu.h b/include/qemu/cpu.h
> index 61b7698..bc004fd 100644
> --- a/include/qemu/cpu.h
> +++ b/include/qemu/cpu.h
> @@ -20,7 +20,7 @@
>  #ifndef QEMU_CPU_H
>  #define QEMU_CPU_H
>  
> -#include "qemu/object.h"
> +#include "hw/qdev-core.h"
>  #include "qemu-thread.h"
>  
>  /**
> @@ -46,7 +46,7 @@ typedef struct CPUState CPUState;
>   */
>  typedef struct CPUClass {
>      /*< private >*/
> -    ObjectClass parent_class;
> +    DeviceClass parent_class;
>      /*< public >*/
>  
>      void (*reset)(CPUState *cpu);
> @@ -62,7 +62,7 @@ typedef struct CPUClass {
>   */
>  struct CPUState {
>      /*< private >*/
> -    Object parent_obj;
> +    DeviceState parent_obj;
>      /*< public >*/
>  
>      struct QemuThread *thread;
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 5b36046..f59db7d 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -20,6 +20,7 @@
>  
>  #include "qemu/cpu.h"
>  #include "qemu-common.h"
> +#include "hw/qdev-core.h"
>  
>  void cpu_reset(CPUState *cpu)
>  {
> @@ -43,7 +44,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
>  
>  static TypeInfo cpu_type_info = {
>      .name = TYPE_CPU,
> -    .parent = TYPE_OBJECT,
> +    .parent = TYPE_DEVICE,
>      .instance_size = sizeof(CPUState),
>      .abstract = true,
>      .class_size = sizeof(CPUClass),

This makes the CPU a device, allowing the user to specify it with
-device. My preference would be to disable that at first[1] by setting
DeviceClass::no_user = 1.

Have you tested what happens if someone tries to hotplug a CPU device?
It may be the first device without bus...

[1] Anthony's and my idea was to handle hotplug at a higher level than
CPUState - X86Socket containing X86Core containing X86Thread or so. This
would require me (or someone) to refactor CPU_COMMON's numa_node (also
used in sPAPR), nr_cores, nr_threads (also used in mips/Malta) - in a
non-trivial way. We may need to go from CPU*State to CPUState (possible
so far) to Core to Socket, for which object_get_parent() would be
helpful. So far Object::parent is declared private.
Are we targetting to do this is two steps, using CPUState at first? Or
has one of you been investigating how involved this redesign would be?

Regards,
Andreas
Eduardo Habkost - Dec. 5, 2012, 3:52 p.m.
On Wed, Dec 05, 2012 at 03:48:10PM +0100, Andreas Färber wrote:
> Am 04.12.2012 14:19, schrieb Eduardo Habkost:
[...]
> > @@ -43,7 +44,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
> >  
> >  static TypeInfo cpu_type_info = {
> >      .name = TYPE_CPU,
> > -    .parent = TYPE_OBJECT,
> > +    .parent = TYPE_DEVICE,
> >      .instance_size = sizeof(CPUState),
> >      .abstract = true,
> >      .class_size = sizeof(CPUClass),
> 
> This makes the CPU a device, allowing the user to specify it with
> -device. My preference would be to disable that at first[1] by setting
> DeviceClass::no_user = 1.

I didn't know no_user existed. It makes sense to set it by now, yes.

> 
> Have you tested what happens if someone tries to hotplug a CPU device?
> It may be the first device without bus...

I don't know, but I won't be surprised if stuff breaks horribly.

> 
> [1] Anthony's and my idea was to handle hotplug at a higher level than
> CPUState - X86Socket containing X86Core containing X86Thread or so.

Yes, and I agree with this approach.

> This
> would require me (or someone) to refactor CPU_COMMON's numa_node (also
> used in sPAPR), nr_cores, nr_threads (also used in mips/Malta) - in a
> non-trivial way. We may need to go from CPU*State to CPUState (possible
> so far) to Core to Socket, for which object_get_parent() would be
> helpful. So far Object::parent is declared private.

Why, exactly, the CPUX86State, CPUThread, or CPUCore objects would need
to access their parents directly? We could just make sure that the
parent provide the necessary information to its children, when creating
them.

The other side of that question is: why exactly we don't allow an object
to know its parent, by design? What's the right mechanism to be used
when a device really needs to send some signal to its parent?


> Are we targetting to do this is two steps, using CPUState at first? Or
> has one of you been investigating how involved this redesign would be?

What do you mean by "using CPUState at first"? I mean, I think we'll
always use CPUState in the code that represents a single VCPU/thread,
and it is not going away soon. The name "CPUState" may be a bit
confusing, though, as it contains the state of a single VCPU/thread (not
a CPU socket).

If you are asking about "qdevifying/subclassing CPUState first", I think
the answer is: yes, making it in two steps sounds better. If we use
no_user, we can more easily change the hierarchy later because there
would be nobody using "-device" to create CPUs yet. And if we have to
design and redo a whole socket/core/thread CPU class hierarchy first, I
don't expect us to finish doing this before QEMU 1.5[1].


When implementing an socket-based interface, I think we may end up with
something like:

- CPUSocket (or CPUPackage)
  - creates multiple CPUCore children

- CPUCore
  - creates multiple CPUThread children
  - maybe have CPUState children directly, to make things simpler

- CPUThread
  - creates one CPUState child
  - (maybe CPUState can be used directly)

- CPUState
  - the class we already have today
  - could be renamed to VCPUState or ThreadState, to make it clearer
  - CPUID data is handled here
  - CPU feature configuration is handled here
  - Will have one subclass for each CPU model


Some variations/alternatives I see:

* Just having two levels. e.g.:

- CPUSocket (or CPUPackage)
  - creates multiple CPUState children, depending on nr_cores/nr_threads
    configuration

- CPUState
  - the class we already have today
  - could be renamed to VCPUState or ThreadState, to make it clearer
  - CPUID data is handled here
  - CPU feature configuration is handled here
  - Will have one subclass for each CPU model


* Having CPU model subclasses at the CPUSocket level instead of
  CPUState level
  - It would make the CPUID code really messy: some CPUID bits would
    come from the CPUSocket subclass, other from the CPUState itself.
  - On the other hand, the external interface may make more sense
    if we do it that way. I mean: "create a 4-core SandyBridge CPU
    [package]" sounds more logical than "create a CPU package with 4
    SandyBridge threads inside it".


[1] Just to explain my expectations: what I _really_ want to have on
    QEMU 1.4 is:
    - A good machine-type compatibility mechanism that allows us to
      update CPU model definitions while keeping compatibility
      - Fortunately, the current global-variable-based approach kind-of
        works
      - But CPU subclasses/properties would give us a cleaner solution
        for free (as we could simply use machine-type global properties)
    - Any interface that libvirt can use to query for CPU model
      information, including:
      - Listing available CPU models
        - This exists, but it doesn't provide much detail
      - Checking which features are going to be enabled by each CPU model
      - Checking which features can really be enabled on a host
        (considering QEMU + kernel + hardware capabilities)
      - CPU subclasses/properties give us (most of) the above for free

  I wouldn't want to delay the CPU subclass/property work because of a
  CPU socket/core/thread modelling rework, as it would means delaying
  (again) the interfaces that libvirt really needs.

Patch

diff --git a/include/qemu/cpu.h b/include/qemu/cpu.h
index 61b7698..bc004fd 100644
--- a/include/qemu/cpu.h
+++ b/include/qemu/cpu.h
@@ -20,7 +20,7 @@ 
 #ifndef QEMU_CPU_H
 #define QEMU_CPU_H
 
-#include "qemu/object.h"
+#include "hw/qdev-core.h"
 #include "qemu-thread.h"
 
 /**
@@ -46,7 +46,7 @@  typedef struct CPUState CPUState;
  */
 typedef struct CPUClass {
     /*< private >*/
-    ObjectClass parent_class;
+    DeviceClass parent_class;
     /*< public >*/
 
     void (*reset)(CPUState *cpu);
@@ -62,7 +62,7 @@  typedef struct CPUClass {
  */
 struct CPUState {
     /*< private >*/
-    Object parent_obj;
+    DeviceState parent_obj;
     /*< public >*/
 
     struct QemuThread *thread;
diff --git a/qom/cpu.c b/qom/cpu.c
index 5b36046..f59db7d 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -20,6 +20,7 @@ 
 
 #include "qemu/cpu.h"
 #include "qemu-common.h"
+#include "hw/qdev-core.h"
 
 void cpu_reset(CPUState *cpu)
 {
@@ -43,7 +44,7 @@  static void cpu_class_init(ObjectClass *klass, void *data)
 
 static TypeInfo cpu_type_info = {
     .name = TYPE_CPU,
-    .parent = TYPE_OBJECT,
+    .parent = TYPE_DEVICE,
     .instance_size = sizeof(CPUState),
     .abstract = true,
     .class_size = sizeof(CPUClass),