| Message ID | 147317195306.24754.2911961107337506103.stgit@fimbulvetr.bsc.es |
|---|---|
| State | New |
| Headers | show |
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"
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
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.
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
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.
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
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). #
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(+)