* [Qemu-devel] [PATCH] target-i386: initialize APIC at CPU level
@ 2012-10-04 14:43 Igor Mammedov
2012-10-04 16:15 ` Andreas Färber
2012-10-04 16:44 ` Jan Kiszka
0 siblings, 2 replies; 6+ messages in thread
From: Igor Mammedov @ 2012-10-04 14:43 UTC (permalink / raw)
To: qemu-devel; +Cc: aliguori, ehabkost, gleb, Don, jan.kiszka, afaerber
(L)APIC is a part of cpu [1] so move APIC initialization inside of
x86_cpu object. Since cpu_model and override flags currently specify
whether APIC should be created or not, APIC creation&initialization is
moved into x86_cpu_apic_init() which is called from x86_cpu_realize().
[1] - all x86 cpus have integrated APIC if we overlook existence of i486,
and it's more convenient to model after majority of them.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
* init APIC mapping at cpu level, due to Peter's objection to putting
it into APIC's initfn and Jan's suggestion to do it inside cpu.
v3:
* create APIC at realize time
* rebased on top of current qemu tree
* whitespace fix
* ifdef only body of x86_cpu_apic_init()
Git tree for testing:
https://github.com/imammedo/qemu/tree/apic_in_cpu
---
hw/pc.c | 56 +++++------------------------------------------------
target-i386/cpu.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 63 insertions(+), 51 deletions(-)
diff --git a/hw/pc.c b/hw/pc.c
index 7e7e0e2..f4c0579 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -72,8 +72,6 @@
#define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
#define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
-#define MSI_ADDR_BASE 0xfee00000
-
#define E820_NR_ENTRIES 16
struct e820_entry {
@@ -847,35 +845,6 @@ DeviceState *cpu_get_current_apic(void)
}
}
-static DeviceState *apic_init(void *env, uint8_t apic_id)
-{
- DeviceState *dev;
- static int apic_mapped;
-
- if (kvm_irqchip_in_kernel()) {
- dev = qdev_create(NULL, "kvm-apic");
- } else if (xen_enabled()) {
- dev = qdev_create(NULL, "xen-apic");
- } else {
- dev = qdev_create(NULL, "apic");
- }
-
- qdev_prop_set_uint8(dev, "id", apic_id);
- qdev_prop_set_ptr(dev, "cpu_env", env);
- qdev_init_nofail(dev);
-
- /* XXX: mapping more APICs at the same memory location */
- if (apic_mapped == 0) {
- /* NOTE: the APIC is directly connected to the CPU - it is not
- on the global memory bus. */
- /* XXX: what if the base changes? */
- sysbus_mmio_map(sysbus_from_qdev(dev), 0, MSI_ADDR_BASE);
- apic_mapped = 1;
- }
-
- return dev;
-}
-
void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
{
CPUX86State *s = opaque;
@@ -885,24 +854,6 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
}
}
-static X86CPU *pc_new_cpu(const char *cpu_model)
-{
- X86CPU *cpu;
- CPUX86State *env;
-
- cpu = cpu_x86_init(cpu_model);
- if (cpu == NULL) {
- fprintf(stderr, "Unable to find x86 CPU definition\n");
- exit(1);
- }
- env = &cpu->env;
- if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
- env->apic_state = apic_init(env, env->cpuid_apic_id);
- }
- cpu_reset(CPU(cpu));
- return cpu;
-}
-
void pc_cpus_init(const char *cpu_model)
{
int i;
@@ -916,8 +867,11 @@ void pc_cpus_init(const char *cpu_model)
#endif
}
- for(i = 0; i < smp_cpus; i++) {
- pc_new_cpu(cpu_model);
+ for (i = 0; i < smp_cpus; i++) {
+ if (!cpu_x86_init(cpu_model)) {
+ fprintf(stderr, "Unable to find x86 CPU definition\n");
+ exit(1);
+ }
}
}
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index bb1e44e..612ed5a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -37,6 +37,12 @@
#include <linux/kvm_para.h>
#endif
+#include "sysemu.h"
+#include "hw/xen.h"
+#ifndef CONFIG_USER_ONLY
+#include "hw/sysbus.h"
+#endif
+
/* feature flags taken from "Intel Processor Identification and the CPUID
* Instruction" and AMD's "CPUID Specification". In cases of disagreement
* between feature naming conventions, aliases may be added.
@@ -1870,6 +1876,56 @@ static void mce_init(X86CPU *cpu)
}
}
+#define MSI_ADDR_BASE 0xfee00000
+
+static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
+{
+#ifndef CONFIG_USER_ONLY
+ static int apic_mapped;
+ CPUX86State *env = &cpu->env;
+ const char *apic_type = "apic";
+
+ if (!(env->cpuid_features & CPUID_APIC || smp_cpus > 1)) {
+ return;
+ }
+
+ if (kvm_irqchip_in_kernel()) {
+ apic_type = "kvm-apic";
+ } else if (xen_enabled()) {
+ apic_type = "xen-apic";
+ }
+ env->apic_state = qdev_create(NULL, apic_type);
+
+ if (env->apic_state == NULL) {
+ error_set(errp, QERR_DEVICE_INIT_FAILED, apic_type);
+ return;
+ }
+
+ object_property_add_child(OBJECT(cpu), "apic",
+ OBJECT(env->apic_state), NULL);
+ qdev_prop_set_uint8(env->apic_state, "id", env->cpuid_apic_id);
+ /* TODO: convert to link<> */
+ qdev_prop_set_ptr(env->apic_state, "cpu_env", env);
+
+ if (qdev_init(env->apic_state)) {
+ error_set(errp, QERR_DEVICE_INIT_FAILED,
+ object_get_typename(OBJECT(env->apic_state)));
+ return;
+ }
+
+ /* XXX: mapping more APICs at the same memory location */
+ if (apic_mapped == 0) {
+ /* NOTE: the APIC is directly connected to the CPU - it is not
+ on the global memory bus. */
+ /* XXX: what if the base changes? */
+ sysbus_mmio_map(sysbus_from_qdev(env->apic_state), 0, MSI_ADDR_BASE);
+ apic_mapped = 1;
+ }
+
+ return;
+#endif
+}
+
void x86_cpu_realize(Object *obj, Error **errp)
{
X86CPU *cpu = X86_CPU(obj);
@@ -1878,6 +1934,8 @@ void x86_cpu_realize(Object *obj, Error **errp)
qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
#endif
+ x86_cpu_apic_init(cpu, errp);
+
mce_init(cpu);
qemu_init_vcpu(&cpu->env);
cpu_reset(CPU(cpu));
--
1.7.11.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] target-i386: initialize APIC at CPU level
2012-10-04 14:43 [Qemu-devel] [PATCH] target-i386: initialize APIC at CPU level Igor Mammedov
@ 2012-10-04 16:15 ` Andreas Färber
2012-10-04 16:31 ` Igor Mammedov
2012-10-05 15:32 ` Igor Mammedov
2012-10-04 16:44 ` Jan Kiszka
1 sibling, 2 replies; 6+ messages in thread
From: Andreas Färber @ 2012-10-04 16:15 UTC (permalink / raw)
To: Igor Mammedov; +Cc: aliguori, ehabkost, gleb, Don, qemu-devel, jan.kiszka
Am 04.10.2012 16:43, schrieb Igor Mammedov:
> (L)APIC is a part of cpu [1] so move APIC initialization inside of
> x86_cpu object. Since cpu_model and override flags currently specify
> whether APIC should be created or not, APIC creation&initialization is
> moved into x86_cpu_apic_init() which is called from x86_cpu_realize().
>
> [1] - all x86 cpus have integrated APIC if we overlook existence of i486,
> and it's more convenient to model after majority of them.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v2:
> * init APIC mapping at cpu level, due to Peter's objection to putting
> it into APIC's initfn and Jan's suggestion to do it inside cpu.
> v3:
> * create APIC at realize time
> * rebased on top of current qemu tree
> * whitespace fix
> * ifdef only body of x86_cpu_apic_init()
>
> Git tree for testing:
> https://github.com/imammedo/qemu/tree/apic_in_cpu
> ---
> hw/pc.c | 56 +++++------------------------------------------------
> target-i386/cpu.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 63 insertions(+), 51 deletions(-)
>
> diff --git a/hw/pc.c b/hw/pc.c
> index 7e7e0e2..f4c0579 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -72,8 +72,6 @@
> #define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
> #define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
>
> -#define MSI_ADDR_BASE 0xfee00000
> -
> #define E820_NR_ENTRIES 16
>
> struct e820_entry {
> @@ -847,35 +845,6 @@ DeviceState *cpu_get_current_apic(void)
> }
> }
>
> -static DeviceState *apic_init(void *env, uint8_t apic_id)
> -{
> - DeviceState *dev;
> - static int apic_mapped;
> -
> - if (kvm_irqchip_in_kernel()) {
> - dev = qdev_create(NULL, "kvm-apic");
> - } else if (xen_enabled()) {
> - dev = qdev_create(NULL, "xen-apic");
> - } else {
> - dev = qdev_create(NULL, "apic");
> - }
> -
> - qdev_prop_set_uint8(dev, "id", apic_id);
> - qdev_prop_set_ptr(dev, "cpu_env", env);
> - qdev_init_nofail(dev);
> -
> - /* XXX: mapping more APICs at the same memory location */
> - if (apic_mapped == 0) {
> - /* NOTE: the APIC is directly connected to the CPU - it is not
> - on the global memory bus. */
> - /* XXX: what if the base changes? */
> - sysbus_mmio_map(sysbus_from_qdev(dev), 0, MSI_ADDR_BASE);
> - apic_mapped = 1;
> - }
> -
> - return dev;
> -}
> -
> void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
> {
> CPUX86State *s = opaque;
> @@ -885,24 +854,6 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
> }
> }
>
> -static X86CPU *pc_new_cpu(const char *cpu_model)
> -{
> - X86CPU *cpu;
> - CPUX86State *env;
> -
> - cpu = cpu_x86_init(cpu_model);
> - if (cpu == NULL) {
> - fprintf(stderr, "Unable to find x86 CPU definition\n");
> - exit(1);
> - }
> - env = &cpu->env;
> - if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
> - env->apic_state = apic_init(env, env->cpuid_apic_id);
> - }
> - cpu_reset(CPU(cpu));
> - return cpu;
> -}
> -
> void pc_cpus_init(const char *cpu_model)
> {
> int i;
> @@ -916,8 +867,11 @@ void pc_cpus_init(const char *cpu_model)
> #endif
> }
>
> - for(i = 0; i < smp_cpus; i++) {
> - pc_new_cpu(cpu_model);
> + for (i = 0; i < smp_cpus; i++) {
> + if (!cpu_x86_init(cpu_model)) {
> + fprintf(stderr, "Unable to find x86 CPU definition\n");
> + exit(1);
> + }
> }
> }
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index bb1e44e..612ed5a 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -37,6 +37,12 @@
> #include <linux/kvm_para.h>
> #endif
>
> +#include "sysemu.h"
> +#include "hw/xen.h"
Could also go into the #ifndef below.
> +#ifndef CONFIG_USER_ONLY
> +#include "hw/sysbus.h"
> +#endif
> +
> /* feature flags taken from "Intel Processor Identification and the CPUID
> * Instruction" and AMD's "CPUID Specification". In cases of disagreement
> * between feature naming conventions, aliases may be added.
> @@ -1870,6 +1876,56 @@ static void mce_init(X86CPU *cpu)
> }
> }
>
> +#define MSI_ADDR_BASE 0xfee00000
> +
> +static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
> +{
> +#ifndef CONFIG_USER_ONLY
> + static int apic_mapped;
> + CPUX86State *env = &cpu->env;
> + const char *apic_type = "apic";
> +
> + if (!(env->cpuid_features & CPUID_APIC || smp_cpus > 1)) {
> + return;
> + }
I would prefer to keep the original logic at the call site
(x86_cpu_initfn) rather than silently returning without setting errp.
> +
> + if (kvm_irqchip_in_kernel()) {
> + apic_type = "kvm-apic";
> + } else if (xen_enabled()) {
> + apic_type = "xen-apic";
> + }
> + env->apic_state = qdev_create(NULL, apic_type);
> +
> + if (env->apic_state == NULL) {
> + error_set(errp, QERR_DEVICE_INIT_FAILED, apic_type);
> + return;
> + }
> +
> + object_property_add_child(OBJECT(cpu), "apic",
> + OBJECT(env->apic_state), NULL);
> + qdev_prop_set_uint8(env->apic_state, "id", env->cpuid_apic_id);
> + /* TODO: convert to link<> */
> + qdev_prop_set_ptr(env->apic_state, "cpu_env", env);
Do we still need env->apic_state and a property on the APIC at all? From
the X86CPU side we can access the "apic" property to retrieve the
pointer, and from the APIC we should be able to navigate to its parent,
right?
> +
> + if (qdev_init(env->apic_state)) {
> + error_set(errp, QERR_DEVICE_INIT_FAILED,
> + object_get_typename(OBJECT(env->apic_state)));
> + return;
> + }
> +
> + /* XXX: mapping more APICs at the same memory location */
> + if (apic_mapped == 0) {
> + /* NOTE: the APIC is directly connected to the CPU - it is not
> + on the global memory bus. */
> + /* XXX: what if the base changes? */
> + sysbus_mmio_map(sysbus_from_qdev(env->apic_state), 0, MSI_ADDR_BASE);
> + apic_mapped = 1;
> + }
> +
> + return;
> +#endif
> +}
> +
> void x86_cpu_realize(Object *obj, Error **errp)
> {
> X86CPU *cpu = X86_CPU(obj);
> @@ -1878,6 +1934,8 @@ void x86_cpu_realize(Object *obj, Error **errp)
> qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
> #endif
>
> + x86_cpu_apic_init(cpu, errp);
if (error_is_set(errp)) {
return;
}
?
> +
> mce_init(cpu);
> qemu_init_vcpu(&cpu->env);
> cpu_reset(CPU(cpu));
>
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] target-i386: initialize APIC at CPU level
2012-10-04 16:15 ` Andreas Färber
@ 2012-10-04 16:31 ` Igor Mammedov
2012-10-04 16:47 ` Andreas Färber
2012-10-05 15:32 ` Igor Mammedov
1 sibling, 1 reply; 6+ messages in thread
From: Igor Mammedov @ 2012-10-04 16:31 UTC (permalink / raw)
To: Andreas Färber; +Cc: aliguori, ehabkost, gleb, Don, qemu-devel, jan.kiszka
On Thu, 04 Oct 2012 18:15:48 +0200
Andreas Färber <afaerber@suse.de> wrote:
> Am 04.10.2012 16:43, schrieb Igor Mammedov:
> > (L)APIC is a part of cpu [1] so move APIC initialization inside of
> > x86_cpu object. Since cpu_model and override flags currently specify
> > whether APIC should be created or not, APIC creation&initialization is
> > moved into x86_cpu_apic_init() which is called from x86_cpu_realize().
> >
> > [1] - all x86 cpus have integrated APIC if we overlook existence of i486,
> > and it's more convenient to model after majority of them.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v2:
> > * init APIC mapping at cpu level, due to Peter's objection to putting
> > it into APIC's initfn and Jan's suggestion to do it inside cpu.
> > v3:
> > * create APIC at realize time
> > * rebased on top of current qemu tree
> > * whitespace fix
> > * ifdef only body of x86_cpu_apic_init()
> >
> > Git tree for testing:
> > https://github.com/imammedo/qemu/tree/apic_in_cpu
> > ---
> > hw/pc.c | 56
> > +++++------------------------------------------------ target-i386/cpu.c |
> > 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files
> > changed, 63 insertions(+), 51 deletions(-)
> >
> > diff --git a/hw/pc.c b/hw/pc.c
> > index 7e7e0e2..f4c0579 100644
> > --- a/hw/pc.c
> > +++ b/hw/pc.c
> > @@ -72,8 +72,6 @@
> > #define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
> > #define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
> >
> > -#define MSI_ADDR_BASE 0xfee00000
> > -
> > #define E820_NR_ENTRIES 16
> >
> > struct e820_entry {
> > @@ -847,35 +845,6 @@ DeviceState *cpu_get_current_apic(void)
> > }
> > }
> >
> > -static DeviceState *apic_init(void *env, uint8_t apic_id)
> > -{
> > - DeviceState *dev;
> > - static int apic_mapped;
> > -
> > - if (kvm_irqchip_in_kernel()) {
> > - dev = qdev_create(NULL, "kvm-apic");
> > - } else if (xen_enabled()) {
> > - dev = qdev_create(NULL, "xen-apic");
> > - } else {
> > - dev = qdev_create(NULL, "apic");
> > - }
> > -
> > - qdev_prop_set_uint8(dev, "id", apic_id);
> > - qdev_prop_set_ptr(dev, "cpu_env", env);
> > - qdev_init_nofail(dev);
> > -
> > - /* XXX: mapping more APICs at the same memory location */
> > - if (apic_mapped == 0) {
> > - /* NOTE: the APIC is directly connected to the CPU - it is not
> > - on the global memory bus. */
> > - /* XXX: what if the base changes? */
> > - sysbus_mmio_map(sysbus_from_qdev(dev), 0, MSI_ADDR_BASE);
> > - apic_mapped = 1;
> > - }
> > -
> > - return dev;
> > -}
> > -
> > void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
> > {
> > CPUX86State *s = opaque;
> > @@ -885,24 +854,6 @@ void pc_acpi_smi_interrupt(void *opaque, int irq,
> > int level) }
> > }
> >
> > -static X86CPU *pc_new_cpu(const char *cpu_model)
> > -{
> > - X86CPU *cpu;
> > - CPUX86State *env;
> > -
> > - cpu = cpu_x86_init(cpu_model);
> > - if (cpu == NULL) {
> > - fprintf(stderr, "Unable to find x86 CPU definition\n");
> > - exit(1);
> > - }
> > - env = &cpu->env;
> > - if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
> > - env->apic_state = apic_init(env, env->cpuid_apic_id);
> > - }
> > - cpu_reset(CPU(cpu));
> > - return cpu;
> > -}
> > -
> > void pc_cpus_init(const char *cpu_model)
> > {
> > int i;
> > @@ -916,8 +867,11 @@ void pc_cpus_init(const char *cpu_model)
> > #endif
> > }
> >
> > - for(i = 0; i < smp_cpus; i++) {
> > - pc_new_cpu(cpu_model);
> > + for (i = 0; i < smp_cpus; i++) {
> > + if (!cpu_x86_init(cpu_model)) {
> > + fprintf(stderr, "Unable to find x86 CPU definition\n");
> > + exit(1);
> > + }
> > }
> > }
> >
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index bb1e44e..612ed5a 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -37,6 +37,12 @@
> > #include <linux/kvm_para.h>
> > #endif
> >
> > +#include "sysemu.h"
> > +#include "hw/xen.h"
>
> Could also go into the #ifndef below.
sure
>
> > +#ifndef CONFIG_USER_ONLY
> > +#include "hw/sysbus.h"
> > +#endif
> > +
> > /* feature flags taken from "Intel Processor Identification and the CPUID
> > * Instruction" and AMD's "CPUID Specification". In cases of
> > disagreement
> > * between feature naming conventions, aliases may be added.
> > @@ -1870,6 +1876,56 @@ static void mce_init(X86CPU *cpu)
> > }
> > }
> >
> > +#define MSI_ADDR_BASE 0xfee00000
> > +
> > +static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
> > +{
> > +#ifndef CONFIG_USER_ONLY
> > + static int apic_mapped;
> > + CPUX86State *env = &cpu->env;
> > + const char *apic_type = "apic";
> > +
> > + if (!(env->cpuid_features & CPUID_APIC || smp_cpus > 1)) {
> > + return;
> > + }
>
> I would prefer to keep the original logic at the call site
> (x86_cpu_initfn)
Could you explain it bit more?
> rather than silently returning without setting errp.
and this one too, pls.
>
> > +
> > + if (kvm_irqchip_in_kernel()) {
> > + apic_type = "kvm-apic";
> > + } else if (xen_enabled()) {
> > + apic_type = "xen-apic";
> > + }
> > + env->apic_state = qdev_create(NULL, apic_type);
> > +
> > + if (env->apic_state == NULL) {
> > + error_set(errp, QERR_DEVICE_INIT_FAILED, apic_type);
> > + return;
> > + }
> > +
> > + object_property_add_child(OBJECT(cpu), "apic",
> > + OBJECT(env->apic_state), NULL);
> > + qdev_prop_set_uint8(env->apic_state, "id", env->cpuid_apic_id);
> > + /* TODO: convert to link<> */
> > + qdev_prop_set_ptr(env->apic_state, "cpu_env", env);
>
> Do we still need env->apic_state and a property on the APIC at all? From
> the X86CPU side we can access the "apic" property to retrieve the
> pointer, and from the APIC we should be able to navigate to its parent,
> right?
I'll look at this.
>
> > +
> > + if (qdev_init(env->apic_state)) {
> > + error_set(errp, QERR_DEVICE_INIT_FAILED,
> > + object_get_typename(OBJECT(env->apic_state)));
> > + return;
> > + }
> > +
> > + /* XXX: mapping more APICs at the same memory location */
> > + if (apic_mapped == 0) {
> > + /* NOTE: the APIC is directly connected to the CPU - it is not
> > + on the global memory bus. */
> > + /* XXX: what if the base changes? */
> > + sysbus_mmio_map(sysbus_from_qdev(env->apic_state), 0,
> > MSI_ADDR_BASE);
> > + apic_mapped = 1;
> > + }
> > +
> > + return;
> > +#endif
> > +}
> > +
> > void x86_cpu_realize(Object *obj, Error **errp)
> > {
> > X86CPU *cpu = X86_CPU(obj);
> > @@ -1878,6 +1934,8 @@ void x86_cpu_realize(Object *obj, Error **errp)
> > qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
> > #endif
> >
> > + x86_cpu_apic_init(cpu, errp);
>
> if (error_is_set(errp)) {
> return;
> }
> ?
Something like this patch was in CPU properties series. I'll include it here
as well.
>
> > +
> > mce_init(cpu);
> > qemu_init_vcpu(&cpu->env);
> > cpu_reset(CPU(cpu));
> >
>
> Andreas
>
Thanks,
Igor
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] target-i386: initialize APIC at CPU level
2012-10-04 16:31 ` Igor Mammedov
@ 2012-10-04 16:47 ` Andreas Färber
0 siblings, 0 replies; 6+ messages in thread
From: Andreas Färber @ 2012-10-04 16:47 UTC (permalink / raw)
To: Igor Mammedov; +Cc: aliguori, ehabkost, gleb, Don, qemu-devel, jan.kiszka
Am 04.10.2012 18:31, schrieb Igor Mammedov:
> On Thu, 04 Oct 2012 18:15:48 +0200
> Andreas Färber <afaerber@suse.de> wrote:
>
>> Am 04.10.2012 16:43, schrieb Igor Mammedov:
>>> +static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
>>> +{
>>> +#ifndef CONFIG_USER_ONLY
>>> + static int apic_mapped;
>>> + CPUX86State *env = &cpu->env;
>>> + const char *apic_type = "apic";
>>> +
>>> + if (!(env->cpuid_features & CPUID_APIC || smp_cpus > 1)) {
>>> + return;
>>> + }
>>
>> I would prefer to keep the original logic at the call site
>> (x86_cpu_initfn)
> Could you explain it bit more?
>
>> rather than silently returning without setting errp.
> and this one too, pls.
Sorry. What I mean here is don't let the function return without setting
an error condition if it didn't do anything (I consider that dangerous
since it potentially hides errors). Therefore only call it when we
actually do want the APIC (condition seems trivial enough) and then
signal an error if something is wrong, so that it's detectable:
>>> @@ -1878,6 +1934,8 @@ void x86_cpu_realize(Object *obj, Error **errp)
>>> qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
>>> #endif
>>>
if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
>>> + x86_cpu_apic_init(cpu, errp);
>>
>> if (error_is_set(errp)) {
>> return;
>> }
}
If we now truely signal errors and not only try to follow the realizefn
convention, you should also check the callers of x86_cpu_realize() to
make sure errp is not NULL and is being checked and printed after the call.
Regards,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] target-i386: initialize APIC at CPU level
2012-10-04 16:15 ` Andreas Färber
2012-10-04 16:31 ` Igor Mammedov
@ 2012-10-05 15:32 ` Igor Mammedov
1 sibling, 0 replies; 6+ messages in thread
From: Igor Mammedov @ 2012-10-05 15:32 UTC (permalink / raw)
To: Andreas Färber; +Cc: aliguori, ehabkost, gleb, Don, qemu-devel, jan.kiszka
On Thu, 04 Oct 2012 18:15:48 +0200
Andreas Färber <afaerber@suse.de> wrote:
> > + env->apic_state = qdev_create(NULL, apic_type);
> > +
> > + if (env->apic_state == NULL) {
> > + error_set(errp, QERR_DEVICE_INIT_FAILED, apic_type);
> > + return;
> > + }
> > +
> > + object_property_add_child(OBJECT(cpu), "apic",
> > + OBJECT(env->apic_state), NULL);
> > + qdev_prop_set_uint8(env->apic_state, "id", env->cpuid_apic_id);
> > + /* TODO: convert to link<> */
> > + qdev_prop_set_ptr(env->apic_state, "cpu_env", env);
>
> Do we still need env->apic_state and a property on the APIC at all? From
> the X86CPU side we can access the "apic" property to retrieve the
> pointer,
It would introduce too much overhead. For test I've just substituted all uses
of env->apic_state with a func that could serve as get_child(name)
+DeviceState *get_apic(CPUX86State *env) {
+ X86CPU *cpu = x86_env_get_cpu(env);
+ Object *o = object_resolve_path_component(OBJECT(cpu), (gchar *)"apic");
+ DeviceState *dev = OBJECT_CHECK(DeviceState, o, "device");
+ return dev;
+}
and run qemu under valgrind --tool=callgrind, booting openbsd for 5 min.
hot path get_apic() call count get_apic() incl cost.
TCG: pic_irq_request() 7M 44 (8th from top 10)
KVM: kvm_arch_post_run() 2M 38 (6th from top 10)
NO PATCH: pic_irq_request() 18M 0.8
pic_irq_request() calls get_apic() 3x, kvm_arch_post_run() - 2x times
I suppose calling get_apic() only once on hot path could reduce amount of
calls but property searching and dynamic cast won't disappear any way.
Looks like it's better to leave apic_state as it is.
> and from the APIC we should be able to navigate to its parent,
> right?
while adding object_get_parent(), I've noticed in object.h
"
* There is no way for a child to determine what its parent is. It is not
* a bidirectional relationship. This is by design.
"
Would be it acceptable to allow child access to parent?
...
>
> Andreas
>
Thanks,
Igor
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] target-i386: initialize APIC at CPU level
2012-10-04 14:43 [Qemu-devel] [PATCH] target-i386: initialize APIC at CPU level Igor Mammedov
2012-10-04 16:15 ` Andreas Färber
@ 2012-10-04 16:44 ` Jan Kiszka
1 sibling, 0 replies; 6+ messages in thread
From: Jan Kiszka @ 2012-10-04 16:44 UTC (permalink / raw)
To: Igor Mammedov; +Cc: aliguori, ehabkost, gleb, Don, qemu-devel, afaerber
On 2012-10-04 16:43, Igor Mammedov wrote:
> + /* XXX: mapping more APICs at the same memory location */
> + if (apic_mapped == 0) {
> + /* NOTE: the APIC is directly connected to the CPU - it is not
> + on the global memory bus. */
> + /* XXX: what if the base changes? */
> + sysbus_mmio_map(sysbus_from_qdev(env->apic_state), 0, MSI_ADDR_BASE);
> + apic_mapped = 1;
> + }
> +
> + return;
Unneeded return.
Jan
> +#endif
> +}
> +
> void x86_cpu_realize(Object *obj, Error **errp)
> {
> X86CPU *cpu = X86_CPU(obj);
> @@ -1878,6 +1934,8 @@ void x86_cpu_realize(Object *obj, Error **errp)
> qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
> #endif
>
> + x86_cpu_apic_init(cpu, errp);
> +
> mce_init(cpu);
> qemu_init_vcpu(&cpu->env);
> cpu_reset(CPU(cpu));
>
--
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-10-05 15:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-04 14:43 [Qemu-devel] [PATCH] target-i386: initialize APIC at CPU level Igor Mammedov
2012-10-04 16:15 ` Andreas Färber
2012-10-04 16:31 ` Igor Mammedov
2012-10-04 16:47 ` Andreas Färber
2012-10-05 15:32 ` Igor Mammedov
2012-10-04 16:44 ` Jan Kiszka
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).