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

From: Greg Kurz <groug@kaod.org>
To: David Gibson <david@gibson.dropbear.id.au>
Cc: qemu-ppc@nongnu.org, "Philippe Mathieu-Daudé" <philmd@redhat.com>,
	"Eduardo Habkost" <ehabkost@redhat.com>,
	qemu-devel@nongnu.org
Subject: Re: [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core
Date: Thu, 10 Dec 2020 09:54:28 +0100	[thread overview]
Message-ID: <20201210095428.5996f0ba@bahia.lan> (raw)
In-Reply-To: <20201210035302.GM2555@yekko.fritz.box>

[-- Attachment #1: Type: text/plain, Size: 2734 bytes --]

On Thu, 10 Dec 2020 14:53:02 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

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

Eduardo and you convinced me that 1/6 and 2/6 might not be an
improvement in the end, but rather making things more complex
than simply calling qdev_get_machine() when needed.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]