Re: [Qemu-devel] [PATCH 08/10] ppc/xive: Extend XiveTCTX with an router object pointer

[David Gibson]

On Wed, Jul 03, 2019 at 07:54:57AM +0200, Cédric Le Goater wrote:
> On 03/07/2019 04:07, David Gibson wrote:
> > On Sun, Jun 30, 2019 at 10:45:59PM +0200, Cédric Le Goater wrote:
> >> This is to perform lookups in the NVT table when a vCPU is dispatched
> >> and possibly resend interrupts.
> > 
> > I'm slightly confused by this one.  Aren't there multiple router
> > objects, each of which can deliver to any thread?  In which case what
> > router object is associated with a specific TCTX?
> 
> when a vCPU is dispatched on a HW thread, the hypervisor does a store 
> on the CAM line to store the VP id. At that time, it checks the IPB in 
> the associated NVT structure and notifies the thread if an interrupt is 
> pending. 
> 
> We need to do a NVT lookup, just like the presenter in HW, hence the 
> router pointer. You should look at the following patch which clarifies 
> the resend sequence.

Hm, ok.

> >> Future XIVE chip will use a different class for the model of the
> >> interrupt controller. So use an 'Object *' instead of a 'XiveRouter *'.
> > 
> > This seems odd to me, shouldn't it be an interface pointer or
> > something in that case?
> 
> I have duplicated most of the XIVE models for P10 because the internal 
> structures have changed. I managed to keep the XiveSource and XiveTCTX 
> but we now have a Xive10Router, this is the reason why.

Right, but XiveRouter and Xive10Router must have something in common
if they can both be used here.  Usually that's expressed as a shared
QOM interface - in which case you can use a pointer to the interface,
rathe than using Object * which kind of implies *anything* can go
here.

> 
> If I was to duplicate XiveTCTX also, I will switch it back to a XiveRouter 
> pointer in the P9 version. 
> 
> C.
> 
> 
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> > 
> >> ---
> >>  include/hw/ppc/xive.h |  4 +++-
> >>  hw/intc/xive.c        | 11 ++++++++++-
> >>  hw/ppc/pnv.c          |  2 +-
> >>  hw/ppc/spapr_irq.c    |  2 +-
> >>  4 files changed, 15 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> >> index d922524982d3..b764e1e4e6d4 100644
> >> --- a/include/hw/ppc/xive.h
> >> +++ b/include/hw/ppc/xive.h
> >> @@ -321,6 +321,8 @@ typedef struct XiveTCTX {
> >>      qemu_irq    os_output;
> >>  
> >>      uint8_t     regs[XIVE_TM_RING_COUNT * XIVE_TM_RING_SIZE];
> >> +
> >> +    Object      *xrtr;
> >>  } XiveTCTX;
> >>  
> >>  /*
> >> @@ -416,7 +418,7 @@ void xive_tctx_tm_write(XiveTCTX *tctx, hwaddr offset, 
> >> uint64_t value,
> >>  uint64_t xive_tctx_tm_read(XiveTCTX *tctx, hwaddr offset, unsigned size);
> >>  
> >>  void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon);
> >> -Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp);
> >> +Object *xive_tctx_create(Object *cpu, Object *xrtr, Error **errp);
> >>  
> >>  static inline uint32_t xive_nvt_cam_line(uint8_t nvt_blk, uint32_t 
> >> nvt_idx)
> >>  {
> >> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> >> index f7ba1c3b622f..56700681884f 100644
> >> --- a/hw/intc/xive.c
> >> +++ b/hw/intc/xive.c
> >> @@ -573,6 +573,14 @@ static void xive_tctx_realize(DeviceState *dev, Error 
> >> **errp)
> >>      Object *obj;
> >>      Error *local_err = NULL;
> >>  
> >> +    obj = object_property_get_link(OBJECT(dev), "xrtr", &local_err);
> >> +    if (!obj) {
> >> +        error_propagate(errp, local_err);
> >> +        error_prepend(errp, "required link 'xrtr' not found: ");
> >> +        return;
> >> +    }
> >> +    tctx->xrtr = obj;
> >> +
> >>      obj = object_property_get_link(OBJECT(dev), "cpu", &local_err);
> >>      if (!obj) {
> >>          error_propagate(errp, local_err);
> >> @@ -657,7 +665,7 @@ static const TypeInfo xive_tctx_info = {
> >>      .class_init    = xive_tctx_class_init,
> >>  };
> >>  
> >> -Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp)
> >> +Object *xive_tctx_create(Object *cpu, Object *xrtr, Error **errp)
> >>  {
> >>      Error *local_err = NULL;
> >>      Object *obj;
> >> @@ -666,6 +674,7 @@ Object *xive_tctx_create(Object *cpu, XiveRouter 
> >> *xrtr, Error **errp)
> >>      object_property_add_child(cpu, TYPE_XIVE_TCTX, obj, &error_abort);
> >>      object_unref(obj);
> >>      object_property_add_const_link(obj, "cpu", cpu, &error_abort);
> >> +    object_property_add_const_link(obj, "xrtr", xrtr, &error_abort);
> >>      object_property_set_bool(obj, true, "realized", &local_err);
> >>      if (local_err) {
> >>          goto error;
> >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> >> index b87e01e5b925..11916dc273c2 100644
> >> --- a/hw/ppc/pnv.c
> >> +++ b/hw/ppc/pnv.c
> >> @@ -765,7 +765,7 @@ static void pnv_chip_power9_intc_create(PnvChip *chip, 
> >> PowerPCCPU *cpu,
> >>       * controller object is initialized afterwards. Hopefully, it's
> >>       * only used at runtime.
> >>       */
> >> -    obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(&chip9->xive), 
> >> &local_err);
> >> +    obj = xive_tctx_create(OBJECT(cpu), OBJECT(&chip9->xive), &local_err);
> >>      if (local_err) {
> >>          error_propagate(errp, local_err);
> >>          return;
> >> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> >> index b2b01e850de8..5b3c3c50967b 100644
> >> --- a/hw/ppc/spapr_irq.c
> >> +++ b/hw/ppc/spapr_irq.c
> >> @@ -353,7 +353,7 @@ static void 
> >> spapr_irq_cpu_intc_create_xive(SpaprMachineState *spapr,
> >>      Object *obj;
> >>      SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
> >>  
> >> -    obj = xive_tctx_create(OBJECT(cpu), XIVE_ROUTER(spapr->xive), 
> >> &local_err);
> >> +    obj = xive_tctx_create(OBJECT(cpu), OBJECT(spapr->xive), &local_err);
> >>      if (local_err) {
> >>          error_propagate(errp, local_err);
> >>          return;
> > 
> 

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