* [Qemu-devel] [PATCH] spapr: reset @kvm_vcpu_dirty before starting CPU
@ 2014-01-15 4:57 Alexey Kardashevskiy
2014-01-15 22:47 ` Alexey Kardashevskiy
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Alexey Kardashevskiy @ 2014-01-15 4:57 UTC (permalink / raw)
To: qemu-devel
Cc: Alexey Kardashevskiy, Paolo Bonzini, qemu-ppc,
Andreas Färber, Alexander Graf
Normally QEMU kvm_arch_get_registers() reads registers and sets a dirty
flag which prevents further registers reading from KVM till
kvm_arch_put_registers() executes and resets the flag.
However if we run QEMU with "-S" ("suspended"), then execute "info cpus"
from the QEMU monitor, we end up with not reading registers in
rtas_start_cpu() as qmp_query_cpus() calls kvm_cpu_synchronize_state()
which leaves @kvm_vcpu_dirty=true what prevents kvm_cpu_synchronize_state()
from synchronizing registers and we loose the values.
This resets @kvm_vcpu_dirty flag as we do not have content which we really want to
keep at this point as the CPU is halted.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
We could also reset @kvm_vcpu_dirty in qmp_query_cpus() but that would be
racy.
---
hw/ppc/spapr_rtas.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 1cb276d..3dade5e 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -185,6 +185,8 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPREnvironment *spapr,
/* This will make sure qemu state is up to date with kvm, and
* mark it dirty so our changes get flushed back before the
* new cpu enters */
+
+ cs->kvm_vcpu_dirty = false;
kvm_cpu_synchronize_state(cs);
env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
--
1.8.4.rc4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] spapr: reset @kvm_vcpu_dirty before starting CPU
2014-01-15 4:57 [Qemu-devel] [PATCH] spapr: reset @kvm_vcpu_dirty before starting CPU Alexey Kardashevskiy
@ 2014-01-15 22:47 ` Alexey Kardashevskiy
2014-01-20 14:45 ` Alexander Graf
2014-01-20 16:20 ` Andreas Färber
2 siblings, 0 replies; 5+ messages in thread
From: Alexey Kardashevskiy @ 2014-01-15 22:47 UTC (permalink / raw)
To: qemu-devel
Cc: Alexey Kardashevskiy, Paolo Bonzini, qemu-ppc,
Andreas Färber, Alexander Graf
On 01/15/2014 03:57 PM, Alexey Kardashevskiy wrote:
> Normally QEMU kvm_arch_get_registers() reads registers and sets a dirty
> flag which prevents further registers reading from KVM till
> kvm_arch_put_registers() executes and resets the flag.
>
> However if we run QEMU with "-S" ("suspended"), then execute "info cpus"
> from the QEMU monitor, we end up with not reading registers in
> rtas_start_cpu() as qmp_query_cpus() calls kvm_cpu_synchronize_state()
> which leaves @kvm_vcpu_dirty=true what prevents kvm_cpu_synchronize_state()
> from synchronizing registers and we loose the values.
>
> This resets @kvm_vcpu_dirty flag as we do not have content which we really want to
> keep at this point as the CPU is halted.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
Alex, ping. Is it complicated or controversial patch? ;)
>
> We could also reset @kvm_vcpu_dirty in qmp_query_cpus() but that would be
> racy.
> ---
> hw/ppc/spapr_rtas.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 1cb276d..3dade5e 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -185,6 +185,8 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPREnvironment *spapr,
> /* This will make sure qemu state is up to date with kvm, and
> * mark it dirty so our changes get flushed back before the
> * new cpu enters */
> +
> + cs->kvm_vcpu_dirty = false;
> kvm_cpu_synchronize_state(cs);
>
> env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
>
--
Alexey
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] spapr: reset @kvm_vcpu_dirty before starting CPU
2014-01-15 4:57 [Qemu-devel] [PATCH] spapr: reset @kvm_vcpu_dirty before starting CPU Alexey Kardashevskiy
2014-01-15 22:47 ` Alexey Kardashevskiy
@ 2014-01-20 14:45 ` Alexander Graf
2014-01-20 15:02 ` Alexey Kardashevskiy
2014-01-20 16:20 ` Andreas Färber
2 siblings, 1 reply; 5+ messages in thread
From: Alexander Graf @ 2014-01-20 14:45 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Paolo Bonzini, qemu-ppc, QEMU Developers, Andreas Färber
On 15.01.2014, at 05:57, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> Normally QEMU kvm_arch_get_registers() reads registers and sets a dirty
> flag which prevents further registers reading from KVM till
> kvm_arch_put_registers() executes and resets the flag.
>
> However if we run QEMU with "-S" ("suspended"), then execute "info cpus"
> from the QEMU monitor, we end up with not reading registers in
> rtas_start_cpu() as qmp_query_cpus() calls kvm_cpu_synchronize_state()
> which leaves @kvm_vcpu_dirty=true what prevents kvm_cpu_synchronize_state()
> from synchronizing registers and we loose the values.
But that's fine because the values are already synchronized and couldn't have changed because the guest cpu wasn't running, no?
Alex
>
> This resets @kvm_vcpu_dirty flag as we do not have content which we really want to
> keep at this point as the CPU is halted.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>
> We could also reset @kvm_vcpu_dirty in qmp_query_cpus() but that would be
> racy.
> ---
> hw/ppc/spapr_rtas.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 1cb276d..3dade5e 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -185,6 +185,8 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPREnvironment *spapr,
> /* This will make sure qemu state is up to date with kvm, and
> * mark it dirty so our changes get flushed back before the
> * new cpu enters */
> +
> + cs->kvm_vcpu_dirty = false;
> kvm_cpu_synchronize_state(cs);
>
> env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
> --
> 1.8.4.rc4
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] spapr: reset @kvm_vcpu_dirty before starting CPU
2014-01-20 14:45 ` Alexander Graf
@ 2014-01-20 15:02 ` Alexey Kardashevskiy
0 siblings, 0 replies; 5+ messages in thread
From: Alexey Kardashevskiy @ 2014-01-20 15:02 UTC (permalink / raw)
To: Alexander Graf
Cc: Paolo Bonzini, qemu-ppc, QEMU Developers, Andreas Färber
On 01/21/2014 01:45 AM, Alexander Graf wrote:
>
> On 15.01.2014, at 05:57, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>
>> Normally QEMU kvm_arch_get_registers() reads registers and sets a dirty
>> flag which prevents further registers reading from KVM till
>> kvm_arch_put_registers() executes and resets the flag.
>>
>> However if we run QEMU with "-S" ("suspended"), then execute "info cpus"
>> from the QEMU monitor, we end up with not reading registers in
>> rtas_start_cpu() as qmp_query_cpus() calls kvm_cpu_synchronize_state()
>> which leaves @kvm_vcpu_dirty=true what prevents kvm_cpu_synchronize_state()
>> from synchronizing registers and we loose the values.
>
> But that's fine because the values are already synchronized and couldn't
> have changed because the guest cpu wasn't running, no?
Discovered today that "[PATCH] spapr: fix H_SET_MODE to sync LPCR with KVM"
seems to be v2 of this as that patch solves the issue this patch was
intended to solve so I'll drop this one.
>
>
> Alex
>
>>
>> This resets @kvm_vcpu_dirty flag as we do not have content which we really want to
>> keep at this point as the CPU is halted.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>
>> We could also reset @kvm_vcpu_dirty in qmp_query_cpus() but that would be
>> racy.
>> ---
>> hw/ppc/spapr_rtas.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>> index 1cb276d..3dade5e 100644
>> --- a/hw/ppc/spapr_rtas.c
>> +++ b/hw/ppc/spapr_rtas.c
>> @@ -185,6 +185,8 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPREnvironment *spapr,
>> /* This will make sure qemu state is up to date with kvm, and
>> * mark it dirty so our changes get flushed back before the
>> * new cpu enters */
>> +
>> + cs->kvm_vcpu_dirty = false;
>> kvm_cpu_synchronize_state(cs);
>>
>> env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
>> --
>> 1.8.4.rc4
>>
>
--
Alexey
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] spapr: reset @kvm_vcpu_dirty before starting CPU
2014-01-15 4:57 [Qemu-devel] [PATCH] spapr: reset @kvm_vcpu_dirty before starting CPU Alexey Kardashevskiy
2014-01-15 22:47 ` Alexey Kardashevskiy
2014-01-20 14:45 ` Alexander Graf
@ 2014-01-20 16:20 ` Andreas Färber
2 siblings, 0 replies; 5+ messages in thread
From: Andreas Färber @ 2014-01-20 16:20 UTC (permalink / raw)
To: Alexey Kardashevskiy, qemu-devel; +Cc: Paolo Bonzini, qemu-ppc, Alexander Graf
Am 15.01.2014 05:57, schrieb Alexey Kardashevskiy:
> Normally QEMU kvm_arch_get_registers() reads registers and sets a dirty
> flag which prevents further registers reading from KVM till
> kvm_arch_put_registers() executes and resets the flag.
>
> However if we run QEMU with "-S" ("suspended"), then execute "info cpus"
> from the QEMU monitor, we end up with not reading registers in
> rtas_start_cpu() as qmp_query_cpus() calls kvm_cpu_synchronize_state()
> which leaves @kvm_vcpu_dirty=true what prevents kvm_cpu_synchronize_state()
> from synchronizing registers and we loose the values.
>
> This resets @kvm_vcpu_dirty flag as we do not have content which we really want to
> keep at this point as the CPU is halted.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>
> We could also reset @kvm_vcpu_dirty in qmp_query_cpus() but that would be
> racy.
> ---
> hw/ppc/spapr_rtas.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 1cb276d..3dade5e 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -185,6 +185,8 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPREnvironment *spapr,
> /* This will make sure qemu state is up to date with kvm, and
> * mark it dirty so our changes get flushed back before the
> * new cpu enters */
> +
> + cs->kvm_vcpu_dirty = false;
> kvm_cpu_synchronize_state(cs);
While I am not familiar with whether or not the dirty flag should be
changed, I've noticed in the past that sPAPR is the only user of
kvm_cpu_synchronize_state() outside of KVM code IIUC. Would be nice if
you could replace that with cpu_synchronize_state() in its own patch
when you find some time.
Regards,
Andreas
>
> env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
>
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-01-20 16:21 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-01-15 4:57 [Qemu-devel] [PATCH] spapr: reset @kvm_vcpu_dirty before starting CPU Alexey Kardashevskiy
2014-01-15 22:47 ` Alexey Kardashevskiy
2014-01-20 14:45 ` Alexander Graf
2014-01-20 15:02 ` Alexey Kardashevskiy
2014-01-20 16:20 ` Andreas Färber
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.