[Qemu-devel] [PATCH] spapr: reset @kvm_vcpu_dirty before starting CPU

[Qemu-devel] [PATCH] spapr: reset @kvm_vcpu_dirty before starting CPU

[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);
 
         env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
-- 
1.8.4.rc4

Re: [Qemu-devel] [PATCH] spapr: reset @kvm_vcpu_dirty before starting CPU

[Alexey Kardashevskiy]

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

Re: [Qemu-devel] [PATCH] spapr: reset @kvm_vcpu_dirty before starting CPU

[Alexander Graf]

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
> 

Re: [Qemu-devel] [PATCH] spapr: reset @kvm_vcpu_dirty before starting CPU

[Alexey Kardashevskiy]

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

Re: [Qemu-devel] [PATCH] spapr: reset @kvm_vcpu_dirty before starting CPU

[Andreas Färber]

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