[Qemu-devel] [PATCH 1/2] trace: Add "cpu_init" event

[Qemu-devel] [PATCH 1/2] trace: Add "cpu_init" event

[Lluís Vilanova]

Signals the creation of a new virtual (guest) CPU.

The event does not have the "vcpu" property because the machine creates
its initial vCPUs before late trace state initialization, making the
event "invisible" at system boot.

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 cpus.c       |    2 ++
 stubs/cpus.c |    2 ++
 trace-events |    8 ++++++++
 3 files changed, 12 insertions(+)

diff --git a/cpus.c b/cpus.c
index 84c3520..eaf4b2c 100644
--- a/cpus.c
+++ b/cpus.c
@@ -46,6 +46,7 @@
 #include "qapi-event.h"
 #include "hw/nmi.h"
 #include "sysemu/replay.h"
+#include "trace.h"
 
 #ifndef _WIN32
 #include "qemu/compatfd.h"
@@ -1448,6 +1449,7 @@ void qemu_init_vcpu(CPUState *cpu)
     } else {
         qemu_dummy_start_vcpu(cpu);
     }
+    trace_guest_cpu_init(cpu);
 }
 
 void cpu_stop_current(void)
diff --git a/stubs/cpus.c b/stubs/cpus.c
index e192722..a797be1 100644
--- a/stubs/cpus.c
+++ b/stubs/cpus.c
@@ -1,6 +1,7 @@
 #include "qemu/osdep.h"
 #include "qemu-common.h"
 #include "qom/cpu.h"
+#include "trace.h"
 
 void cpu_resume(CPUState *cpu)
 {
@@ -8,4 +9,5 @@ void cpu_resume(CPUState *cpu)
 
 void qemu_init_vcpu(CPUState *cpu)
 {
+    trace_guest_cpu_init(cpu);
 }
diff --git a/trace-events b/trace-events
index 616cc52..5715826 100644
--- a/trace-events
+++ b/trace-events
@@ -142,6 +142,14 @@ memory_region_tb_write(int cpu_index, uint64_t addr, uint64_t value, unsigned si
 
 ### Guest events, keep at bottom
 
+
+## vCPU
+
+# Create a new virtual (guest) CPU
+#
+# Targets: all
+guest_cpu_init(void *cpu) "cpu=%p"
+
 # @vaddr: Access' virtual address.
 # @info : Access' information (see below).
 #

[Qemu-devel] [PATCH 2/2] trace: Add "cpu_reset" event

[Lluís Vilanova]

Signals the reset of the state a virtual (guest) CPU.

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 qom/cpu.c    |    3 +++
 trace-events |    5 +++++
 2 files changed, 8 insertions(+)

diff --git a/qom/cpu.c b/qom/cpu.c
index 2553247..47bd807 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -29,6 +29,7 @@
 #include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
 #include "hw/qdev-properties.h"
+#include "trace.h"
 
 bool cpu_exists(int64_t id)
 {
@@ -245,6 +246,8 @@ void cpu_reset(CPUState *cpu)
     if (klass->reset != NULL) {
         (*klass->reset)(cpu);
     }
+
+    trace_guest_cpu_reset(cpu);
 }
 
 static void cpu_common_reset(CPUState *cpu)
diff --git a/trace-events b/trace-events
index 5715826..2780bc8 100644
--- a/trace-events
+++ b/trace-events
@@ -150,6 +150,11 @@ memory_region_tb_write(int cpu_index, uint64_t addr, uint64_t value, unsigned si
 # Targets: all
 guest_cpu_init(void *cpu) "cpu=%p"
 
+# Reset the state of a virtual (guest) CPU
+#
+# Targets: all
+vcpu guest_cpu_reset(void)
+
 # @vaddr: Access' virtual address.
 # @info : Access' information (see below).
 #

Re: [Qemu-devel] [PATCH 1/2] trace: Add "cpu_init" event

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 326 bytes --]

On Tue, Sep 06, 2016 at 04:25:53PM +0200, Lluís Vilanova wrote:
> +## vCPU
> +
> +# Create a new virtual (guest) CPU
> +#
> +# Targets: all
> +guest_cpu_init(void *cpu) "cpu=%p"

This isn't a vcpu trace event.  Please add keep it with the other
non-vcpu trace events:

# cpus.c
guest_cpu_init(void *cpu) "cpu=%p"

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 1/2] trace: Add "cpu_init" event

[Lluís Vilanova]

Stefan Hajnoczi writes:

> On Tue, Sep 06, 2016 at 04:25:53PM +0200, Lluís Vilanova wrote:
>> +## vCPU
>> +
>> +# Create a new virtual (guest) CPU
>> +#
>> +# Targets: all
>> +guest_cpu_init(void *cpu) "cpu=%p"

> This isn't a vcpu trace event.  Please add keep it with the other
> non-vcpu trace events:

> # cpus.c
> guest_cpu_init(void *cpu) "cpu=%p"

It actually is, but as the commit message says, declaring it as such prevents
the event to be emitted.

The culprit of this problem is that new vCPUs start with an empty per-vCPU trace
event set. Should we make vCPUs "inherit" the state from the global state?
(i.e., if any vcpu event is set on any vCPU, set it on the new one). The next
question would then be, should this inheritance only apply until tracing is
fully initialized of for the whole duration of QEMU?

Cheers,
  Lluis

Re: [Qemu-devel] [PATCH 1/2] trace: Add "cpu_init" event

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1410 bytes --]

On Wed, Sep 14, 2016 at 06:01:17PM +0200, Lluís Vilanova wrote:
> Stefan Hajnoczi writes:
> 
> > On Tue, Sep 06, 2016 at 04:25:53PM +0200, Lluís Vilanova wrote:
> >> +## vCPU
> >> +
> >> +# Create a new virtual (guest) CPU
> >> +#
> >> +# Targets: all
> >> +guest_cpu_init(void *cpu) "cpu=%p"
> 
> > This isn't a vcpu trace event.  Please add keep it with the other
> > non-vcpu trace events:
> 
> > # cpus.c
> > guest_cpu_init(void *cpu) "cpu=%p"
> 
> It actually is, but as the commit message says, declaring it as such prevents
> the event to be emitted.

If it cannot be toggled on a per-vcpu basis then claiming it's a vcpu
event doesn't make sense.

> The culprit of this problem is that new vCPUs start with an empty per-vCPU trace
> event set. Should we make vCPUs "inherit" the state from the global state?
> (i.e., if any vcpu event is set on any vCPU, set it on the new one). The next
> question would then be, should this inheritance only apply until tracing is
> fully initialized of for the whole duration of QEMU?

I think the underlying issue is that trace_init_vcpu_events() assumes
there is a single instant where vcpus all exist and need to be
initialized.

A model that supports vcpu hotplug is really needed.  In that model the
global dstate should be the "global" state that determines whether vcpus
are initialized with the event enabled or disabled.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 1/2] trace: Add "cpu_init" event

[Lluís Vilanova]

Stefan Hajnoczi writes:

> On Wed, Sep 14, 2016 at 06:01:17PM +0200, Lluís Vilanova wrote:
>> Stefan Hajnoczi writes:
>> 
>> > On Tue, Sep 06, 2016 at 04:25:53PM +0200, Lluís Vilanova wrote:
>> >> +## vCPU
>> >> +
>> >> +# Create a new virtual (guest) CPU
>> >> +#
>> >> +# Targets: all
>> >> +guest_cpu_init(void *cpu) "cpu=%p"
>> 
>> > This isn't a vcpu trace event.  Please add keep it with the other
>> > non-vcpu trace events:
>> 
>> > # cpus.c
>> > guest_cpu_init(void *cpu) "cpu=%p"
>> 
>> It actually is, but as the commit message says, declaring it as such prevents
>> the event to be emitted.

> If it cannot be toggled on a per-vcpu basis then claiming it's a vcpu
> event doesn't make sense.

>> The culprit of this problem is that new vCPUs start with an empty per-vCPU trace
>> event set. Should we make vCPUs "inherit" the state from the global state?
>> (i.e., if any vcpu event is set on any vCPU, set it on the new one). The next
>> question would then be, should this inheritance only apply until tracing is
>> fully initialized of for the whole duration of QEMU?

> I think the underlying issue is that trace_init_vcpu_events() assumes
> there is a single instant where vcpus all exist and need to be
> initialized.

> A model that supports vcpu hotplug is really needed.  In that model the
> global dstate should be the "global" state that determines whether vcpus
> are initialized with the event enabled or disabled.

Ok, so then you're going for what I was saying. "Inheriting" the state of every
new vCPU from the global dynamic state. If *any* vCPU has an event enabled, all
new vCPUs will too.

As I said, next question is whether inheritance should apply only until full
initialization, or the whole time. I thing the latter is less confusing.


Cheers,
  Lluis

Re: [Qemu-devel] [PATCH 1/2] trace: Add "cpu_init" event

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2333 bytes --]

On Thu, Sep 15, 2016 at 02:51:59PM +0200, Lluís Vilanova wrote:
> Stefan Hajnoczi writes:
> 
> > On Wed, Sep 14, 2016 at 06:01:17PM +0200, Lluís Vilanova wrote:
> >> Stefan Hajnoczi writes:
> >> 
> >> > On Tue, Sep 06, 2016 at 04:25:53PM +0200, Lluís Vilanova wrote:
> >> >> +## vCPU
> >> >> +
> >> >> +# Create a new virtual (guest) CPU
> >> >> +#
> >> >> +# Targets: all
> >> >> +guest_cpu_init(void *cpu) "cpu=%p"
> >> 
> >> > This isn't a vcpu trace event.  Please add keep it with the other
> >> > non-vcpu trace events:
> >> 
> >> > # cpus.c
> >> > guest_cpu_init(void *cpu) "cpu=%p"
> >> 
> >> It actually is, but as the commit message says, declaring it as such prevents
> >> the event to be emitted.
> 
> > If it cannot be toggled on a per-vcpu basis then claiming it's a vcpu
> > event doesn't make sense.
> 
> >> The culprit of this problem is that new vCPUs start with an empty per-vCPU trace
> >> event set. Should we make vCPUs "inherit" the state from the global state?
> >> (i.e., if any vcpu event is set on any vCPU, set it on the new one). The next
> >> question would then be, should this inheritance only apply until tracing is
> >> fully initialized of for the whole duration of QEMU?
> 
> > I think the underlying issue is that trace_init_vcpu_events() assumes
> > there is a single instant where vcpus all exist and need to be
> > initialized.
> 
> > A model that supports vcpu hotplug is really needed.  In that model the
> > global dstate should be the "global" state that determines whether vcpus
> > are initialized with the event enabled or disabled.
> 
> Ok, so then you're going for what I was saying. "Inheriting" the state of every
> new vCPU from the global dynamic state. If *any* vCPU has an event enabled, all
> new vCPUs will too.

No, I was thinking of something slightly different:

Right now vcpu trace events use the global dstate during intialization
only.  After trace_init_vcpu_events() their dstate is zeroed.

What if we leave the global dstate and use it as the template for
enabling events in new vCPUs (hotplugged or static)?

> As I said, next question is whether inheritance should apply only until full
> initialization, or the whole time. I thing the latter is less confusing.

I agree that it should apply the whole time.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 1/2] trace: Add "cpu_init" event

[Lluís Vilanova]

Stefan Hajnoczi writes:

> On Thu, Sep 15, 2016 at 02:51:59PM +0200, Lluís Vilanova wrote:
>> Stefan Hajnoczi writes:
>> 
>> > On Wed, Sep 14, 2016 at 06:01:17PM +0200, Lluís Vilanova wrote:
>> >> Stefan Hajnoczi writes:
>> >> 
>> >> > On Tue, Sep 06, 2016 at 04:25:53PM +0200, Lluís Vilanova wrote:
>> >> >> +## vCPU
>> >> >> +
>> >> >> +# Create a new virtual (guest) CPU
>> >> >> +#
>> >> >> +# Targets: all
>> >> >> +guest_cpu_init(void *cpu) "cpu=%p"
>> >> 
>> >> > This isn't a vcpu trace event.  Please add keep it with the other
>> >> > non-vcpu trace events:
>> >> 
>> >> > # cpus.c
>> >> > guest_cpu_init(void *cpu) "cpu=%p"
>> >> 
>> >> It actually is, but as the commit message says, declaring it as such prevents
>> >> the event to be emitted.
>> 
>> > If it cannot be toggled on a per-vcpu basis then claiming it's a vcpu
>> > event doesn't make sense.
>> 
>> >> The culprit of this problem is that new vCPUs start with an empty per-vCPU trace
>> >> event set. Should we make vCPUs "inherit" the state from the global state?
>> >> (i.e., if any vcpu event is set on any vCPU, set it on the new one). The next
>> >> question would then be, should this inheritance only apply until tracing is
>> >> fully initialized of for the whole duration of QEMU?
>> 
>> > I think the underlying issue is that trace_init_vcpu_events() assumes
>> > there is a single instant where vcpus all exist and need to be
>> > initialized.
>> 
>> > A model that supports vcpu hotplug is really needed.  In that model the
>> > global dstate should be the "global" state that determines whether vcpus
>> > are initialized with the event enabled or disabled.
>> 
>> Ok, so then you're going for what I was saying. "Inheriting" the state of every
>> new vCPU from the global dynamic state. If *any* vCPU has an event enabled, all
>> new vCPUs will too.

> No, I was thinking of something slightly different:

> Right now vcpu trace events use the global dstate during intialization
> only.  After trace_init_vcpu_events() their dstate is zeroed.

> What if we leave the global dstate and use it as the template for
> enabling events in new vCPUs (hotplugged or static)?

I think we both mean the same. The global dstate is not actually zeroed after
trace_init_vcpu_events(). It is zeroed and *then*, iff it was set, set again for
that vCPU. The end result is that the global dstate (for vcpu events) counts how
many vCPUs have that event enabled.

What I mean by "inheriting if any vCPU has the event enabled", is actually check
if that counter is greater than zero. If so, then enable it for the new vCPU.

The only corner case is moving the logic in trace_init_vcpu_events() (zeroing
and setting again if it was set) into vCPU creation iff we're the first vCPU
created on the system.


>> As I said, next question is whether inheritance should apply only until full
>> initialization, or the whole time. I thing the latter is less confusing.

> I agree that it should apply the whole time.

Right. I just wrote the hotplugging path, but could not find where is the path
for hot-unplugging, if it exists.

The wiki [1] seems to point that unplugging is not yet supported, but it might
be outdated. Can anybody shed some light onto this?

[1] http://wiki.qemu.org/Features/CPUHotplug


Thanks,
  Lluis