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

[David Gibson]

On Mon, Sep 30, 2019 at 07:00:43PM +0200, Greg Kurz wrote:
> 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.

Ah, ok.

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



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