From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33462) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1br5iQ-00084S-4N for qemu-devel@nongnu.org; Mon, 03 Oct 2016 12:03:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1br5iL-0001EN-4D for qemu-devel@nongnu.org; Mon, 03 Oct 2016 12:03:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51813) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1br5iK-0001ED-Py for qemu-devel@nongnu.org; Mon, 03 Oct 2016 12:03:37 -0400 Date: Mon, 3 Oct 2016 13:03:33 -0300 From: Eduardo Habkost Message-ID: <20161003160333.GD3877@thinpad.lan.raisama.net> References: <20160930161013.9832-1-rkrcmar@redhat.com> <20160930161013.9832-2-rkrcmar@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160930161013.9832-2-rkrcmar@redhat.com> 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: Radim =?utf-8?B?S3LEjW3DocWZ?= Cc: qemu-devel@nongnu.org, Peter Xu , Igor Mammedov , Paolo Bonzini , Richard Henderson , "Michael S. Tsirkin" On Fri, Sep 30, 2016 at 06:10:06PM +0200, Radim Kr=C4=8Dm=C3=A1=C5=99 wro= te: > Every configuration has only up to one APIC class and we'll be extendin= g > 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 > index 14ac43c18666..8d01c9c8750e 100644 > --- a/hw/intc/apic_common.c > +++ b/hw/intc/apic_common.c > @@ -18,6 +18,7 @@ > * License along with this library; if not, see > */ > #include "qemu/osdep.h" > +#include "qemu/error-report.h" > #include "qapi/error.h" > #include "qemu-common.h" > #include "cpu.h" > diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_int= ernal.h > index 06c4e9f6f95b..286684857e9f 100644 > --- a/include/hw/i386/apic_internal.h > +++ b/include/hw/i386/apic_internal.h > @@ -222,4 +222,6 @@ static inline int apic_get_bit(uint32_t *tab, int i= ndex) > return !!(tab[i] & mask); > } > =20 > +APICCommonClass *apic_get_class(void); > + > #endif /* QEMU_APIC_INTERNAL_H */ > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 333309b9a70e..6acf9c3c2372 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -2845,9 +2845,8 @@ static void mce_init(X86CPU *cpu) > } > =20 > #ifndef CONFIG_USER_ONLY > -static void x86_cpu_apic_create(X86CPU *cpu, Error **errp) > +APICCommonClass *apic_get_class(void) > { > - APICCommonState *apic; > const char *apic_type =3D "apic"; > =20 > if (kvm_apic_in_kernel()) { > @@ -2856,7 +2855,16 @@ static void x86_cpu_apic_create(X86CPU *cpu, Err= or **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)); ObjectClass::type is private. I believe the common idiom is object_new(object_class_get_name(c)). Except for that, I believe the interface is OK and matches the existing logic. We can always make it better later, if appropriate. (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). > =20 > object_property_add_child(OBJECT(cpu), "lapic", > OBJECT(cpu->apic_state), &error_abort); > --=20 > 2.10.0 >=20 --=20 Eduardo