| Message ID | 1340197164-9574-4-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 20.06.2012 14:59, schrieb Igor Mammedov: > It's not correct to make CPU runnable (i.e. calling x86_cpu_realize()) > when not all properties are set (APIC in this case). > > Fix it by calling x86_cpu_realize() at board level after APIC is > initialized, right before cpu_reset(). > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/pc.c | 1 + > target-i386/helper.c | 2 -- > 2 files changed, 1 insertion(+), 2 deletions(-) > > diff --git a/hw/pc.c b/hw/pc.c > index 8368701..8a662cf 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -948,6 +948,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model) > env->apic_state = apic_init(env, env->cpuid_apic_id); > } > qemu_register_reset(pc_cpu_reset, cpu); > + x86_cpu_realize(OBJECT(cpu), NULL); > pc_cpu_reset(cpu); > return cpu; > } > diff --git a/target-i386/helper.c b/target-i386/helper.c > index c52ec13..b38ea7f 100644 > --- a/target-i386/helper.c > +++ b/target-i386/helper.c > @@ -1161,8 +1161,6 @@ X86CPU *cpu_x86_init(const char *cpu_model) > return NULL; > } > > - x86_cpu_realize(OBJECT(cpu), NULL); > - > return cpu; > } > This will require changes in linux-user and possibly bsd-user. Having a cpu_realize() would probably help with avoiding #ifdef'ery. Unfortunately deriving CPUState from DeviceState proves a bit difficult in the meantime (it worked at one point, now there's lots of circular header dependencies), and realize support for Object got stopped. Andreas
On 06/20/2012 03:35 PM, Andreas Färber wrote: > Am 20.06.2012 14:59, schrieb Igor Mammedov: >> It's not correct to make CPU runnable (i.e. calling x86_cpu_realize()) >> when not all properties are set (APIC in this case). >> >> Fix it by calling x86_cpu_realize() at board level after APIC is >> initialized, right before cpu_reset(). >> >> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >> --- >> hw/pc.c | 1 + >> target-i386/helper.c | 2 -- >> 2 files changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/hw/pc.c b/hw/pc.c >> index 8368701..8a662cf 100644 >> --- a/hw/pc.c >> +++ b/hw/pc.c >> @@ -948,6 +948,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model) >> env->apic_state = apic_init(env, env->cpuid_apic_id); >> } >> qemu_register_reset(pc_cpu_reset, cpu); >> + x86_cpu_realize(OBJECT(cpu), NULL); >> pc_cpu_reset(cpu); >> return cpu; >> } >> diff --git a/target-i386/helper.c b/target-i386/helper.c >> index c52ec13..b38ea7f 100644 >> --- a/target-i386/helper.c >> +++ b/target-i386/helper.c >> @@ -1161,8 +1161,6 @@ X86CPU *cpu_x86_init(const char *cpu_model) >> return NULL; >> } >> >> - x86_cpu_realize(OBJECT(cpu), NULL); >> - >> return cpu; >> } >> > > This will require changes in linux-user and possibly bsd-user. Having a Why it would require changes in linux-user? So far x86_cpu_realize() does nothing useful in linux-user, compiled and tested. It should be harmless for linux-user not to execute it. But I haven't compiled/tested bsd-user, do I need BSD for this? > cpu_realize() would probably help with avoiding #ifdef'ery. > Unfortunately deriving CPUState from DeviceState proves a bit difficult > in the meantime (it worked at one point, now there's lots of circular > header dependencies), and realize support for Object got stopped. I'm in process of untangling this header mayhem (at least to a point that allows compilation complete when CPU is derived from Device) > > Andreas >
Am 21.06.2012 11:43, schrieb Igor Mammedov: > On 06/20/2012 03:35 PM, Andreas Färber wrote: >> Am 20.06.2012 14:59, schrieb Igor Mammedov: >>> It's not correct to make CPU runnable (i.e. calling x86_cpu_realize()) >>> when not all properties are set (APIC in this case). >>> >>> Fix it by calling x86_cpu_realize() at board level after APIC is >>> initialized, right before cpu_reset(). >>> >>> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >>> --- >>> hw/pc.c | 1 + >>> target-i386/helper.c | 2 -- >>> 2 files changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/hw/pc.c b/hw/pc.c >>> index 8368701..8a662cf 100644 >>> --- a/hw/pc.c >>> +++ b/hw/pc.c >>> @@ -948,6 +948,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model) >>> env->apic_state = apic_init(env, env->cpuid_apic_id); >>> } >>> qemu_register_reset(pc_cpu_reset, cpu); >>> + x86_cpu_realize(OBJECT(cpu), NULL); >>> pc_cpu_reset(cpu); >>> return cpu; >>> } >>> diff --git a/target-i386/helper.c b/target-i386/helper.c >>> index c52ec13..b38ea7f 100644 >>> --- a/target-i386/helper.c >>> +++ b/target-i386/helper.c >>> @@ -1161,8 +1161,6 @@ X86CPU *cpu_x86_init(const char *cpu_model) >>> return NULL; >>> } >>> >>> - x86_cpu_realize(OBJECT(cpu), NULL); >>> - >>> return cpu; >>> } >>> >> >> This will require changes in linux-user and possibly bsd-user. Having a > Why it would require changes in linux-user? So far x86_cpu_realize() does > nothing useful in linux-user, compiled and tested. It should be harmless > for linux-user not to execute it. Hm, I'd need to recheck... > But I haven't compiled/tested bsd-user, do I need BSD for this? Yes, you do. But if it's not needed for linux-user then it shouldn't be needed for bsd-user either. >> cpu_realize() would probably help with avoiding #ifdef'ery. >> Unfortunately deriving CPUState from DeviceState proves a bit difficult >> in the meantime (it worked at one point, now there's lots of circular >> header dependencies), and realize support for Object got stopped. > I'm in process of untangling this header mayhem (at least to a point > that allows compilation complete when CPU is derived from Device) So am I... A few weeks ago my qom-cpu-dev branch on GitHub used to compile, now something has changed and I needed to take cpu.h out of qemu-common.h and move lots of, e.g., ARM devices into libhw to avoid cpu.h dependencies and add cpu.h includes elsewhere. Would be good to coordinate that, are you on IRC later today? Andreas
On 06/21/2012 12:14 PM, Andreas Färber wrote: > Am 21.06.2012 11:43, schrieb Igor Mammedov: >> On 06/20/2012 03:35 PM, Andreas Färber wrote: >>> Am 20.06.2012 14:59, schrieb Igor Mammedov: >>>> It's not correct to make CPU runnable (i.e. calling x86_cpu_realize()) >>>> when not all properties are set (APIC in this case). >>>> >>>> Fix it by calling x86_cpu_realize() at board level after APIC is >>>> initialized, right before cpu_reset(). >>>> >>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >>>> --- >>>> hw/pc.c | 1 + >>>> target-i386/helper.c | 2 -- >>>> 2 files changed, 1 insertion(+), 2 deletions(-) >>>> >>>> diff --git a/hw/pc.c b/hw/pc.c >>>> index 8368701..8a662cf 100644 >>>> --- a/hw/pc.c >>>> +++ b/hw/pc.c >>>> @@ -948,6 +948,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model) >>>> env->apic_state = apic_init(env, env->cpuid_apic_id); >>>> } >>>> qemu_register_reset(pc_cpu_reset, cpu); >>>> + x86_cpu_realize(OBJECT(cpu), NULL); >>>> pc_cpu_reset(cpu); >>>> return cpu; >>>> } >>>> diff --git a/target-i386/helper.c b/target-i386/helper.c >>>> index c52ec13..b38ea7f 100644 >>>> --- a/target-i386/helper.c >>>> +++ b/target-i386/helper.c >>>> @@ -1161,8 +1161,6 @@ X86CPU *cpu_x86_init(const char *cpu_model) >>>> return NULL; >>>> } >>>> >>>> - x86_cpu_realize(OBJECT(cpu), NULL); >>>> - >>>> return cpu; >>>> } >>>> >>> >>> This will require changes in linux-user and possibly bsd-user. Having a >> Why it would require changes in linux-user? So far x86_cpu_realize() does >> nothing useful in linux-user, compiled and tested. It should be harmless >> for linux-user not to execute it. > > Hm, I'd need to recheck... > >> But I haven't compiled/tested bsd-user, do I need BSD for this? > > Yes, you do. But if it's not needed for linux-user then it shouldn't be > needed for bsd-user either. > >>> cpu_realize() would probably help with avoiding #ifdef'ery. >>> Unfortunately deriving CPUState from DeviceState proves a bit difficult >>> in the meantime (it worked at one point, now there's lots of circular >>> header dependencies), and realize support for Object got stopped. > >> I'm in process of untangling this header mayhem (at least to a point >> that allows compilation complete when CPU is derived from Device) > > So am I... A few weeks ago my qom-cpu-dev branch on GitHub used to > compile, now something has changed and I needed to take cpu.h out of > qemu-common.h and move lots of, e.g., ARM devices into libhw to avoid > cpu.h dependencies and add cpu.h includes elsewhere. Would be good to > coordinate that, are you on IRC later today? Sure, I'd like to. Could you send me connection details for "IRC"? I do not know where it is. > > Andreas >
On 06/20/2012 03:35 PM, Andreas Färber wrote: > Am 20.06.2012 14:59, schrieb Igor Mammedov: >> It's not correct to make CPU runnable (i.e. calling x86_cpu_realize()) >> when not all properties are set (APIC in this case). >> >> Fix it by calling x86_cpu_realize() at board level after APIC is >> initialized, right before cpu_reset(). >> >> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >> --- >> hw/pc.c | 1 + >> target-i386/helper.c | 2 -- >> 2 files changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/hw/pc.c b/hw/pc.c >> index 8368701..8a662cf 100644 >> --- a/hw/pc.c >> +++ b/hw/pc.c >> @@ -948,6 +948,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model) >> env->apic_state = apic_init(env, env->cpuid_apic_id); >> } >> qemu_register_reset(pc_cpu_reset, cpu); >> + x86_cpu_realize(OBJECT(cpu), NULL); >> pc_cpu_reset(cpu); >> return cpu; >> } >> diff --git a/target-i386/helper.c b/target-i386/helper.c >> index c52ec13..b38ea7f 100644 >> --- a/target-i386/helper.c >> +++ b/target-i386/helper.c >> @@ -1161,8 +1161,6 @@ X86CPU *cpu_x86_init(const char *cpu_model) >> return NULL; >> } >> >> - x86_cpu_realize(OBJECT(cpu), NULL); >> - >> return cpu; >> } >> > > This will require changes in linux-user and possibly bsd-user. Having a > cpu_realize() would probably help with avoiding #ifdef'ery. > Unfortunately deriving CPUState from DeviceState proves a bit difficult > in the meantime (it worked at one point, now there's lots of circular > header dependencies), and realize support for Object got stopped. > > Andreas > As alternative to keep, I could leave x86_cpu_realize() in cpu_x86_init() and keep pc_cpu_reset() in pc_new_cpu(). That will result in calling cpu_reset() 3 instead of 2 times. Later when apic_init is moved inside cpu.c, a pc_cpu_reset() in pc_new_cpu() would be unnecessary and could be cleaned up then.
Am 09.07.2012 12:59, schrieb igor: > On 06/20/2012 03:35 PM, Andreas Färber wrote: >> Am 20.06.2012 14:59, schrieb Igor Mammedov: >>> It's not correct to make CPU runnable (i.e. calling x86_cpu_realize()) >>> when not all properties are set (APIC in this case). >>> >>> Fix it by calling x86_cpu_realize() at board level after APIC is >>> initialized, right before cpu_reset(). >>> >>> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >>> --- >>> hw/pc.c | 1 + >>> target-i386/helper.c | 2 -- >>> 2 files changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/hw/pc.c b/hw/pc.c >>> index 8368701..8a662cf 100644 >>> --- a/hw/pc.c >>> +++ b/hw/pc.c >>> @@ -948,6 +948,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model) >>> env->apic_state = apic_init(env, env->cpuid_apic_id); >>> } >>> qemu_register_reset(pc_cpu_reset, cpu); >>> + x86_cpu_realize(OBJECT(cpu), NULL); >>> pc_cpu_reset(cpu); >>> return cpu; >>> } >>> diff --git a/target-i386/helper.c b/target-i386/helper.c >>> index c52ec13..b38ea7f 100644 >>> --- a/target-i386/helper.c >>> +++ b/target-i386/helper.c >>> @@ -1161,8 +1161,6 @@ X86CPU *cpu_x86_init(const char *cpu_model) >>> return NULL; >>> } >>> >>> - x86_cpu_realize(OBJECT(cpu), NULL); >>> - >>> return cpu; >>> } >>> >> >> This will require changes in linux-user and possibly bsd-user. Having a >> cpu_realize() would probably help with avoiding #ifdef'ery. >> Unfortunately deriving CPUState from DeviceState proves a bit difficult >> in the meantime (it worked at one point, now there's lots of circular >> header dependencies), and realize support for Object got stopped. >> > As alternative to keep, I could leave x86_cpu_realize() in > cpu_x86_init() and keep pc_cpu_reset() in pc_new_cpu(). That will result > in calling cpu_reset() 3 instead of 2 times. > Later when apic_init is moved inside cpu.c, a pc_cpu_reset() in > pc_new_cpu() would be unnecessary and could be cleaned up then. Let me explain in more detail what I was thinking about: cpu_init() and cpu_x86_init() today return an initialized/realized object. I don't want bugs to creep into the user emulators because someone is not aware that x86 is semantically differing from all other targets. What I did for a qemu-rl78 machine is to inline cpu_rl78_init() so that I could put code in between, i.e., for x86: object_new(), APIC/BSP stuff, x86_cpu_realize(). That way any addition to the realize function will still affect the user emulators. The downside is that when we add x86 CPU subclasses we'd have to remember to update two places. The solution to that would be to split out a x86_cpu_new() function used from cpu_x86_init() and wherever you need it for the PC machine. Then the code is still maintainable in one central place and you get to do your APIC cleanups, and we don't depend on a central realize implementation or device parent, what do you think? Regards, Andreas
On 07/09/2012 02:57 PM, Andreas Färber wrote: > Am 09.07.2012 12:59, schrieb igor: >> On 06/20/2012 03:35 PM, Andreas Färber wrote: >>> Am 20.06.2012 14:59, schrieb Igor Mammedov: >>>> It's not correct to make CPU runnable (i.e. calling x86_cpu_realize()) >>>> when not all properties are set (APIC in this case). >>>> >>>> Fix it by calling x86_cpu_realize() at board level after APIC is >>>> initialized, right before cpu_reset(). >>>> >>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >>>> --- >>>> hw/pc.c | 1 + >>>> target-i386/helper.c | 2 -- >>>> 2 files changed, 1 insertion(+), 2 deletions(-) >>>> >>>> diff --git a/hw/pc.c b/hw/pc.c >>>> index 8368701..8a662cf 100644 >>>> --- a/hw/pc.c >>>> +++ b/hw/pc.c >>>> @@ -948,6 +948,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model) >>>> env->apic_state = apic_init(env, env->cpuid_apic_id); >>>> } >>>> qemu_register_reset(pc_cpu_reset, cpu); >>>> + x86_cpu_realize(OBJECT(cpu), NULL); >>>> pc_cpu_reset(cpu); >>>> return cpu; >>>> } >>>> diff --git a/target-i386/helper.c b/target-i386/helper.c >>>> index c52ec13..b38ea7f 100644 >>>> --- a/target-i386/helper.c >>>> +++ b/target-i386/helper.c >>>> @@ -1161,8 +1161,6 @@ X86CPU *cpu_x86_init(const char *cpu_model) >>>> return NULL; >>>> } >>>> >>>> - x86_cpu_realize(OBJECT(cpu), NULL); >>>> - >>>> return cpu; >>>> } >>>> >>> >>> This will require changes in linux-user and possibly bsd-user. Having a >>> cpu_realize() would probably help with avoiding #ifdef'ery. >>> Unfortunately deriving CPUState from DeviceState proves a bit difficult >>> in the meantime (it worked at one point, now there's lots of circular >>> header dependencies), and realize support for Object got stopped. >>> >> As alternative to keep, I could leave x86_cpu_realize() in >> cpu_x86_init() and keep pc_cpu_reset() in pc_new_cpu(). That will result >> in calling cpu_reset() 3 instead of 2 times. >> Later when apic_init is moved inside cpu.c, a pc_cpu_reset() in >> pc_new_cpu() would be unnecessary and could be cleaned up then. > > Let me explain in more detail what I was thinking about: cpu_init() and > cpu_x86_init() today return an initialized/realized object. I don't want > bugs to creep into the user emulators because someone is not aware that > x86 is semantically differing from all other targets. > > What I did for a qemu-rl78 machine is to inline cpu_rl78_init() so that > I could put code in between, i.e., for x86: object_new(), APIC/BSP > stuff, x86_cpu_realize(). That way any addition to the realize function > will still affect the user emulators. > The downside is that when we add x86 CPU subclasses we'd have to > remember to update two places. The solution to that would be to split > out a x86_cpu_new() function used from cpu_x86_init() and wherever you > need it for the PC machine. Then the code is still maintainable in one > central place and you get to do your APIC cleanups, and we don't depend > on a central realize implementation or device parent, what do you think? If you mean x86_cpu_new() == pc_new_cpu() that calls cpu_x86_init(), then I'd like get rid of pc_new_cpu() completely, eventually replacing it by cpu_x86_init() in hw/pc.c:pc_cpus_init(), something like this: -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; @@ -950,7 +932,7 @@ void pc_cpus_init(const char *cpu_model) } for(i = 0; i < smp_cpus; i++) { - pc_new_cpu(cpu_model); + cpu_x86_init(cpu_model); } } goal I'm aiming at is to have a working cpu object that could be created using qdev_device_add without any adhoc calls. So in the end cpu_x86_init() should become object_new(x86_cpu), [set props], realize() and nothing else. And maybe in some far future removing cpu_init -> cpu_x86_init() completely. That would give us a single implementation of CPU one place (cpu.c)
On Mon, Jul 9, 2012 at 8:57 PM, Andreas Färber <afaerber@suse.de> wrote: > Am 09.07.2012 12:59, schrieb igor: >> On 06/20/2012 03:35 PM, Andreas Färber wrote: >>> Am 20.06.2012 14:59, schrieb Igor Mammedov: >>>> It's not correct to make CPU runnable (i.e. calling x86_cpu_realize()) >>>> when not all properties are set (APIC in this case). >>>> >>>> Fix it by calling x86_cpu_realize() at board level after APIC is >>>> initialized, right before cpu_reset(). >>>> >>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >>>> --- >>>> hw/pc.c | 1 + >>>> target-i386/helper.c | 2 -- >>>> 2 files changed, 1 insertion(+), 2 deletions(-) >>>> >>>> diff --git a/hw/pc.c b/hw/pc.c >>>> index 8368701..8a662cf 100644 >>>> --- a/hw/pc.c >>>> +++ b/hw/pc.c >>>> @@ -948,6 +948,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model) >>>> env->apic_state = apic_init(env, env->cpuid_apic_id); >>>> } >>>> qemu_register_reset(pc_cpu_reset, cpu); >>>> + x86_cpu_realize(OBJECT(cpu), NULL); >>>> pc_cpu_reset(cpu); >>>> return cpu; >>>> } >>>> diff --git a/target-i386/helper.c b/target-i386/helper.c >>>> index c52ec13..b38ea7f 100644 >>>> --- a/target-i386/helper.c >>>> +++ b/target-i386/helper.c >>>> @@ -1161,8 +1161,6 @@ X86CPU *cpu_x86_init(const char *cpu_model) >>>> return NULL; >>>> } >>>> >>>> - x86_cpu_realize(OBJECT(cpu), NULL); >>>> - >>>> return cpu; >>>> } >>>> >>> >>> This will require changes in linux-user and possibly bsd-user. Having a >>> cpu_realize() would probably help with avoiding #ifdef'ery. >>> Unfortunately deriving CPUState from DeviceState proves a bit difficult >>> in the meantime (it worked at one point, now there's lots of circular >>> header dependencies), and realize support for Object got stopped. >>> >> As alternative to keep, I could leave x86_cpu_realize() in >> cpu_x86_init() and keep pc_cpu_reset() in pc_new_cpu(). That will result >> in calling cpu_reset() 3 instead of 2 times. >> Later when apic_init is moved inside cpu.c, a pc_cpu_reset() in >> pc_new_cpu() would be unnecessary and could be cleaned up then. > > Let me explain in more detail what I was thinking about: cpu_init() and > cpu_x86_init() today return an initialized/realized object. I don't want > bugs to creep into the user emulators because someone is not aware that > x86 is semantically differing from all other targets. > > What I did for a qemu-rl78 machine is to inline cpu_rl78_init() so that > I could put code in between, i.e., for x86: object_new(), APIC/BSP > stuff, x86_cpu_realize(). That way any addition to the realize function > will still affect the user emulators. > The downside is that when we add x86 CPU subclasses we'd have to What do you mean " add x86 CPU subclasses" ? Derive from class X86CPU ? And any scene for that? Thanks, pingfan > remember to update two places. The solution to that would be to split > out a x86_cpu_new() function used from cpu_x86_init() and wherever you > need it for the PC machine. Then the code is still maintainable in one > central place and you get to do your APIC cleanups, and we don't depend > on a central realize implementation or device parent, what do you think? > > 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 > > >
On Tue, Jul 10, 2012 at 9:35 PM, Igor Mammedov <imammedo@redhat.com> wrote: > On 07/09/2012 02:57 PM, Andreas Färber wrote: >> >> Am 09.07.2012 12:59, schrieb igor: >>> >>> On 06/20/2012 03:35 PM, Andreas Färber wrote: >>>> >>>> Am 20.06.2012 14:59, schrieb Igor Mammedov: >>>>> >>>>> It's not correct to make CPU runnable (i.e. calling x86_cpu_realize()) >>>>> when not all properties are set (APIC in this case). >>>>> >>>>> Fix it by calling x86_cpu_realize() at board level after APIC is >>>>> initialized, right before cpu_reset(). >>>>> >>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >>>>> --- >>>>> hw/pc.c | 1 + >>>>> target-i386/helper.c | 2 -- >>>>> 2 files changed, 1 insertion(+), 2 deletions(-) >>>>> >>>>> diff --git a/hw/pc.c b/hw/pc.c >>>>> index 8368701..8a662cf 100644 >>>>> --- a/hw/pc.c >>>>> +++ b/hw/pc.c >>>>> @@ -948,6 +948,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model) >>>>> env->apic_state = apic_init(env, env->cpuid_apic_id); >>>>> } >>>>> qemu_register_reset(pc_cpu_reset, cpu); >>>>> + x86_cpu_realize(OBJECT(cpu), NULL); >>>>> pc_cpu_reset(cpu); >>>>> return cpu; >>>>> } >>>>> diff --git a/target-i386/helper.c b/target-i386/helper.c >>>>> index c52ec13..b38ea7f 100644 >>>>> --- a/target-i386/helper.c >>>>> +++ b/target-i386/helper.c >>>>> @@ -1161,8 +1161,6 @@ X86CPU *cpu_x86_init(const char *cpu_model) >>>>> return NULL; >>>>> } >>>>> >>>>> - x86_cpu_realize(OBJECT(cpu), NULL); >>>>> - >>>>> return cpu; >>>>> } >>>>> >>>> >>>> This will require changes in linux-user and possibly bsd-user. Having a >>>> cpu_realize() would probably help with avoiding #ifdef'ery. >>>> Unfortunately deriving CPUState from DeviceState proves a bit difficult >>>> in the meantime (it worked at one point, now there's lots of circular >>>> header dependencies), and realize support for Object got stopped. >>>> >>> As alternative to keep, I could leave x86_cpu_realize() in >>> cpu_x86_init() and keep pc_cpu_reset() in pc_new_cpu(). That will result >>> in calling cpu_reset() 3 instead of 2 times. >>> Later when apic_init is moved inside cpu.c, a pc_cpu_reset() in >>> pc_new_cpu() would be unnecessary and could be cleaned up then. >> >> >> Let me explain in more detail what I was thinking about: cpu_init() and >> cpu_x86_init() today return an initialized/realized object. I don't want >> bugs to creep into the user emulators because someone is not aware that >> x86 is semantically differing from all other targets. >> >> What I did for a qemu-rl78 machine is to inline cpu_rl78_init() so that >> I could put code in between, i.e., for x86: object_new(), APIC/BSP >> stuff, x86_cpu_realize(). That way any addition to the realize function >> will still affect the user emulators. >> The downside is that when we add x86 CPU subclasses we'd have to >> remember to update two places. The solution to that would be to split >> out a x86_cpu_new() function used from cpu_x86_init() and wherever you >> need it for the PC machine. Then the code is still maintainable in one >> central place and you get to do your APIC cleanups, and we don't depend >> on a central realize implementation or device parent, what do you think? > > > If you mean x86_cpu_new() == pc_new_cpu() that calls cpu_x86_init(), > then I'd like get rid of pc_new_cpu() completely, eventually replacing it by > cpu_x86_init() in hw/pc.c:pc_cpus_init(), something like this: > > -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; > @@ -950,7 +932,7 @@ void pc_cpus_init(const char *cpu_model) > } > > for(i = 0; i < smp_cpus; i++) { > - pc_new_cpu(cpu_model); > + cpu_x86_init(cpu_model); > } > } > > goal I'm aiming at is to have a working cpu object that could be created > using qdev_device_add without any adhoc calls. So in the end cpu_x86_init() > should become object_new(x86_cpu), [set props], realize() and nothing else. Could we think apic's "creation + realize" as part of x86_cpu_realize(), but not [set props]? For the concept of building sub log unit inside chip. Regards, pingfan > And maybe in some far future removing cpu_init -> cpu_x86_init() completely. > That would give us a single implementation of CPU one place (cpu.c) > -- > ----- > Regards, > Igor > > >
On 07/11/2012 09:35 AM, liu ping fan wrote: > On Tue, Jul 10, 2012 at 9:35 PM, Igor Mammedov <imammedo@redhat.com> wrote: >> On 07/09/2012 02:57 PM, Andreas Färber wrote: >>> >>> Am 09.07.2012 12:59, schrieb igor: >>>> >>>> On 06/20/2012 03:35 PM, Andreas Färber wrote: >>>>> >>>>> Am 20.06.2012 14:59, schrieb Igor Mammedov: >>>>>> >>>>>> It's not correct to make CPU runnable (i.e. calling x86_cpu_realize()) >>>>>> when not all properties are set (APIC in this case). >>>>>> >>>>>> Fix it by calling x86_cpu_realize() at board level after APIC is >>>>>> initialized, right before cpu_reset(). >>>>>> >>>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >>>>>> --- >>>>>> hw/pc.c | 1 + >>>>>> target-i386/helper.c | 2 -- >>>>>> 2 files changed, 1 insertion(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/hw/pc.c b/hw/pc.c >>>>>> index 8368701..8a662cf 100644 >>>>>> --- a/hw/pc.c >>>>>> +++ b/hw/pc.c >>>>>> @@ -948,6 +948,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model) >>>>>> env->apic_state = apic_init(env, env->cpuid_apic_id); >>>>>> } >>>>>> qemu_register_reset(pc_cpu_reset, cpu); >>>>>> + x86_cpu_realize(OBJECT(cpu), NULL); >>>>>> pc_cpu_reset(cpu); >>>>>> return cpu; >>>>>> } >>>>>> diff --git a/target-i386/helper.c b/target-i386/helper.c >>>>>> index c52ec13..b38ea7f 100644 >>>>>> --- a/target-i386/helper.c >>>>>> +++ b/target-i386/helper.c >>>>>> @@ -1161,8 +1161,6 @@ X86CPU *cpu_x86_init(const char *cpu_model) >>>>>> return NULL; >>>>>> } >>>>>> >>>>>> - x86_cpu_realize(OBJECT(cpu), NULL); >>>>>> - >>>>>> return cpu; >>>>>> } >>>>>> >>>>> >>>>> This will require changes in linux-user and possibly bsd-user. Having a >>>>> cpu_realize() would probably help with avoiding #ifdef'ery. >>>>> Unfortunately deriving CPUState from DeviceState proves a bit difficult >>>>> in the meantime (it worked at one point, now there's lots of circular >>>>> header dependencies), and realize support for Object got stopped. >>>>> >>>> As alternative to keep, I could leave x86_cpu_realize() in >>>> cpu_x86_init() and keep pc_cpu_reset() in pc_new_cpu(). That will result >>>> in calling cpu_reset() 3 instead of 2 times. >>>> Later when apic_init is moved inside cpu.c, a pc_cpu_reset() in >>>> pc_new_cpu() would be unnecessary and could be cleaned up then. >>> >>> >>> Let me explain in more detail what I was thinking about: cpu_init() and >>> cpu_x86_init() today return an initialized/realized object. I don't want >>> bugs to creep into the user emulators because someone is not aware that >>> x86 is semantically differing from all other targets. >>> >>> What I did for a qemu-rl78 machine is to inline cpu_rl78_init() so that >>> I could put code in between, i.e., for x86: object_new(), APIC/BSP >>> stuff, x86_cpu_realize(). That way any addition to the realize function >>> will still affect the user emulators. >>> The downside is that when we add x86 CPU subclasses we'd have to >>> remember to update two places. The solution to that would be to split >>> out a x86_cpu_new() function used from cpu_x86_init() and wherever you >>> need it for the PC machine. Then the code is still maintainable in one >>> central place and you get to do your APIC cleanups, and we don't depend >>> on a central realize implementation or device parent, what do you think? >> >> >> If you mean x86_cpu_new() == pc_new_cpu() that calls cpu_x86_init(), >> then I'd like get rid of pc_new_cpu() completely, eventually replacing it by >> cpu_x86_init() in hw/pc.c:pc_cpus_init(), something like this: >> >> -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; >> @@ -950,7 +932,7 @@ void pc_cpus_init(const char *cpu_model) >> } >> >> for(i = 0; i < smp_cpus; i++) { >> - pc_new_cpu(cpu_model); >> + cpu_x86_init(cpu_model); >> } >> } >> >> goal I'm aiming at is to have a working cpu object that could be created >> using qdev_device_add without any adhoc calls. So in the end cpu_x86_init() >> should become object_new(x86_cpu), [set props], realize() and nothing else. > > Could we think apic's "creation + realize" as part of > x86_cpu_realize(), but not [set props]? > For the concept of building sub log unit inside chip. Yes, sure. Please look at https://github.com/imammedo/qemu/tree/x86_qom_apic it lacks apic_reset() from cpu_reset() but it is easy to add. > > Regards, > pingfan >> And maybe in some far future removing cpu_init -> cpu_x86_init() completely. >> That would give us a single implementation of CPU one place (cpu.c) >> -- >> ----- >> Regards, >> Igor >> >> >>
On Wed, Jul 11, 2012 at 8:27 PM, Igor Mammedov <imammedo@redhat.com> wrote: > On 07/11/2012 09:35 AM, liu ping fan wrote: >> >> On Tue, Jul 10, 2012 at 9:35 PM, Igor Mammedov <imammedo@redhat.com> >> wrote: >>> >>> On 07/09/2012 02:57 PM, Andreas Färber wrote: >>>> >>>> >>>> Am 09.07.2012 12:59, schrieb igor: >>>>> >>>>> >>>>> On 06/20/2012 03:35 PM, Andreas Färber wrote: >>>>>> >>>>>> >>>>>> Am 20.06.2012 14:59, schrieb Igor Mammedov: >>>>>>> >>>>>>> >>>>>>> It's not correct to make CPU runnable (i.e. calling >>>>>>> x86_cpu_realize()) >>>>>>> when not all properties are set (APIC in this case). >>>>>>> >>>>>>> Fix it by calling x86_cpu_realize() at board level after APIC is >>>>>>> initialized, right before cpu_reset(). >>>>>>> >>>>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >>>>>>> --- >>>>>>> hw/pc.c | 1 + >>>>>>> target-i386/helper.c | 2 -- >>>>>>> 2 files changed, 1 insertion(+), 2 deletions(-) >>>>>>> >>>>>>> diff --git a/hw/pc.c b/hw/pc.c >>>>>>> index 8368701..8a662cf 100644 >>>>>>> --- a/hw/pc.c >>>>>>> +++ b/hw/pc.c >>>>>>> @@ -948,6 +948,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model) >>>>>>> env->apic_state = apic_init(env, env->cpuid_apic_id); >>>>>>> } >>>>>>> qemu_register_reset(pc_cpu_reset, cpu); >>>>>>> + x86_cpu_realize(OBJECT(cpu), NULL); >>>>>>> pc_cpu_reset(cpu); >>>>>>> return cpu; >>>>>>> } >>>>>>> diff --git a/target-i386/helper.c b/target-i386/helper.c >>>>>>> index c52ec13..b38ea7f 100644 >>>>>>> --- a/target-i386/helper.c >>>>>>> +++ b/target-i386/helper.c >>>>>>> @@ -1161,8 +1161,6 @@ X86CPU *cpu_x86_init(const char *cpu_model) >>>>>>> return NULL; >>>>>>> } >>>>>>> >>>>>>> - x86_cpu_realize(OBJECT(cpu), NULL); >>>>>>> - >>>>>>> return cpu; >>>>>>> } >>>>>>> >>>>>> >>>>>> This will require changes in linux-user and possibly bsd-user. Having >>>>>> a >>>>>> cpu_realize() would probably help with avoiding #ifdef'ery. >>>>>> Unfortunately deriving CPUState from DeviceState proves a bit >>>>>> difficult >>>>>> in the meantime (it worked at one point, now there's lots of circular >>>>>> header dependencies), and realize support for Object got stopped. >>>>>> >>>>> As alternative to keep, I could leave x86_cpu_realize() in >>>>> cpu_x86_init() and keep pc_cpu_reset() in pc_new_cpu(). That will >>>>> result >>>>> in calling cpu_reset() 3 instead of 2 times. >>>>> Later when apic_init is moved inside cpu.c, a pc_cpu_reset() in >>>>> pc_new_cpu() would be unnecessary and could be cleaned up then. >>>> >>>> >>>> >>>> Let me explain in more detail what I was thinking about: cpu_init() and >>>> cpu_x86_init() today return an initialized/realized object. I don't want >>>> bugs to creep into the user emulators because someone is not aware that >>>> x86 is semantically differing from all other targets. >>>> >>>> What I did for a qemu-rl78 machine is to inline cpu_rl78_init() so that >>>> I could put code in between, i.e., for x86: object_new(), APIC/BSP >>>> stuff, x86_cpu_realize(). That way any addition to the realize function >>>> will still affect the user emulators. >>>> The downside is that when we add x86 CPU subclasses we'd have to >>>> remember to update two places. The solution to that would be to split >>>> out a x86_cpu_new() function used from cpu_x86_init() and wherever you >>>> need it for the PC machine. Then the code is still maintainable in one >>>> central place and you get to do your APIC cleanups, and we don't depend >>>> on a central realize implementation or device parent, what do you think? >>> >>> >>> >>> If you mean x86_cpu_new() == pc_new_cpu() that calls cpu_x86_init(), >>> then I'd like get rid of pc_new_cpu() completely, eventually replacing it >>> by >>> cpu_x86_init() in hw/pc.c:pc_cpus_init(), something like this: >>> >>> -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; >>> @@ -950,7 +932,7 @@ void pc_cpus_init(const char *cpu_model) >>> } >>> >>> for(i = 0; i < smp_cpus; i++) { >>> - pc_new_cpu(cpu_model); >>> + cpu_x86_init(cpu_model); >>> } >>> } >>> >>> goal I'm aiming at is to have a working cpu object that could be created >>> using qdev_device_add without any adhoc calls. So in the end >>> cpu_x86_init() >>> should become object_new(x86_cpu), [set props], realize() and nothing >>> else. >> >> >> Could we think apic's "creation + realize" as part of >> x86_cpu_realize(), but not [set props]? >> For the concept of building sub log unit inside chip. > > > Yes, sure. > Please look at https://github.com/imammedo/qemu/tree/x86_qom_apic > it lacks apic_reset() from cpu_reset() but it is easy to add. > Just wonder whether it is acceptable to call apic_reset directly , or before that, we must make CPU a child of DeviceState, then using qdev_reset_all() pingfan > >> >> Regards, >> pingfan >>> >>> And maybe in some far future removing cpu_init -> cpu_x86_init() >>> completely. >>> That would give us a single implementation of CPU one place (cpu.c) >>> -- >>> ----- >>> Regards, >>> Igor >>> >>> >>> > > -- > ----- > Igor > >
diff --git a/hw/pc.c b/hw/pc.c index 8368701..8a662cf 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -948,6 +948,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model) env->apic_state = apic_init(env, env->cpuid_apic_id); } qemu_register_reset(pc_cpu_reset, cpu); + x86_cpu_realize(OBJECT(cpu), NULL); pc_cpu_reset(cpu); return cpu; } diff --git a/target-i386/helper.c b/target-i386/helper.c index c52ec13..b38ea7f 100644 --- a/target-i386/helper.c +++ b/target-i386/helper.c @@ -1161,8 +1161,6 @@ X86CPU *cpu_x86_init(const char *cpu_model) return NULL; } - x86_cpu_realize(OBJECT(cpu), NULL); - return cpu; }
It's not correct to make CPU runnable (i.e. calling x86_cpu_realize()) when not all properties are set (APIC in this case). Fix it by calling x86_cpu_realize() at board level after APIC is initialized, right before cpu_reset(). Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- hw/pc.c | 1 + target-i386/helper.c | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-)