[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 1/8] apic: add global apic_get_class()
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH v3 1/8] apic: add global apic_get_class() |
Date: |
Mon, 3 Oct 2016 13:03:33 -0300 |
User-agent: |
Mutt/1.7.0 (2016-08-17) |
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 <address@hidden>
> Signed-off-by: Radim Krčmář <address@hidden>
> ---
> 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
>
--
Eduardo
- Re: [Qemu-devel] [PATCH v3 1/8] apic: add global apic_get_class(),
Eduardo Habkost <=