Re: [Qemu-devel] [PATCH v2 09/17] ppc/xive: Extend XiveTCTX with a XiveRouter pointer

[Cédric Le Goater]

On 28/07/2019 09:46, David Gibson wrote:
> On Thu, Jul 18, 2019 at 01:54:12PM +0200, Cédric Le Goater wrote:
>> This is to perform lookups in the NVT table when a vCPU is dispatched
>> and possibily resend interrupts.
>>
>> Future XIVE chip will use a different class for the model of the
>> interrupt controller and we might need to change the type of
>> 'XiveRouter *' to 'Object *'
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
> 
> Hrm.  This still bothers me. 

Your feeling is right. There should be a good reason to link two objects 
together as it can be an issue later on (such P10). It should not be an 
hidden parameter to function calls. this is more or less the case. 
 
See below for more explanation.

> AIUI there can be multiple XiveRouters in the system, yes?  

yes and it works relatively well with 4 chips. I say relatively because 
the presenter model is taking a shortcut we should fix. 

> And at least theoretically can present irqs from multiple routers? 

Yes. the console being the most simple example. We only have one device 
per system on the LPC bus of chip 0. 
 
> In which case what's the rule for which one should be associated with 
> a specific.
> I guess it's the one on the same chip, but that needs to be explained
> up front, with some justification of why that's the relevant one.

Yes. we try to minimize the traffic on the PowerBUS so generally CPU 
targets are on the same IC. The EAT on POWER10 should be on the same
HW chip.


I think we can address the proposed changes from another perspective, 
from the presenter one. this is cleaner and reflects better the HW design. 

The thread contexts are owned by the presenter. It can scan its list 
when doing CAM matching and when the thread context registers are being 
accessed by a CPU. Adding a XiveRouter parameter to all the TIMA 
operations seems like a better option and solves the problem.
 

The thread context registers are modeled under the CPU object today 
because it was practical for sPAPR but on HW, these are SRAM entries,
one for each HW thread of the chip. So may be, we should have introduced
an other table under the XiveRouter to model the contexts but there
was no real need for the XIVE POWER9 IC of the pseries machine. This 
design might be reaching its limits with PowerNV and POWER10.  


Looking at :
 
  [PATCH v2 15/17] ppc/pnv: Grab the XiveRouter object from XiveTCTX in 
pnv_xive_get_tctx()

we see that the code adds an extra check on the THREAD_ENABLE registers 
and for that, its needs the IC to which belongs the thread context. This 
code is wrong. We should not be need to find a XiveRouter object from a 
XiveRouter handler.

This is because the xive_presenter_match() routine does:
                       
    CPU_FOREACH(cs) {
        XiveTCTX *tctx = xive_router_get_tctx(xrtr, cs);
 
we should be, instead, looping on the different IC of the system 
looking for a match. Something else to fix. I think I will use the
PIR to match the CPU of a chip.


Looking at POWER10, XIVE internal structures have changed and we will
need to introduce new IC models, one for PowerNV obviously but surely 
also one for pseries. A third one ... yes, sorry about that. If we go 
in that direction, it would be good to have a common XiveTCTX and not 
link it to a specific XiveRouter (P9 or P10). Another good reason not
to use that link.


So I will rework the end of that patchset. Thanks for having given me 
time to think more about it before merging. I did more experiments and
the models are now more precise, specially with guest and multichip
support. 


C.

 
> 
>> ---
>>  include/hw/ppc/xive.h | 2 ++
>>  hw/intc/xive.c        | 9 +++++++++
>>  2 files changed, 11 insertions(+)
>>
>> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
>> index 4851ff87e795..206b23ecfab3 100644
>> --- a/include/hw/ppc/xive.h
>> +++ b/include/hw/ppc/xive.h
>> @@ -320,6 +320,8 @@ typedef struct XiveTCTX {
>>      qemu_irq    os_output;
>>  
>>      uint8_t     regs[XIVE_TM_RING_COUNT * XIVE_TM_RING_SIZE];
>> +
>> +    struct XiveRouter  *xrtr;
>>  } XiveTCTX;
>>  
>>  /*
>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
>> index 88f2e560db0f..1b0eccb6df40 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 = XIVE_ROUTER(obj);
>> +
>>      obj = object_property_get_link(OBJECT(dev), "cpu", &local_err);
>>      if (!obj) {
>>          error_propagate(errp, local_err);
>> @@ -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", OBJECT(xrtr), &error_abort);
>>      object_property_set_bool(obj, true, "realized", &local_err);
>>      if (local_err) {
>>          goto error;
>