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 <c...@kaod.org>
>>>
>>> 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.
