| Message ID | 20160930161013.9832-2-rkrcmar@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Sep 30, 2016 at 06:10:06PM +0200, Radim Krčmář wrote: > Every configuration has only up to one APIC class and we'll be extending > the class with a function that can be called without an instanced > object, so a direct access to the class is convenient. > > This patch will break compilation if some code uses apic_get_class() > with CONFIG_USER_ONLY. > > Suggested-by: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > --- > v2: assert() instead of error_report() and exit() [Peter] > v3: completely rewrite the mechanism [Eduardo] > > 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(-) > > 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 <http://www.gnu.org/licenses/> > */ > #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_internal.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 index) > return !!(tab[i] & mask); > } > > +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) > } > > #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 = "apic"; > > if (kvm_apic_in_kernel()) { > @@ -2856,7 +2855,16 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp) > apic_type = "xen-apic"; > } > > - cpu->apic_state = 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 = OBJECT_CLASS(apic_get_class()); > + > + assert(apic_object_class); > + cpu->apic_state = 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). > > object_property_add_child(OBJECT(cpu), "lapic", > OBJECT(cpu->apic_state), &error_abort); > -- > 2.10.0 >
On Mon, 3 Oct 2016 13:03:33 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Fri, Sep 30, 2016 at 06:10:06PM +0200, Radim Krčmář wrote: > > Every configuration has only up to one APIC class and we'll be extending > > the class with a function that can be called without an instanced > > object, so a direct access to the class is convenient. > > > > This patch will break compilation if some code uses apic_get_class() > > with CONFIG_USER_ONLY. > > > > Suggested-by: Eduardo Habkost <ehabkost@redhat.com> > > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > > --- > > v2: assert() instead of error_report() and exit() [Peter] > > v3: completely rewrite the mechanism [Eduardo] > > > > 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(-) > > > > 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 <http://www.gnu.org/licenses/> > > */ > > #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_internal.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 index) > > return !!(tab[i] & mask); > > } > > > > +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) > > } > > > > #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 = "apic"; > > > > if (kvm_apic_in_kernel()) { > > @@ -2856,7 +2855,16 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp) > > apic_type = "xen-apic"; > > } > > > > - cpu->apic_state = 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 = OBJECT_CLASS(apic_get_class()); > > + > > + assert(apic_object_class); > > + cpu->apic_state = DEVICE(object_new_with_type(apic_object_class->type)); [...] > (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). That's probably not worth all the boilerplate code it would bring. > > > > > object_property_add_child(OBJECT(cpu), "lapic", > > OBJECT(cpu->apic_state), &error_abort); > > -- > > 2.10.0 > > >
2016-10-03 13:03-0300, Eduardo Habkost: > On Fri, Sep 30, 2016 at 06:10:06PM +0200, Radim Krčmář wrote: >> Every configuration has only up to one APIC class and we'll be extending >> the class with a function that can be called without an instanced >> object, so a direct access to the class is convenient. >> >> This patch will break compilation if some code uses apic_get_class() >> with CONFIG_USER_ONLY. >> >> Suggested-by: Eduardo Habkost <ehabkost@redhat.com> >> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> >> --- >> v2: assert() instead of error_report() and exit() [Peter] >> v3: completely rewrite the mechanism [Eduardo] >> >> 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(-) >> >> 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, Error **errp) >> apic_type = "xen-apic"; >> } >> >> - cpu->apic_state = 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 = OBJECT_CLASS(apic_get_class()); >> + >> + assert(apic_object_class); >> + cpu->apic_state = 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)). 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 ...
On Tue, Oct 04, 2016 at 03:38:01PM +0200, Radim Krčmář wrote: > 2016-10-03 13:03-0300, Eduardo Habkost: > > On Fri, Sep 30, 2016 at 06:10:06PM +0200, Radim Krčmář wrote: [...] > >> +static void x86_cpu_apic_create(X86CPU *cpu, Error **errp) > >> +{ > >> + APICCommonState *apic; > >> + ObjectClass *apic_object_class = OBJECT_CLASS(apic_get_class()); > >> + > >> + assert(apic_object_class); > >> + cpu->apic_state = 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)). > > 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. True. > > 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? I think that would be welcome. I don't know why it doesn't exist yet.
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 <http://www.gnu.org/licenses/> */ #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_internal.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 index) return !!(tab[i] & mask); } +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) } #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 = "apic"; if (kvm_apic_in_kernel()) { @@ -2856,7 +2855,16 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp) apic_type = "xen-apic"; } - cpu->apic_state = 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 = OBJECT_CLASS(apic_get_class()); + + assert(apic_object_class); + cpu->apic_state = DEVICE(object_new_with_type(apic_object_class->type)); object_property_add_child(OBJECT(cpu), "lapic", OBJECT(cpu->apic_state), &error_abort);
Every configuration has only up to one APIC class and we'll be extending the class with a function that can be called without an instanced object, so a direct access to the class is convenient. This patch will break compilation if some code uses apic_get_class() with CONFIG_USER_ONLY. Suggested-by: Eduardo Habkost <ehabkost@redhat.com> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> --- v2: assert() instead of error_report() and exit() [Peter] v3: completely rewrite the mechanism [Eduardo] 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(-)