Re: [Qemu-devel] [PATCH 12/25] spapr: introduce a XIVE interrupt presenter model

[David Gibson]

On Wed, Nov 29, 2017 at 10:55:34AM +0100, Cédric Le Goater wrote:
> On 11/29/2017 06:11 AM, David Gibson wrote:
> > On Thu, Nov 23, 2017 at 02:29:42PM +0100, Cédric Le Goater wrote:
> >> The XIVE interrupt presenter exposes a set of rings, also called
> >> Thread Interrupt Management Areas (TIMA), to handle priority
> >> management and interrupt acknowledgment among other things. There is
> >> one ring per level of privilege, four in all. The one we are
> >> interested in for the sPAPR machine is the OS ring.
> >>
> >> The TIMA is mapped at the same address for each CPU. 'current_cpu' is
> >> used to retrieve the targeted interrupt presenter object holding the
> >> cache data of the registers the model use.
> >>
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> >> ---
> >>  hw/intc/spapr_xive.c        | 271 
> >> ++++++++++++++++++++++++++++++++++++++++++++
> >>  hw/intc/xive-internal.h     |  89 +++++++++++++++
> >>  include/hw/ppc/spapr_xive.h |  11 ++
> >>  3 files changed, 371 insertions(+)
> >>
> >> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> >> index b1e3f8710cff..554b25e0884c 100644
> >> --- a/hw/intc/spapr_xive.c
> >> +++ b/hw/intc/spapr_xive.c
> >> @@ -23,9 +23,166 @@
> >>  #include "sysemu/dma.h"
> >>  #include "monitor/monitor.h"
> >>  #include "hw/ppc/spapr_xive.h"
> >> +#include "hw/ppc/xics.h"
> >>  
> >>  #include "xive-internal.h"
> >>  
> >> +struct sPAPRXiveICP {
> > 
> > I'd really prefer to avoid calling anything in xive "icp" to avoid
> > confusion with xics.
> 
> OK. 
> 
> The specs refers to the whole as an IVPE : Interrupt Virtualization 
> Presentation Engine. In our model, we use the TIMA cached values of 
> the OS ring and the qemu_irq for the CPU line. 
> 
> Would 'sPAPRXivePresenter' be fine ?

That'd be ok.  Or call if sPAPRIVPE.  Or even call it TIMA.  I'd be
fine with any of those.

[snip]
> >> +static uint64_t spapr_xive_tm_read(void *opaque, hwaddr offset, unsigned 
> >> size)
> >> +{
> >> +    PowerPCCPU *cpu = POWERPC_CPU(current_cpu);
> > 
> > So, strictly speaking this could be handled by setting each of the
> > CPUs address spaces separately, to something with their own TIMA
> > superimposed on address_space_memory. 
> 
> Ah. I didn't know we could do that.

I think that should work from having seen the code before.  I haven't
actually attempted it..

> > What you have might be more practical though.
> 
> well, you will see at the end of the patchset how cpu->intc is
> assigned.

[snip]

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