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

[David Gibson]

On Wed, Dec 09, 2020 at 01:26:17PM -0500, Eduardo Habkost 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().

Well, also machine reset, but the point is it's not the resetfn() of
the cpu.

Basically what this is doing is machine specific (rather than just cpu
specific) initialization of the cpu state - we need that because the
pseries machine is implementing an explicitly paravirtualized platform
which starts things off in a state a bit different from the "raw" cpu
behaviour.

So, although it's working on a CPU's state, this function actually
belongs to the machine, rather than the cpu.

> 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.

But, yeah, this...

I've applied some of the later patches in this series, but I'm not
convinced on this one or 2/6.  It seems like they're just replacing
one ugly (access to qdev_machine_state() as a global) with a different
ugly (duplicating something which has to equal the global machine
pointer as properties in a bunch of other objects).

Both 1/6 and 2/6 are altering explicitly spapr specific devices, they
have interactions with the overall platform model which mean they have
to sit in that environment, so I think trying to add a property here
implies an abstraction that can't actually be used in practice.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature