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