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

[Cédric Le Goater]

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.

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

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