Re: [PATCH 06/20] xics: Create sPAPR specific ICS subtype

[Greg Kurz]

On Mon, 30 Sep 2019 18:45:30 +1000
David Gibson <address@hidden> wrote:

> On Fri, Sep 27, 2019 at 06:05:56PM +0200, Greg Kurz wrote:
> > On Thu, 26 Sep 2019 10:56:46 +1000
> > David Gibson <address@hidden> wrote:
> > 
> > > On Wed, Sep 25, 2019 at 10:55:35AM +0200, Cédric Le Goater wrote:
> > > > On 25/09/2019 10:40, Greg Kurz wrote:
> > > > > On Wed, 25 Sep 2019 16:45:20 +1000
> > > > > David Gibson <address@hidden> wrote:
> > > > > 
> > > > >> We create a subtype of TYPE_ICS specifically for sPAPR.  For now all 
> > > > >> this
> > > > >> does is move the setup of the PAPR specific hcalls and RTAS calls to
> > > > >> the realize() function for this, rather than requiring the PAPR code 
> > > > >> to
> > > > >> explicitly call xics_spapr_init().  In future it will have some more
> > > > >> function.
> > > > >>
> > > > >> Signed-off-by: David Gibson <address@hidden>
> > > > >> ---
> > > > > 
> > > > > LGTM, but for extra safety I would also introduce a SpaprIcsState 
> > > > > typedef
> > > > 
> > > > why ? we have ICS_SPAPR() for the checks already.
> > > 
> > > Eh.. using typedefs when we haven't actually extended a base type
> > > isn't common QOM practice.  Yes, it's not as typesafe as it could be,
> > > but I'm not really inclined to go to the extra effort here.
> > 
> > I'll soon need to extend the base type with a nr_servers field,
> 
> Uh.. nr_servers doesn't seem like it belongs in the base ICS type.

Of course ! I re-used the wording "extended a base type" of your sentence,
that I understand as "a subtype extends a base type with some more data".
I'm talking about the sPAPR ICS subtype here, not the base ICS type.

> That really would conflict with the pnv usage where the ICS is
> supposed to just represent the ICS, not the xics as a whole.  If you
> need nr_servers information here, it seems like pulling it via a
> method in XICSFabric would make more sense.
> 
> > and while here with an fd field as well in order to get rid of
> > the ugly global in xics.c. I'll go to the extra effort :)
> 
> That could go in the derived type.  We already kind of conflate ICS
> and XICS-as-a-whole for the PAPR subtype, and the KVM xics is PAPR
> only anyway.
> 

Exactly, so that's why I was thinking about adding nr_servers there,
but it could go to XICSFabric as well I guess.

pgpsAcfwEJuAm.pgp
Description: OpenPGP digital signature