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

[Cédric Le Goater]

On 29/07/2019 08:11, David Gibson wrote:
> On Sun, Jul 28, 2019 at 11:06:27AM +0200, Cédric Le Goater wrote:
>> 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.
> 
> Ok, good to hear.  I will wait for the respin.
> 

Could you check the patch adding the powernv{8,9} machines ?  I am 
rebasing the respin on top of this patch and introducing a new handler 
to the machine to perform the CAM line matching.

Thanks,

C.