[Qemu-devel] Re: [Qemu-commits] [COMMIT 733318e] don't call cpu_sychronize_state from reset handlers

[Qemu-devel] Re: [Qemu-commits] [COMMIT 733318e] don't call cpu_sychronize_state from reset handlers

[Gerd Hoffmann]

On 09/10/09 00:37, Anthony Liguori wrote:
> From: Glauber Costa<glommer@redhat.com>
>
> Doing this will make the vcpu ioctl be issued from the I/O thread, instead
> of cpu thread. The correct behaviour is to call it from within the cpu thread,
> as soon as we are ready to go.

With this patch booting rawhide stops working.
The kernel has trouble setting up the timer and stops after hpet setup:

[ ... ]
..TIMER: vector=0x30 apic1=0 pin1=2 apic2=-1 pin2=-1
..MP-BIOS bug: 8254 timer not connected to IO-APIC
...trying to set up timer (IRQ0) through the 8259A ...
..... (found apic 0 pin 2) ...
....... failed.
...trying to set up timer as Virtual Wire IRQ...
..... failed.
...trying to set up timer as ExtINT IRQ...
..... works.
[ ... ]
hpet0: at MMIO 0xfed00000, IRQs 2, 8, 0
hpet0: 3 comparators, 64-bit 100.000000 MHz counter
[ kernel hangs here ]

cheers,
   Gerd

[Qemu-devel] Re: [Qemu-commits] [COMMIT 733318e] don't call cpu_sychronize_state from reset handlers

[Avi Kivity]

On 09/10/2009 01:37 AM, Anthony Liguori wrote:
> From: Glauber Costa<glommer@redhat.com>
>
> Doing this will make the vcpu ioctl be issued from the I/O thread, instead
> of cpu thread. The correct behaviour is to call it from within the cpu thread,
> as soon as we are ready to go.
>
>    

This commit, when merged into qemu-kvm, breaks reboot and system_reset.  
I haven't checked upstream qemu (probably needs iothread enabled to have 
an effect).

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [Qemu-commits] [COMMIT 733318e] don't call cpu_sychronize_state from reset handlers

[Glauber Costa]

On Thu, Sep 10, 2009 at 02:44:04PM +0300, Avi Kivity wrote:
> On 09/10/2009 01:37 AM, Anthony Liguori wrote:
>> From: Glauber Costa<glommer@redhat.com>
>>
>> Doing this will make the vcpu ioctl be issued from the I/O thread, instead
>> of cpu thread. The correct behaviour is to call it from within the cpu thread,
>> as soon as we are ready to go.
>>
>>    
>
> This commit, when merged into qemu-kvm, breaks reboot and system_reset.   
> I haven't checked upstream qemu (probably needs iothread enabled to have  
> an effect).
Yes, unfortunately. I told anthony yesterday to drop it, as I have noticed it
myself. this relation is much more subtle, and I'll have to take a look at a
better way to solve the problem

[Qemu-devel] Re: [COMMIT 733318e] don't call cpu_sychronize_state from reset handlers

[Jan Kiszka]

Anthony Liguori wrote:
> From: Glauber Costa <glommer-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> Doing this will make the vcpu ioctl be issued from the I/O thread, instead
> of cpu thread. The correct behaviour is to call it from within the cpu thread,
> as soon as we are ready to go.

Note that in the good old days, this used to work properly (in qemu-kvm)
as registers write-back was routed through on_vcpu.

> 
> Signed-off-by: Glauber Costa <glommer-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Anthony Liguori <aliguori-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> 
> diff --git a/hw/apic.c b/hw/apic.c
> index 2c414c1..9f1d25e 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -938,8 +938,6 @@ static void apic_reset(void *opaque)
>      APICState *s = opaque;
>      int bsp;
>  
> -    cpu_synchronize_state(s->cpu_env);
> -
>      bsp = cpu_is_bsp(s->cpu_env);
>      s->apicbase = 0xfee00000 |
>          (bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_APICBASE_ENABLE;
> diff --git a/vl.c b/vl.c
> index 8e5d9db..c6c6a6b 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3683,10 +3683,12 @@ static void *kvm_cpu_thread_fn(void *arg)
>      while (!qemu_system_ready)
>          qemu_cond_timedwait(&qemu_system_cond, &qemu_global_mutex, 100);
>  
> +    cpu_synchronize_state(env);
> +
>      while (1) {
> +        qemu_wait_io_event(env);
>          if (cpu_can_run(env))
>              qemu_cpu_exec(env);
> -        qemu_wait_io_event(env);
>      }
>  
>      return NULL;
> @@ -3711,6 +3713,9 @@ static void *tcg_cpu_thread_fn(void *arg)
>      while (!qemu_system_ready)
>          qemu_cond_timedwait(&qemu_system_cond, &qemu_global_mutex, 100);
>  
> +    for (env = first_cpu; env != NULL; env = env->next_cpu) {
> +        cpu_synchronize_state(env);
> +    }
>      while (1) {
>          tcg_cpu_exec();
>          qemu_wait_io_event(cur_cpu);
> 

This unfortunately breaks upstream KVM (Linux fails to detect the tiemr
IRQ properly). Is there a fix already in sight? I lost a bit overview of
all on_vcpu refactorings and kvm-register sync "simplifications".

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [COMMIT 733318e] don't call cpu_sychronize_state from reset handlers

[Glauber Costa]

On Fri, Sep 11, 2009 at 01:15:49PM +0200, Jan Kiszka wrote:
> Anthony Liguori wrote:
> > From: Glauber Costa <glommer-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > 
> > Doing this will make the vcpu ioctl be issued from the I/O thread, instead
> > of cpu thread. The correct behaviour is to call it from within the cpu thread,
> > as soon as we are ready to go.
> 
> Note that in the good old days, this used to work properly (in qemu-kvm)
> as registers write-back was routed through on_vcpu.

I believe we should avoid the use of those things, specially at initialization. They are
totally racy and fragile. One way to do that, is to do all the reset functions inside the
cpu thread.

I already have something hacked up for this, will send as soon as I finish testing.

> 
> > 
> > Signed-off-by: Glauber Costa <glommer-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Anthony Liguori <aliguori-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> > 
> > diff --git a/hw/apic.c b/hw/apic.c
> > index 2c414c1..9f1d25e 100644
> > --- a/hw/apic.c
> > +++ b/hw/apic.c
> > @@ -938,8 +938,6 @@ static void apic_reset(void *opaque)
> >      APICState *s = opaque;
> >      int bsp;
> >  
> > -    cpu_synchronize_state(s->cpu_env);
> > -
> >      bsp = cpu_is_bsp(s->cpu_env);
> >      s->apicbase = 0xfee00000 |
> >          (bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_APICBASE_ENABLE;
> > diff --git a/vl.c b/vl.c
> > index 8e5d9db..c6c6a6b 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -3683,10 +3683,12 @@ static void *kvm_cpu_thread_fn(void *arg)
> >      while (!qemu_system_ready)
> >          qemu_cond_timedwait(&qemu_system_cond, &qemu_global_mutex, 100);
> >  
> > +    cpu_synchronize_state(env);
> > +
> >      while (1) {
> > +        qemu_wait_io_event(env);
> >          if (cpu_can_run(env))
> >              qemu_cpu_exec(env);
> > -        qemu_wait_io_event(env);
> >      }
> >  
> >      return NULL;
> > @@ -3711,6 +3713,9 @@ static void *tcg_cpu_thread_fn(void *arg)
> >      while (!qemu_system_ready)
> >          qemu_cond_timedwait(&qemu_system_cond, &qemu_global_mutex, 100);
> >  
> > +    for (env = first_cpu; env != NULL; env = env->next_cpu) {
> > +        cpu_synchronize_state(env);
> > +    }
> >      while (1) {
> >          tcg_cpu_exec();
> >          qemu_wait_io_event(cur_cpu);
> > 
> 
> This unfortunately breaks upstream KVM (Linux fails to detect the tiemr
> IRQ properly). Is there a fix already in sight? I lost a bit overview of
> all on_vcpu refactorings and kvm-register sync "simplifications".
> 
> Jan
> 
> -- 
> Siemens AG, Corporate Technology, CT SE 2
> Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [COMMIT 733318e] don't call cpu_sychronize_state from reset handlers

[Jan Kiszka]

Glauber Costa wrote:
> On Fri, Sep 11, 2009 at 01:15:49PM +0200, Jan Kiszka wrote:
>> Anthony Liguori wrote:
>>> From: Glauber Costa <glommer-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>
>>> Doing this will make the vcpu ioctl be issued from the I/O thread, instead
>>> of cpu thread. The correct behaviour is to call it from within the cpu thread,
>>> as soon as we are ready to go.
>> Note that in the good old days, this used to work properly (in qemu-kvm)
>> as registers write-back was routed through on_vcpu.
> 
> I believe we should avoid the use of those things, specially at initialization. They are
> totally racy and fragile. One way to do that, is to do all the reset functions inside the
> cpu thread.

As all this used to work fine in practice in upstream as well as in
qemu-kvm, I'm slightly unhappy about the current situation.

> 
> I already have something hacked up for this, will send as soon as I finish testing.

OK, looking forward.

BTW, what's the state of the fix for the guest-debug regression under KVM?

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [COMMIT 733318e] don't call cpu_sychronize_state from reset handlers

[Glauber Costa]

On Fri, Sep 11, 2009 at 01:52:05PM +0200, Jan Kiszka wrote:
> Glauber Costa wrote:
> > On Fri, Sep 11, 2009 at 01:15:49PM +0200, Jan Kiszka wrote:
> >> Anthony Liguori wrote:
> >>> From: Glauber Costa <glommer-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >>>
> >>> Doing this will make the vcpu ioctl be issued from the I/O thread, instead
> >>> of cpu thread. The correct behaviour is to call it from within the cpu thread,
> >>> as soon as we are ready to go.
> >> Note that in the good old days, this used to work properly (in qemu-kvm)
> >> as registers write-back was routed through on_vcpu.
> > 
> > I believe we should avoid the use of those things, specially at initialization. They are
> > totally racy and fragile. One way to do that, is to do all the reset functions inside the
> > cpu thread.
> 
> As all this used to work fine in practice in upstream as well as in
> qemu-kvm, I'm slightly unhappy about the current situation.
> 
> > 
> > I already have something hacked up for this, will send as soon as I finish testing.
> 
> OK, looking forward.
> 
> BTW, what's the state of the fix for the guest-debug regression under KVM?
The last proposal I sent for queue_work would fix it, but it is a too big job, for just a fix.
If you are not using the IO thread, I can do what I did in there: define on_vcpu to be
conditional to the I/O thread, and in case it is not defined, just call the function

What do you think?

[Qemu-devel] Re: [COMMIT 733318e] don't call cpu_sychronize_state from reset handlers

[Jan Kiszka]

Glauber Costa wrote:
> On Fri, Sep 11, 2009 at 01:52:05PM +0200, Jan Kiszka wrote:
>> Glauber Costa wrote:
>>> On Fri, Sep 11, 2009 at 01:15:49PM +0200, Jan Kiszka wrote:
>>>> Anthony Liguori wrote:
>>>>> From: Glauber Costa <glommer-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>>>
>>>>> Doing this will make the vcpu ioctl be issued from the I/O thread, instead
>>>>> of cpu thread. The correct behaviour is to call it from within the cpu thread,
>>>>> as soon as we are ready to go.
>>>> Note that in the good old days, this used to work properly (in qemu-kvm)
>>>> as registers write-back was routed through on_vcpu.
>>> I believe we should avoid the use of those things, specially at initialization. They are
>>> totally racy and fragile. One way to do that, is to do all the reset functions inside the
>>> cpu thread.
>> As all this used to work fine in practice in upstream as well as in
>> qemu-kvm, I'm slightly unhappy about the current situation.
>>
>>> I already have something hacked up for this, will send as soon as I finish testing.
>> OK, looking forward.
>>
>> BTW, what's the state of the fix for the guest-debug regression under KVM?
> The last proposal I sent for queue_work would fix it, but it is a too big job, for just a fix.
> If you are not using the IO thread, I can do what I did in there: define on_vcpu to be
> conditional to the I/O thread, and in case it is not defined, just call the function
> 
> What do you think?

IIUC, iothread and kvm is still broken in many ways here, so I'm fine
with a temporary workaround. Just make sure that it doesn't cause too
much merging pain downstream.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [COMMIT 733318e] don't call cpu_sychronize_state from reset handlers

[Glauber Costa]

On Fri, Sep 11, 2009 at 02:28:41PM +0200, Jan Kiszka wrote:
> Glauber Costa wrote:
> > On Fri, Sep 11, 2009 at 01:52:05PM +0200, Jan Kiszka wrote:
> >> Glauber Costa wrote:
> >>> On Fri, Sep 11, 2009 at 01:15:49PM +0200, Jan Kiszka wrote:
> >>>> Anthony Liguori wrote:
> >>>>> From: Glauber Costa <glommer-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >>>>>
> >>>>> Doing this will make the vcpu ioctl be issued from the I/O thread, instead
> >>>>> of cpu thread. The correct behaviour is to call it from within the cpu thread,
> >>>>> as soon as we are ready to go.
> >>>> Note that in the good old days, this used to work properly (in qemu-kvm)
> >>>> as registers write-back was routed through on_vcpu.
> >>> I believe we should avoid the use of those things, specially at initialization. They are
> >>> totally racy and fragile. One way to do that, is to do all the reset functions inside the
> >>> cpu thread.
> >> As all this used to work fine in practice in upstream as well as in
> >> qemu-kvm, I'm slightly unhappy about the current situation.
> >>
> >>> I already have something hacked up for this, will send as soon as I finish testing.
> >> OK, looking forward.
> >>
> >> BTW, what's the state of the fix for the guest-debug regression under KVM?
> > The last proposal I sent for queue_work would fix it, but it is a too big job, for just a fix.
> > If you are not using the IO thread, I can do what I did in there: define on_vcpu to be
> > conditional to the I/O thread, and in case it is not defined, just call the function
> > 
> > What do you think?
> 
> IIUC, iothread and kvm is still broken in many ways here, so I'm fine
> with a temporary workaround. Just make sure that it doesn't cause too
> much merging pain downstream.
Actually, with the fixes I sent this week, it should work fine.