[Qemu-devel] [PATCH v2] Apic creation should not depend on pci

[Qemu-devel] [PATCH v2] Apic creation should not depend on pci

[Gleb Natapov]

    
It should depend on whether cpu has APIC.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
diff --git a/hw/pc.c b/hw/pc.c
index 0934778..cb49772 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -878,14 +878,10 @@ static void pc_init1(ram_addr_t ram_size,
         }
         if (i != 0)
             env->halted = 1;
-        if (smp_cpus > 1) {
-            /* XXX: enable it in all cases */
-            env->cpuid_features |= CPUID_APIC;
-        }
-        qemu_register_reset(main_cpu_reset, 0, env);
-        if (pci_enabled) {
+        if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
             apic_init(env);
         }
+        qemu_register_reset(main_cpu_reset, 0, env);
     }
 
     vmport_init();
--
			Gleb.

[Qemu-devel] Re: [PATCH v2] Apic creation should not depend on pci

[Jan Kiszka]

Gleb Natapov wrote:
>     
> It should depend on whether cpu has APIC.
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> diff --git a/hw/pc.c b/hw/pc.c
> index 0934778..cb49772 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -878,14 +878,10 @@ static void pc_init1(ram_addr_t ram_size,
>          }
>          if (i != 0)
>              env->halted = 1;
> -        if (smp_cpus > 1) {
> -            /* XXX: enable it in all cases */
> -            env->cpuid_features |= CPUID_APIC;
> -        }
> -        qemu_register_reset(main_cpu_reset, 0, env);
> -        if (pci_enabled) {
> +        if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {

Obviously :), I'm fine with that change. Needs testing, though. What
scenarios did you already check?

>              apic_init(env);
>          }
> +        qemu_register_reset(main_cpu_reset, 0, env);

But this line silently reorders CPU and APIC reset handlers. If you did
it intentionally (I vaguely recall it may have some benefit /wrt KVM
synchronizing kernel and user space states), I would suggest pushing it
as a separate patch.

>      }
>  
>      vmport_init();
> --
> 			Gleb.
> 

Jan

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

[Qemu-devel] Re: [PATCH v2] Apic creation should not depend on pci

[Gleb Natapov]

On Mon, Jun 08, 2009 at 04:03:19PM +0200, Jan Kiszka wrote:
> Gleb Natapov wrote:
> >     
> > It should depend on whether cpu has APIC.
> > 
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > diff --git a/hw/pc.c b/hw/pc.c
> > index 0934778..cb49772 100644
> > --- a/hw/pc.c
> > +++ b/hw/pc.c
> > @@ -878,14 +878,10 @@ static void pc_init1(ram_addr_t ram_size,
> >          }
> >          if (i != 0)
> >              env->halted = 1;
> > -        if (smp_cpus > 1) {
> > -            /* XXX: enable it in all cases */
> > -            env->cpuid_features |= CPUID_APIC;
> > -        }
> > -        qemu_register_reset(main_cpu_reset, 0, env);
> > -        if (pci_enabled) {
> > +        if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
> 
> Obviously :), I'm fine with that change. Needs testing, though. What
> scenarios did you already check?
> 
:) Only standard runs. I don't have 486 smp guest around.

> >              apic_init(env);
> >          }
> > +        qemu_register_reset(main_cpu_reset, 0, env);
> 
> But this line silently reorders CPU and APIC reset handlers. If you did
> it intentionally (I vaguely recall it may have some benefit /wrt KVM
> synchronizing kernel and user space states), I would suggest pushing it
> as a separate patch.
I don't know what this line is doing here. I'll remove it

--
			Gleb.

[Qemu-devel] Re: [PATCH v2] Apic creation should not depend on pci

[Gleb Natapov]

On Mon, Jun 08, 2009 at 04:03:19PM +0200, Jan Kiszka wrote:
> Gleb Natapov wrote:
> >     
> > It should depend on whether cpu has APIC.
> > 
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > diff --git a/hw/pc.c b/hw/pc.c
> > index 0934778..cb49772 100644
> > --- a/hw/pc.c
> > +++ b/hw/pc.c
> > @@ -878,14 +878,10 @@ static void pc_init1(ram_addr_t ram_size,
> >          }
> >          if (i != 0)
> >              env->halted = 1;
> > -        if (smp_cpus > 1) {
> > -            /* XXX: enable it in all cases */
> > -            env->cpuid_features |= CPUID_APIC;
> > -        }
> > -        qemu_register_reset(main_cpu_reset, 0, env);
> > -        if (pci_enabled) {
> > +        if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
> 
> Obviously :), I'm fine with that change. Needs testing, though. What
> scenarios did you already check?
> 
> >              apic_init(env);
> >          }
> > +        qemu_register_reset(main_cpu_reset, 0, env);
> 
> But this line silently reorders CPU and APIC reset handlers. If you did
> it intentionally (I vaguely recall it may have some benefit /wrt KVM
> synchronizing kernel and user space states), I would suggest pushing it
> as a separate patch.
> 
BTW relying on order of callback registration is not a good idea
especially since we have "order" parameter now. On the other hand apic
reset handler already resets cpu so if apic is present there is no need to
register main_cpu_reset().

--
			Gleb.

[Qemu-devel] Re: [PATCH v2] Apic creation should not depend on pci

[Jan Kiszka]

Gleb Natapov wrote:
> On Mon, Jun 08, 2009 at 04:03:19PM +0200, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>>     
>>> It should depend on whether cpu has APIC.
>>>
>>> Signed-off-by: Gleb Natapov <gleb@redhat.com>
>>> diff --git a/hw/pc.c b/hw/pc.c
>>> index 0934778..cb49772 100644
>>> --- a/hw/pc.c
>>> +++ b/hw/pc.c
>>> @@ -878,14 +878,10 @@ static void pc_init1(ram_addr_t ram_size,
>>>          }
>>>          if (i != 0)
>>>              env->halted = 1;
>>> -        if (smp_cpus > 1) {
>>> -            /* XXX: enable it in all cases */
>>> -            env->cpuid_features |= CPUID_APIC;
>>> -        }
>>> -        qemu_register_reset(main_cpu_reset, 0, env);
>>> -        if (pci_enabled) {
>>> +        if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
>> Obviously :), I'm fine with that change. Needs testing, though. What
>> scenarios did you already check?
>>
>>>              apic_init(env);
>>>          }
>>> +        qemu_register_reset(main_cpu_reset, 0, env);
>> But this line silently reorders CPU and APIC reset handlers. If you did
>> it intentionally (I vaguely recall it may have some benefit /wrt KVM
>> synchronizing kernel and user space states), I would suggest pushing it
>> as a separate patch.
>>
> BTW relying on order of callback registration is not a good idea
> especially since we have "order" parameter now.

The order parameter is obsolete, I already posted a patch to revert this
idea again (reminds me of posting an update as a new reset handler
arrived in the meantime).

The system-wide assumed and applied order is that earlier instantiated
devices will be reset first. That specifically makes sense if you think
of bus/device relations.

> On the other hand apic
> reset handler already resets cpu so if apic is present there is no need to
> register main_cpu_reset().

OK, this is a special case as the APIC reset triggers an init IPI and
that resets the CPU, too. Then make this explicit, replace
main_cpu_reset with cpu_reset (so that no one adds code to the former
that is not run in the APIC case) and add some comment why.

Jan

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

[Qemu-devel] Re: [PATCH v2] Apic creation should not depend on pci

[Gleb Natapov]

On Tue, Jun 09, 2009 at 10:37:53AM +0200, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Mon, Jun 08, 2009 at 04:03:19PM +0200, Jan Kiszka wrote:
> >> Gleb Natapov wrote:
> >>>     
> >>> It should depend on whether cpu has APIC.
> >>>
> >>> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> >>> diff --git a/hw/pc.c b/hw/pc.c
> >>> index 0934778..cb49772 100644
> >>> --- a/hw/pc.c
> >>> +++ b/hw/pc.c
> >>> @@ -878,14 +878,10 @@ static void pc_init1(ram_addr_t ram_size,
> >>>          }
> >>>          if (i != 0)
> >>>              env->halted = 1;
> >>> -        if (smp_cpus > 1) {
> >>> -            /* XXX: enable it in all cases */
> >>> -            env->cpuid_features |= CPUID_APIC;
> >>> -        }
> >>> -        qemu_register_reset(main_cpu_reset, 0, env);
> >>> -        if (pci_enabled) {
> >>> +        if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
> >> Obviously :), I'm fine with that change. Needs testing, though. What
> >> scenarios did you already check?
> >>
> >>>              apic_init(env);
> >>>          }
> >>> +        qemu_register_reset(main_cpu_reset, 0, env);
> >> But this line silently reorders CPU and APIC reset handlers. If you did
> >> it intentionally (I vaguely recall it may have some benefit /wrt KVM
> >> synchronizing kernel and user space states), I would suggest pushing it
> >> as a separate patch.
> >>
> > BTW relying on order of callback registration is not a good idea
> > especially since we have "order" parameter now.
> 
> The order parameter is obsolete, I already posted a patch to revert this
> idea again (reminds me of posting an update as a new reset handler
> arrived in the meantime).
> 
> The system-wide assumed and applied order is that earlier instantiated
> devices will be reset first. That specifically makes sense if you think
> of bus/device relations.
> 
Sounds fragile to me, but explicit order far from ideal too. Why should
order matter anyway. Device reset callbacks should not rely on the state
of other devices. 

> > On the other hand apic
> > reset handler already resets cpu so if apic is present there is no need to
> > register main_cpu_reset().
> 
> OK, this is a special case as the APIC reset triggers an init IPI and
> that resets the CPU, too. Then make this explicit, replace
> main_cpu_reset with cpu_reset (so that no one adds code to the former
> that is not run in the APIC case) and add some comment why.
Already doing it on top of this patch.

--
			Gleb.