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 <ehabk...@redhat.com>
>> Signed-off-by: Radim Krčmář <rkrc...@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 ...
