Patchwork [5/7] tell kernel about all registers instead of just mp_state

login
register
mail settings
Submitter Glauber Costa
Date Nov. 26, 2009, 5:24 p.m.
Message ID <1259256300-23937-6-git-send-email-glommer@redhat.com>
Download mbox | patch
Permalink /patch/39577/
State New
Headers show

Comments

Glauber Costa - Nov. 26, 2009, 5:24 p.m.
This fix a bug with -smp in kvm. Since we have updated apic_base,
we also have to tell kernel about it. So instead of just updating
mp_state, update every regs.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 hw/apic-kvm.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
Avi Kivity - Nov. 29, 2009, 3:32 p.m.
On 11/26/2009 07:24 PM, Glauber Costa wrote:
> This fix a bug with -smp in kvm. Since we have updated apic_base,
> we also have to tell kernel about it. So instead of just updating
> mp_state, update every regs.
>
> Signed-off-by: Glauber Costa<glommer@redhat.com>
> ---
>   hw/apic-kvm.c |    5 ++++-
>   1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/hw/apic-kvm.c b/hw/apic-kvm.c
> index e5a0bfc..dc61386 100644
> --- a/hw/apic-kvm.c
> +++ b/hw/apic-kvm.c
> @@ -126,7 +126,10 @@ static void kvm_apic_reset(void *opaque)
>       s->cpu_env->mp_state
>               = bsp ? KVM_MP_STATE_RUNNABLE : KVM_MP_STATE_UNINITIALIZED;
>
> -    kvm_put_mp_state(s->cpu_env);
> +    /* We have to tell the kernel about mp_state, but also save sregs, since
> +     * apic base was just updated
> +     */
> +    kvm_arch_put_registers(s->cpu_env);
>
>    

Better to use cpu_synchronize_state() instead.
Glauber Costa - Nov. 30, 2009, 11:45 a.m.
>> since
>> +     * apic base was just updated
>> +     */
>> +    kvm_arch_put_registers(s->cpu_env);
>>
>>
>
> Better to use cpu_synchronize_state() instead.
>
I might be misreading it, but :

void kvm_cpu_synchronize_state(CPUState *env)
{
    if (!env->kvm_state->regs_modified) {
        kvm_arch_get_registers(env);
        env->kvm_state->regs_modified = 1;
    }
}

Only does get. And we need put.

Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."
Gleb Natapov - Nov. 30, 2009, 12:04 p.m.
On Mon, Nov 30, 2009 at 09:45:56AM -0200, Glauber Costa wrote:
> >> since
> >> +     * apic base was just updated
> >> +     */
> >> +    kvm_arch_put_registers(s->cpu_env);
> >>
> >>
> >
> > Better to use cpu_synchronize_state() instead.
> >
> I might be misreading it, but :
> 
> void kvm_cpu_synchronize_state(CPUState *env)
> {
>     if (!env->kvm_state->regs_modified) {
>         kvm_arch_get_registers(env);
>         env->kvm_state->regs_modified = 1;
>     }
> }
> 
> Only does get. And we need put.
> 
Put is on the entry into the kernel in vcpu_run.

--
			Gleb.
Avi Kivity - Nov. 30, 2009, 12:05 p.m.
On 11/30/2009 01:45 PM, Glauber Costa wrote:
>>
>> Better to use cpu_synchronize_state() instead.
>>
>>      
> I might be misreading it, but :
>
> void kvm_cpu_synchronize_state(CPUState *env)
> {
>      if (!env->kvm_state->regs_modified) {
>          kvm_arch_get_registers(env);
>          env->kvm_state->regs_modified = 1;
>      }
> }
>
> Only does get. And we need put.
>    

It will autoput during the next guest entry.
Glauber Costa - Nov. 30, 2009, 1:31 p.m.
On Mon, Nov 30, 2009 at 10:05 AM, Avi Kivity <avi@redhat.com> wrote:
> On 11/30/2009 01:45 PM, Glauber Costa wrote:
>>>
>>> Better to use cpu_synchronize_state() instead.
>>>
>>>
>>
>> I might be misreading it, but :
>>
>> void kvm_cpu_synchronize_state(CPUState *env)
>> {
>>     if (!env->kvm_state->regs_modified) {
>>         kvm_arch_get_registers(env);
>>         env->kvm_state->regs_modified = 1;
>>     }
>> }
>>
>> Only does get. And we need put.
>>
>
> It will autoput during the next guest entry.

So it should be working already, no?

We do a cpu_synchronize_state() on the beginning of that function, and
vcpu does not run
until it is finished.

>
> --
> error compiling committee.c: too many arguments to function
>
>
Avi Kivity - Nov. 30, 2009, 1:32 p.m.
On 11/30/2009 03:31 PM, Glauber Costa wrote:
>
>>> Only does get. And we need put.
>>>
>>>        
>> It will autoput during the next guest entry.
>>      
> So it should be working already, no?
>
> We do a cpu_synchronize_state() on the beginning of that function, and
> vcpu does not run
> until it is finished

Yes.

Patch

diff --git a/hw/apic-kvm.c b/hw/apic-kvm.c
index e5a0bfc..dc61386 100644
--- a/hw/apic-kvm.c
+++ b/hw/apic-kvm.c
@@ -126,7 +126,10 @@  static void kvm_apic_reset(void *opaque)
     s->cpu_env->mp_state
             = bsp ? KVM_MP_STATE_RUNNABLE : KVM_MP_STATE_UNINITIALIZED;
 
-    kvm_put_mp_state(s->cpu_env);
+    /* We have to tell the kernel about mp_state, but also save sregs, since
+     * apic base was just updated
+     */
+    kvm_arch_put_registers(s->cpu_env);
 
     if (bsp) {
         /*