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

[David Gibson]

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.

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