Re: [Qemu-devel] [PATCH v3 1/8] apic: add global apic_get_class()

["Radim Krčmář" <rkrcmar@redhat.com>]

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 ...