[v3,1/8] apic: add global apic_get_class()

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(-)

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.