From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:49932)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <afaerber@suse.de>) id 1TJoZx-0002XN-HB
	for qemu-devel@nongnu.org; Thu, 04 Oct 2012 12:47:18 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <afaerber@suse.de>) id 1TJoZr-0001Nq-NL
	for qemu-devel@nongnu.org; Thu, 04 Oct 2012 12:47:17 -0400
Received: from cantor2.suse.de ([195.135.220.15]:45716 helo=mx2.suse.de)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <afaerber@suse.de>) id 1TJoZr-0001NI-Dm
	for qemu-devel@nongnu.org; Thu, 04 Oct 2012 12:47:11 -0400
Message-ID: <506DBD8B.6040000@suse.de>
Date: Thu, 04 Oct 2012 18:47:07 +0200
From: =?ISO-8859-1?Q?Andreas_F=E4rber?= <afaerber@suse.de>
MIME-Version: 1.0
References: <1349361818-15331-1-git-send-email-imammedo@redhat.com>
	<506DB634.70001@suse.de>
	<20121004183117.4d51b0d2@nial.usersys.redhat.com>
In-Reply-To: <20121004183117.4d51b0d2@nial.usersys.redhat.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
Subject: Re: [Qemu-devel] [PATCH] target-i386: initialize APIC at CPU level
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Igor Mammedov <imammedo@redhat.com>
Cc: aliguori@us.ibm.com, ehabkost@redhat.com, gleb@redhat.com, Don@CloudSwitch.com, qemu-devel@nongnu.org, jan.kiszka@web.de

Am 04.10.2012 18:31, schrieb Igor Mammedov:
> On Thu, 04 Oct 2012 18:15:48 +0200
> Andreas F=E4rber <afaerber@suse.de> wrote:
>=20
>> 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 =3D &cpu->env;
>>> +    const char *apic_type =3D "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?
>=20
>> 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
>>> =20

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 cal=
l.

Regards,
Andreas

--=20
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe=
rg