On 12/9/20 9:54 PM, Eduardo Habkost wrote: > On Wed, Dec 09, 2020 at 09:24:36PM +0100, Greg Kurz wrote: >> On Wed, 9 Dec 2020 13:26:17 -0500 >> Eduardo Habkost <ehabk...@redhat.com> wrote: >> >>> On Wed, Dec 09, 2020 at 07:11:40PM +0100, Philippe Mathieu-Daudé wrote: >>> [...] >>>>>>>> @@ -200,7 +199,7 @@ static void spapr_cpu_core_reset(DeviceState *dev) >>>>>>>> int i; >>>>>>>> >>>>>>>> for (i = 0; i < cc->nr_threads; i++) { >>>>>>>> - spapr_reset_vcpu(sc->threads[i]); >>>>>>>> + spapr_reset_vcpu(sc->threads[i], sc->spapr); >>>>>>> >>>>>>> Why reset() needs access to the machine state, don't >>>>>>> you have it in realize()? >>>>>>> >>>>>> >>>>>> This is for the vCPU threads of the sPAPR CPU core. They don't have the >>>>>> link to the machine state. >>>>> >>>>> They are created by spapr_create_vcpu() + spapr_realize_vcpu() in >>>>> spapr_cpu_core_realize(), which has sc->spapr set... Am I missing >>>>> something? >>>> >>>> Anyhow, from a QOM design point of view, resetfn() is not the correct >>>> place to set a property IMHO, so Cc'ing Eduardo. >>> >>> This patch is not setting the property on resetfn(), it is >>> setting it on CPU core pre_plug(). >>> >>> This is more complex than simply using qdev_get_machine() and I >>> don't see why it would be better, but I don't think it's wrong. >>> >> >> The reference to the machine state is basically needed to >> setup/reset/teardown interrupt presenters in the IRQ chip >> backend. It is a bit unfortunate to express this dependency >> at realize(), reset() and unrealize(). Maybe having an >> "irq_chip" property linked to the IRQ chip backend would >> make more sense ? >> > > Considering that the spapr_irq_*() functions get a > SpaprMachineState argument and deal with two interrupt > controllers, maybe you won't be able to save what you need in a > single irq_chip field?
The sPAPR machine needs to operate on all available interrupt controllers and the array is built under the spapr_irq* routines with : SpaprInterruptController *intcs[] = ALL_INTCS(spapr); We could add the array under the machine and use a link to that instead but we need to keep the existing interrupt controllers anyway because of migration compatibility. > I don't have a strong opinion here. It feels weird to me to save > a reference to the global machine object that is always > available, but I don't think that's a problem if you believe the > resulting code looks better. I think it's a good cleanup to remove globals. QEMU might emulate multiple "machines" in a single binary one day. C.