Re: [Qemu-devel] [PATCH qom-next 07/12] target-i386: move cpu halted decision into x86_cpu_reset

[Igor Mammedov]

On 05/30/2012 02:11 PM, Andreas Färber wrote:
> Am 30.05.2012 00:10, schrieb Igor Mammedov:
> > From: Igor Mammedov<address@hidden>
> >
> > MP initialization protocol differs between cpu families, and for P6 and
> > onward models it is up to CPU to decide if it will be BSP using this
> > protocol, so try to model this. However there is no point in implementing
> > MP initialization protocol in qemu. Thus first CPU is always marked as BSP.
> >
> > This patch:
> >   - moves decision to designate BSP from board into cpu, making cpu
> > self-sufficient in this regard. Later it will allow to cleanup hw/pc.c
> > and remove cpu_reset and wrappers from there.
> >   - stores flag that CPU is BSP in IA32_APIC_BASE to model behavior
> > described in Inted SDM vol 3a part 1 chapter 8.4.1
> >   - uses MSR_IA32_APICBASE_BSP flag in apic_base for checking if cpu is BSP
> >
> > patch is based on Jan Kiszka's proposal:
> >      http://thread.gmane.org/gmane.comp.emulators.qemu/100806
> >
> > v2:
> >    - fix build for i386-linux-user
> >        spotted-by: Peter Maydell<address@hidden>
> >
> > Signed-off-by: Igor Mammedov<address@hidden>
> > ---
> >   hw/apic.h            |    2 +-
> >   hw/apic_common.c     |   18 ++++++++++++------
> >   hw/pc.c              |    9 ---------
> >   target-i386/cpu.c    |    9 +++++++++
> >   target-i386/helper.c |    1 -
> >   target-i386/kvm.c    |    5 +++--
> >   6 files changed, 25 insertions(+), 19 deletions(-)
> >
> > diff --git a/hw/apic.h b/hw/apic.h
> > index 62179ce..d961ed4 100644
> > --- a/hw/apic.h
> > +++ b/hw/apic.h
> > @@ -20,9 +20,9 @@ void apic_init_reset(DeviceState *s);
> >   void apic_sipi(DeviceState *s);
> >   void apic_handle_tpr_access_report(DeviceState *d, target_ulong ip,
> >                                      TPRAccess access);
> > +void apic_designate_bsp(DeviceState *d);
> >
> >   /* pc.c */
> > -int cpu_is_bsp(CPUX86State *env);
> >   DeviceState *cpu_get_current_apic(void);
> >
> >   #endif
> > diff --git a/hw/apic_common.c b/hw/apic_common.c
> > index 46a9ff7..98ad6f0 100644
> > --- a/hw/apic_common.c
> > +++ b/hw/apic_common.c
> > @@ -43,8 +43,8 @@ uint64_t cpu_get_apic_base(DeviceState *d)
> >           trace_cpu_get_apic_base((uint64_t)s->apicbase);
> >           return s->apicbase;
> >       } else {
> > -        trace_cpu_get_apic_base(0);
> > -        return 0;
> > +        trace_cpu_get_apic_base(MSR_IA32_APICBASE_BSP);
> > +        return MSR_IA32_APICBASE_BSP;
> >       }
> >   }
> >
> > @@ -201,22 +201,28 @@ void apic_init_reset(DeviceState *d)
> >       s->timer_expiry = -1;
> >   }
> >
> > +void apic_designate_bsp(DeviceState *d)
> > +{
> > +    if (d) {
>
> This check looks odd. The function call is already guarded with
> cpu_index == 0. What other case would lead to d == NULL and can't we
> check that at the call site?

cpu_index == 0 doesn't imply that cpu has apic hence the check. And we for sure
can check it call site, would you like to do it there?
PS:
 there are many checks for this condition in APIC code, so may be it should
 be there style-wise at least?

> > +        APICCommonState *s = APIC_COMMON(d);
>
> If this is a QOM cast macro, it will implicitly assert d != NULL, no?

it actually will lead to null pointer dereference, like this:
  OBJECT_CHECK(type, obj, name) \
    ((type *)object_dynamic_cast_assert(OBJECT(obj), (name)))
      =>  object_dynamic_cast(obj, typename)
        =>  object_is_type(obj, target_type)
          =>  type_is_ancestor(obj->class->type, target_type);
                                  ^^^

> Andreas
>
> > +        s->apicbase |= MSR_IA32_APICBASE_BSP;
> > +    }
> > +}
> > +
> >   static void apic_reset_common(DeviceState *d)
> >   {
> >       APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
> >       APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
> > -    bool bsp;
> >
> > -    bsp = cpu_is_bsp(&s->cpu->env);
> >       s->apicbase = 0xfee00000 |
> > -        (bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_APICBASE_ENABLE;
> > +        (s->apicbase&  MSR_IA32_APICBASE_BSP) | MSR_IA32_APICBASE_ENABLE;
> >
> >       s->vapic_paddr = 0;
> >       info->vapic_base_update(s);
> >
> >       apic_init_reset(d);
> >
> > -    if (bsp) {
> > +    if (s->apicbase&  MSR_IA32_APICBASE_BSP) {
> >           /*
> >            * LINT0 delivery mode on CPU #0 is set to ExtInt at initialization
> >            * time typically by BIOS, so PIC interrupt can be delivered to the
> > diff --git a/hw/pc.c b/hw/pc.c
> > index 6bb3d2a..2f681db 100644
> > --- a/hw/pc.c
> > +++ b/hw/pc.c
> > @@ -870,12 +870,6 @@ void pc_init_ne2k_isa(ISABus *bus, NICInfo *nd)
> >       nb_ne2k++;
> >   }
> >
> > -int cpu_is_bsp(CPUX86State *env)
> > -{
> > -    /* We hard-wire the BSP to the first CPU. */
> > -    return env->cpu_index == 0;
> > -}
> > -
> >   DeviceState *cpu_get_current_apic(void)
> >   {
> >       if (cpu_single_env) {
> > @@ -942,10 +936,7 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int 
> > level)
> >   static void pc_cpu_reset(void *opaque)
> >   {
> >       X86CPU *cpu = opaque;
> > -    CPUX86State *env =&cpu->env;
> > -
> >       cpu_reset(CPU(cpu));
> > -    env->halted = !cpu_is_bsp(env);
> >   }
> >
> >   static X86CPU *pc_new_cpu(const char *cpu_model)
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 41a0436..f029f2a 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1704,6 +1704,15 @@ static void x86_cpu_reset(CPUState *s)
> >       env->dr[7] = DR7_FIXED_1;
> >       cpu_breakpoint_remove_all(env, BP_CPU);
> >       cpu_watchpoint_remove_all(env, BP_CPU);
> > +
> > +#if !defined(CONFIG_USER_ONLY)
> > +    /* We hard-wire the BSP to the first CPU. */
> > +    if (env->cpu_index == 0) {
> > +        apic_designate_bsp(env->apic_state);
> > +    }
> > +
> > +    env->halted = !(cpu_get_apic_base(env->apic_state)&  
> > MSR_IA32_APICBASE_BSP);
> > +#endif
> >   }
> >
> >   static void mce_init(X86CPU *cpu)
> > diff --git a/target-i386/helper.c b/target-i386/helper.c
> > index a94be0a..fd20fd4 100644
> > --- a/target-i386/helper.c
> > +++ b/target-i386/helper.c
> > @@ -1179,7 +1179,6 @@ void do_cpu_init(X86CPU *cpu)
> >       env->interrupt_request = sipi;
> >       env->pat = pat;
> >       apic_init_reset(env->apic_state);
> > -    env->halted = !cpu_is_bsp(env);
> >   }
> >
> >   void do_cpu_sipi(X86CPU *cpu)
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 0d0d8f6..09621e5 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -583,8 +583,9 @@ void kvm_arch_reset_vcpu(CPUX86State *env)
> >       env->interrupt_injected = -1;
> >       env->xcr0 = 1;
> >       if (kvm_irqchip_in_kernel()) {
> > -        env->mp_state = cpu_is_bsp(env) ? KVM_MP_STATE_RUNNABLE :
> > -                                          KVM_MP_STATE_UNINITIALIZED;
> > +        env->mp_state =
> > +            cpu_get_apic_base(env->apic_state)&  MSR_IA32_APICBASE_BSP ?
> > +            KVM_MP_STATE_RUNNABLE : KVM_MP_STATE_UNINITIALIZED;
> >       } else {
> >           env->mp_state = KVM_MP_STATE_RUNNABLE;
> >       }

--
-----
 Igor