Message ID | 1363628125-5310-1-git-send-email-pbonzini@redhat.com |
---|---|
State | New |
Headers | show |
Il 18/03/2013 19:17, Peter Maydell ha scritto: >> > Shouldn't these containers also host the CPU device(s), rather than the >> > boards? And create them according to the num-cpu property? If so, they >> > would have to go in hw/arm. > Yes, ideally they should have the CPU devices in them too. > Remind me why devices which instantiate the CPU device go > in hw/arm ? Because they refer to ARMCPU/CPUARMState. Paolo
On 18 March 2013 20:05, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 18/03/2013 19:17, Peter Maydell ha scritto: >>> > Shouldn't these containers also host the CPU device(s), rather than the >>> > boards? And create them according to the num-cpu property? If so, they >>> > would have to go in hw/arm. >> Yes, ideally they should have the CPU devices in them too. >> Remind me why devices which instantiate the CPU device go >> in hw/arm ? > > Because they refer to ARMCPU/CPUARMState. Well, a container object that instantiated the CPUs wouldn't be referring to the internal CPU state struct, it would just be treating them as QOM objects like any other, so that doesn't apply. -- PMM
Il 18/03/2013 21:21, Peter Maydell ha scritto: > On 18 March 2013 20:05, Paolo Bonzini <pbonzini@redhat.com> wrote: >> Il 18/03/2013 19:17, Peter Maydell ha scritto: >>>>> Shouldn't these containers also host the CPU device(s), rather than the >>>>> boards? And create them according to the num-cpu property? If so, they >>>>> would have to go in hw/arm. >>> Yes, ideally they should have the CPU devices in them too. >>> Remind me why devices which instantiate the CPU device go >>> in hw/arm ? >> >> Because they refer to ARMCPU/CPUARMState. > > Well, a container object that instantiated the CPUs wouldn't > be referring to the internal CPU state struct, it would just > be treating them as QOM objects like any other, so that doesn't > apply. Wouldn't it also bridge the CPU's internal interrupt pins to the GIC? Like this code in highbank.c: /* This will become a QOM property eventually */ irqp = arm_pic_init_cpu(cpu); cpu_irq[n] = irqp[ARM_PIC_CPU_IRQ]; ... for (n = 0; n < smp_cpus; n++) { sysbus_connect_irq(busdev, n, cpu_irq[n]); } Paolo
On 19 March 2013 09:26, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 18/03/2013 21:21, Peter Maydell ha scritto: >> On 18 March 2013 20:05, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> Il 18/03/2013 19:17, Peter Maydell ha scritto: >>>>>> Shouldn't these containers also host the CPU device(s), rather than the >>>>>> boards? And create them according to the num-cpu property? If so, they >>>>>> would have to go in hw/arm. >>>> Yes, ideally they should have the CPU devices in them too. >>>> Remind me why devices which instantiate the CPU device go >>>> in hw/arm ? >>> >>> Because they refer to ARMCPU/CPUARMState. >> >> Well, a container object that instantiated the CPUs wouldn't >> be referring to the internal CPU state struct, it would just >> be treating them as QOM objects like any other, so that doesn't >> apply. > > Wouldn't it also bridge the CPU's internal interrupt pins to the GIC? > Like this code in highbank.c: > > > /* This will become a QOM property eventually */ > irqp = arm_pic_init_cpu(cpu); > cpu_irq[n] = irqp[ARM_PIC_CPU_IRQ]; > ... > for (n = 0; n < smp_cpus; n++) { > sysbus_connect_irq(busdev, n, cpu_irq[n]); > } Well, for the CPU to be a proper QOM object it should be exposing the IRQ/FIQ lines normally, not via the code in hw/arm/pic_cpu.c. My point is that the QOM abstraction should encapsulate the CPU cores just like any other piece of hardware. We're not there yet but that's where we should be going. You can't really put the CPUs into the a9mpcore &c containers until we've done that abstraction properly anyway. -- PMM
Il 19/03/2013 11:10, Peter Maydell ha scritto: > On 19 March 2013 09:26, Paolo Bonzini <pbonzini@redhat.com> wrote: >> Il 18/03/2013 21:21, Peter Maydell ha scritto: >>> On 18 March 2013 20:05, Paolo Bonzini <pbonzini@redhat.com> wrote: >>>> Il 18/03/2013 19:17, Peter Maydell ha scritto: >>>>>>> Shouldn't these containers also host the CPU device(s), rather than the >>>>>>> boards? And create them according to the num-cpu property? If so, they >>>>>>> would have to go in hw/arm. >>>>> Yes, ideally they should have the CPU devices in them too. >>>>> Remind me why devices which instantiate the CPU device go >>>>> in hw/arm ? >>>> >>>> Because they refer to ARMCPU/CPUARMState. >>> >>> Well, a container object that instantiated the CPUs wouldn't >>> be referring to the internal CPU state struct, it would just >>> be treating them as QOM objects like any other, so that doesn't >>> apply. >> >> Wouldn't it also bridge the CPU's internal interrupt pins to the GIC? >> Like this code in highbank.c: >> >> >> /* This will become a QOM property eventually */ >> irqp = arm_pic_init_cpu(cpu); >> cpu_irq[n] = irqp[ARM_PIC_CPU_IRQ]; >> ... >> for (n = 0; n < smp_cpus; n++) { >> sysbus_connect_irq(busdev, n, cpu_irq[n]); >> } > > Well, for the CPU to be a proper QOM object it should be exposing > the IRQ/FIQ lines normally, not via the code in hw/arm/pic_cpu.c. That applies to everything else that was put in hw/ARCH. Everything could become a QOM property. > My point is that the QOM abstraction should encapsulate the CPU > cores just like any other piece of hardware. We're not there yet > but that's where we should be going. You can't really put the > CPUs into the a9mpcore &c containers until we've done that > abstraction properly anyway. Why not? It would remove a bunch of code that is currently duplicated in the boards. Paolo
On 19 March 2013 10:27, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 19/03/2013 11:10, Peter Maydell ha scritto: >> Well, for the CPU to be a proper QOM object it should be exposing >> the IRQ/FIQ lines normally, not via the code in hw/arm/pic_cpu.c. > > That applies to everything else that was put in hw/ARCH. Everything > could become a QOM property. Yes. If we clean this stuff up I can kick it back out of hw/ARCH :-) >> My point is that the QOM abstraction should encapsulate the CPU >> cores just like any other piece of hardware. We're not there yet >> but that's where we should be going. You can't really put the >> CPUs into the a9mpcore &c containers until we've done that >> abstraction properly anyway. > > Why not? It would remove a bunch of code that is currently duplicated > in the boards. Hmm, maybe. -- PMM
Il 19/03/2013 11:32, Peter Maydell ha scritto: > On 19 March 2013 10:27, Paolo Bonzini <pbonzini@redhat.com> wrote: >> Il 19/03/2013 11:10, Peter Maydell ha scritto: >>> Well, for the CPU to be a proper QOM object it should be exposing >>> the IRQ/FIQ lines normally, not via the code in hw/arm/pic_cpu.c. >> >> That applies to everything else that was put in hw/ARCH. Everything >> could become a QOM property. > > Yes. If we clean this stuff up I can kick it back out of hw/ARCH :-) > >>> My point is that the QOM abstraction should encapsulate the CPU >>> cores just like any other piece of hardware. We're not there yet >>> but that's where we should be going. You can't really put the >>> CPUs into the a9mpcore &c containers until we've done that >>> abstraction properly anyway. >> >> Why not? It would remove a bunch of code that is currently duplicated >> in the boards. > > Hmm, maybe. So, okay to put these in hw/arm and then I'll work on patches moving cpu_arm_init to a*mpcore.c? Paolo
On 19 March 2013 22:23, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 19/03/2013 11:32, Peter Maydell ha scritto: >> On 19 March 2013 10:27, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> Il 19/03/2013 11:10, Peter Maydell ha scritto: >>>> My point is that the QOM abstraction should encapsulate the CPU >>>> cores just like any other piece of hardware. We're not there yet >>>> but that's where we should be going. You can't really put the >>>> CPUs into the a9mpcore &c containers until we've done that >>>> abstraction properly anyway. >>> >>> Why not? It would remove a bunch of code that is currently duplicated >>> in the boards. >> >> Hmm, maybe. > > So, okay to put these in hw/arm and then I'll work on patches moving > cpu_arm_init to a*mpcore.c? Wrong way round. If you can't put the cpu_arm_init into the a*mpcore in a way that doesn't make you want to put them in hw/arm/ then it should wait until we've QOMified the CPU cores sufficiently that we can do it properly. -- PMM
Il 19/03/2013 23:34, Peter Maydell ha scritto: > On 19 March 2013 22:23, Paolo Bonzini <pbonzini@redhat.com> wrote: >> Il 19/03/2013 11:32, Peter Maydell ha scritto: >>> On 19 March 2013 10:27, Paolo Bonzini <pbonzini@redhat.com> wrote: >>>> Il 19/03/2013 11:10, Peter Maydell ha scritto: >>>>> My point is that the QOM abstraction should encapsulate the CPU >>>>> cores just like any other piece of hardware. We're not there yet >>>>> but that's where we should be going. You can't really put the >>>>> CPUs into the a9mpcore &c containers until we've done that >>>>> abstraction properly anyway. >>>> >>>> Why not? It would remove a bunch of code that is currently duplicated >>>> in the boards. >>> >>> Hmm, maybe. >> >> So, okay to put these in hw/arm and then I'll work on patches moving >> cpu_arm_init to a*mpcore.c? > > Wrong way round. If you can't put the cpu_arm_init into the a*mpcore > in a way that doesn't make you want to put them in hw/arm/ then > it should wait until we've QOMified the CPU cores sufficiently > that we can do it properly. Does that include calling the CPU constructor something else than cpu_arm_init (which is defined in target-arm/)? For me that would be enough to put it in hw/arm. Paolo
On 19 March 2013 23:35, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 19/03/2013 23:34, Peter Maydell ha scritto: >> On 19 March 2013 22:23, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> So, okay to put these in hw/arm and then I'll work on patches moving >>> cpu_arm_init to a*mpcore.c? >> >> Wrong way round. If you can't put the cpu_arm_init into the a*mpcore >> in a way that doesn't make you want to put them in hw/arm/ then >> it should wait until we've QOMified the CPU cores sufficiently >> that we can do it properly. > > Does that include calling the CPU constructor something else than > cpu_arm_init (which is defined in target-arm/)? For me that would be > enough to put it in hw/arm. The CPU should be created (and thus initialised and realised) the same way as any other QOM object or device; the containers shouldn't need to call cpu_arm_init directly. -- PMM
Il 20/03/2013 00:44, Peter Maydell ha scritto: > On 19 March 2013 23:35, Paolo Bonzini <pbonzini@redhat.com> wrote: >> > Il 19/03/2013 23:34, Peter Maydell ha scritto: >>> >> On 19 March 2013 22:23, Paolo Bonzini <pbonzini@redhat.com> wrote: >>>> >>> So, okay to put these in hw/arm and then I'll work on patches moving >>>> >>> cpu_arm_init to a*mpcore.c? >>> >> >>> >> Wrong way round. If you can't put the cpu_arm_init into the a*mpcore >>> >> in a way that doesn't make you want to put them in hw/arm/ then >>> >> it should wait until we've QOMified the CPU cores sufficiently >>> >> that we can do it properly. >> > >> > Does that include calling the CPU constructor something else than >> > cpu_arm_init (which is defined in target-arm/)? For me that would be >> > enough to put it in hw/arm. > The CPU should be created (and thus initialised and realised) the same > way as any other QOM object or device; the containers shouldn't need > to call cpu_arm_init directly. ARM is already quite good in that respect. However, until all architectures are converted cpu_*_init needs to remain because of user-mode targets (where the CPUs are created by common code, there is no board to encapsulate the target-specific differences). IMHO waiting for the demise of cpu_*_init is putting the cart before the horse, or another similar proverb. (Also, I don't see much difference between using a function in target-ARCH and using a TYPE_FOO define from target-ARCH. They are the same thing masked through RTTI. hw/ARCH should really be the bridge between target-ARCH and hw/everything-else, and a*mpcore.c fits in that description). Paolo
On 20 March 2013 00:00, Paolo Bonzini <pbonzini@redhat.com> wrote: > ARM is already quite good in that respect. However, until all > architectures are converted cpu_*_init needs to remain because of > user-mode targets (where the CPUs are created by common code, there is > no board to encapsulate the target-specific differences). IMHO waiting > for the demise of cpu_*_init is putting the cart before the horse, or > another similar proverb. As I say, I don't object to you moving the cpu init code into the a*mpcore containers. I do object to you moving the a*mpcore containers into hw/arm, and so transitively I object to changes to the containers made only with the intent to cause them to move into hw/arm. > (Also, I don't see much difference between using a function in > target-ARCH and using a TYPE_FOO define from target-ARCH. They are the > same thing masked through RTTI. hw/ARCH should really be the bridge > between target-ARCH and hw/everything-else, and a*mpcore.c fits in that > description). Basically I don't like the way your categorization scheme seems to be "kind of device, except for stuff in hw/ARCH which has a completely different rationale". I'm OK with hw/ARCH having board models, because that's really "kind of device", it's just split what ought to be hw/boards into a bunch of separate directories. And I've accepted having some of the random "needs fixing and splitting and chucking back out of hw/ARCH" code as a temporary measure. But please stop trying to push stuff into hw/ARCH rather than categorising it properly. I feel like I'm arguing round in circles here. -- PMM
Il 20/03/2013 11:30, Peter Maydell ha scritto: > Basically I don't like the way your categorization scheme seems > to be "kind of device, except for stuff in hw/ARCH which has > a completely different rationale". I'm OK with hw/ARCH having > board models, because that's really "kind of device", it's > just split what ought to be hw/boards into a bunch of separate > directories. And I've accepted having some of the random "needs > fixing and splitting and chucking back out of hw/ARCH" code as > a temporary measure. But please stop trying to push stuff into > hw/ARCH rather than categorising it properly. I feel like I'm > arguing round in circles here. Ok, I guess I'll add hw/cpu. Paolo