Re: [PATCH v5 4/7] ppc/pnv: Add a PnvChip pointer to PnvCore

[David Gibson <david@gibson.dropbear.id.au>]

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

On Thu, Oct 24, 2019 at 07:30:30PM +0200, Greg Kurz wrote:
> On Thu, 24 Oct 2019 13:38:12 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Tue, Oct 22, 2019 at 06:38:09PM +0200, Cédric Le Goater wrote:
> > > We will use it to reset the interrupt presenter from the CPU reset
> > > handler.
> > > 
> > > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > > Reviewed-by: Greg Kurz <groug@kaod.org>
> > > ---
> > >  include/hw/ppc/pnv_core.h | 3 +++
> > >  hw/ppc/pnv_core.c         | 3 ++-
> > >  2 files changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
> > > index bfbd2ec42aa6..55eee95104da 100644
> > > --- a/include/hw/ppc/pnv_core.h
> > > +++ b/include/hw/ppc/pnv_core.h
> > > @@ -31,6 +31,8 @@
> > >  #define PNV_CORE_GET_CLASS(obj) \
> > >       OBJECT_GET_CLASS(PnvCoreClass, (obj), TYPE_PNV_CORE)
> > >  
> > > +typedef struct PnvChip PnvChip;
> > > +
> > >  typedef struct PnvCore {
> > >      /*< private >*/
> > >      CPUCore parent_obj;
> > > @@ -38,6 +40,7 @@ typedef struct PnvCore {
> > >      /*< public >*/
> > >      PowerPCCPU **threads;
> > >      uint32_t pir;
> > > +    PnvChip *chip;
> > 
> > I don't love having this as a redundant encoding of the information
> > already in the property, since it raises the possibility of confusing
> > bugs if they ever got out of sync.
> > 
> 
> Ouch, we also have this pattern in xive_tctx_realize(). The XiveTCXT
> object has both a "cpu" property and a pointer to the vCPU.
> 
> > It's not a huge deal, but it would be nice to at least to at least
> > consider either a) grabbing the property everywhere you need it (if
> > there aren't too many places) or b) customizing the property
> > definition so it's written directly into that field.
> > 
> 
> The pointer to the vCPU is used among other things to get the
> value of the PIR, which is needed by the presenting logic to
> match physical CAM lines. This is a _hot_ path so it's probably
> better to go for b).

Agreed.

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

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