From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44706) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brPvB-00068y-Cd for qemu-devel@nongnu.org; Tue, 04 Oct 2016 09:38:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1brPv5-00068V-Fn for qemu-devel@nongnu.org; Tue, 04 Oct 2016 09:38:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45318) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brPv5-00066q-5h for qemu-devel@nongnu.org; Tue, 04 Oct 2016 09:38:07 -0400 Date: Tue, 4 Oct 2016 15:38:01 +0200 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Message-ID: <20161004133801.GA23539@potion> References: <20160930161013.9832-1-rkrcmar@redhat.com> <20160930161013.9832-2-rkrcmar@redhat.com> <20161003160333.GD3877@thinpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20161003160333.GD3877@thinpad.lan.raisama.net> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 1/8] apic: add global apic_get_class() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: "Michael S. Tsirkin" , qemu-devel@nongnu.org, Peter Xu , Paolo Bonzini , Igor Mammedov , Richard Henderson 2016-10-03 13:03-0300, Eduardo Habkost: > On Fri, Sep 30, 2016 at 06:10:06PM +0200, Radim Kr=C4=8Dm=C3=A1=C5=99 w= rote: >> Every configuration has only up to one APIC class and we'll be extendi= ng >> the class with a function that can be called without an instanced >> object, so a direct access to the class is convenient. >>=20 >> This patch will break compilation if some code uses apic_get_class() >> with CONFIG_USER_ONLY. >>=20 >> Suggested-by: Eduardo Habkost >> Signed-off-by: Radim Kr=C4=8Dm=C3=A1=C5=99 >> --- >> v2: assert() instead of error_report() and exit() [Peter] >> v3: completely rewrite the mechanism [Eduardo] >>=20 >> It still looks horrible, so I'll be glad for any advice. >> And what is CONFIG_USER_ONLY? >> --- >> hw/intc/apic_common.c | 1 + >> include/hw/i386/apic_internal.h | 2 ++ >> target-i386/cpu.c | 14 +++++++++++--- >> 3 files changed, 14 insertions(+), 3 deletions(-) >>=20 >> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c >> @@ -2856,7 +2855,16 @@ static void x86_cpu_apic_create(X86CPU *cpu, Er= ror **errp) >> apic_type =3D "xen-apic"; >> } >> =20 >> - cpu->apic_state =3D DEVICE(object_new(apic_type)); >> + return APIC_COMMON_CLASS(object_class_by_name(apic_type)); >> +} >> + >> +static void x86_cpu_apic_create(X86CPU *cpu, Error **errp) >> +{ >> + APICCommonState *apic; >> + ObjectClass *apic_object_class =3D OBJECT_CLASS(apic_get_class())= ; >> + >> + assert(apic_object_class); >> + cpu->apic_state =3D DEVICE(object_new_with_type(apic_object_class= ->type)); >=20 > ObjectClass::type is private. I believe the common idiom is > object_new(object_class_get_name(c)). Will fix in v4. object_class_get_name() loses information about the type, so object_new() has to look it up from the name, which is unnecessary. Should I follow with a patch/series that adds Object *object_new_with_class(ObjectClass *class) { return object_new_with_type(class->type); } into qom/object.[ch] and replaces occurrences of object_new_with_type(object_class_get_name()) with it? > Except for that, I believe the interface is OK and matches the > existing logic. We can always make it better later, if > appropriate. Thanks. > (e.g. I wonder if we could have a container object for all APICs > (icc-bus?), and move the send_msi() method and the > apic_get_class() logic to it). Yes, and a QEMU-specific bus for interrupts seems nicer that going the hardware-emulation route and implementing FSB for q35 and APIC bus for fx440. I think that the MMIO area we have should only work from PCI devices (not when the CPU writes to the area), so it would be a step towards hardware-like behavior as well. I don't think it would achieve negative diffstat, though ...