Re: [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core

[Greg Kurz]

On Wed, 9 Dec 2020 13:26:17 -0500
Eduardo Habkost <ehabkost@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 ?