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

[Greg Kurz]

On Thu, 24 Oct 2019 13:38:12 +1100
David Gibson <address@hidden> 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 <address@hidden>
> > Reviewed-by: Greg Kurz <address@hidden>
> > ---
> >  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).

> >  
> >      MemoryRegion xscom_regs;
> >  } PnvCore;
> > diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> > index 9f981a4940e6..cc17bbfed829 100644
> > --- a/hw/ppc/pnv_core.c
> > +++ b/hw/ppc/pnv_core.c
> > @@ -222,6 +222,7 @@ static void pnv_core_realize(DeviceState *dev, Error 
> > **errp)
> >                                  "required link 'chip' not found: ");
> >          return;
> >      }
> > +    pc->chip = PNV_CHIP(chip);
> >  
> >      pc->threads = g_new(PowerPCCPU *, cc->nr_threads);
> >      for (i = 0; i < cc->nr_threads; i++) {
> > @@ -243,7 +244,7 @@ static void pnv_core_realize(DeviceState *dev, Error 
> > **errp)
> >      }
> >  
> >      for (j = 0; j < cc->nr_threads; j++) {
> > -        pnv_realize_vcpu(pc->threads[j], PNV_CHIP(chip), &local_err);
> > +        pnv_realize_vcpu(pc->threads[j], pc->chip, &local_err);
> >          if (local_err) {
> >              goto err;
> >          }
>

pgpNLh6HXC3_t.pgp
Description: OpenPGP digital signature