Re: [Qemu-devel] [PATCH] target-i386: initialize APIC at CPU level

[Andreas Färber]

Am 04.10.2012 18:31, schrieb Igor Mammedov:
> On Thu, 04 Oct 2012 18:15:48 +0200
> Andreas Färber <address@hidden> wrote:
> 
>> Am 04.10.2012 16:43, schrieb Igor Mammedov:
>>> +static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
>>> +{
>>> +#ifndef CONFIG_USER_ONLY
>>> +    static int apic_mapped;
>>> +    CPUX86State *env = &cpu->env;
>>> +    const char *apic_type = "apic";
>>> +
>>> +    if (!(env->cpuid_features & CPUID_APIC || smp_cpus > 1)) {
>>> +        return;
>>> +    }
>>
>> I would prefer to keep the original logic at the call site
>> (x86_cpu_initfn)
> Could you explain it bit more?
> 
>> rather than silently returning without setting errp.
> and this one too, pls.

Sorry. What I mean here is don't let the function return without setting
an error condition if it didn't do anything (I consider that dangerous
since it potentially hides errors). Therefore only call it when we
actually do want the APIC (condition seems trivial enough) and then
signal an error if something is wrong, so that it's detectable:

>>> @@ -1878,6 +1934,8 @@ void x86_cpu_realize(Object *obj, Error **errp)
>>>      qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
>>>  #endif
>>>  

if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {

>>> +    x86_cpu_apic_init(cpu, errp);
>>
>> if (error_is_set(errp)) {
>>     return;
>> }

}

If we now truely signal errors and not only try to follow the realizefn
convention, you should also check the callers of x86_cpu_realize() to
make sure errp is not NULL and is being checked and printed after the call.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg