Re: [Qemu-devel] [PATCH for-2.12 v3 03/11] spapr: introduce new XICSFabric operations for an IRQ allocator

[David Gibson]

On Fri, Nov 17, 2017 at 08:16:47AM +0100, Cédric Le Goater wrote:
> On 11/17/2017 05:48 AM, David Gibson wrote:
> > On Fri, Nov 10, 2017 at 03:20:09PM +0000, Cédric Le Goater wrote:
> >> Currently, the ICSState 'ics' object of the sPAPR machine acts as the
> >> global interrupt source handler and also as the IRQ number allocator
> >> for the machine. Some IRQ numbers are allocated very early in the
> >> machine initialization sequence to populate the device tree, and this
> >> is a problem to introduce the new POWER XIVE interrupt model, as it
> >> needs to share the IRQ numbers with the older model.
> >>
> >> To prepare ground for XIVE, here is a set of new XICSFabric operations
> >> to let the machine handle directly the IRQ number allocation and to
> >> decorrelate the allocation from the interrupt source object :
> >>
> >>     bool (*irq_test)(XICSFabric *xi, int irq);
> >>     int (*irq_alloc_block)(XICSFabric *xi, int count, int align);
> >>     void (*irq_free_block)(XICSFabric *xi, int irq, int num);
> >>
> >> In these prototypes, the 'irq' parameter refers to a number in the
> >> global IRQ number space. Indexes for arrays storing different state
> >> informations on the interrupts, like the ICSIRQState, are usually
> >> named 'srcno'.
> >>
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> > 
> > This doesn't seem sensible to me.  When I said you should move irq
> > allocation to the machine, I mean actually move the code.  The only
> > user of irq allocation should be in the machine, so we shouldn't need
> > to indirect via the XICSFabric interface to do that.
> 
> OK. so we can probably do the same with machine class handlers because 
> we do need an indirection to handle the way older pseries machines 
> allocate IRQs that will change with newer machines  supporting XIVE.

Right.  You could do it either with a MachineClass callback (similar
to the phb placement one), or with just a flag in the MachineClass
that's checked explicitly.  I'd be ok with either approach.

> > And, we shouldn't be using XICSFabric things for XIVE.
> 
> ok. The spapr machine should be enough. 
> 
> Thanks,
> 
> C.
>  
> >> ---
> >>  hw/ppc/spapr.c        | 19 +++++++++++++++++++
> >>  include/hw/ppc/xics.h |  4 ++++
> >>  2 files changed, 23 insertions(+)
> >>
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index a2dcbee07214..84d68f2fdbae 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -3536,6 +3536,21 @@ static ICPState *spapr_icp_get(XICSFabric *xi, int 
> >> vcpu_id)
> >>      return cpu ? ICP(cpu->intc) : NULL;
> >>  }
> >>  
> >> +static bool spapr_irq_test(XICSFabric *xi, int irq)
> >> +{
> >> +    return false;
> >> +}
> >> +
> >> +static int spapr_irq_alloc_block(XICSFabric *xi, int count, int align)
> >> +{
> >> +    return -1;
> >> +}
> >> +
> >> +static void spapr_irq_free_block(XICSFabric *xi, int irq, int num)
> >> +{
> >> +    ;
> >> +}
> >> +
> >>  static void spapr_pic_print_info(InterruptStatsProvider *obj,
> >>                                   Monitor *mon)
> >>  {
> >> @@ -3630,6 +3645,10 @@ static void spapr_machine_class_init(ObjectClass 
> >> *oc, void *data)
> >>      xic->ics_get = spapr_ics_get;
> >>      xic->ics_resend = spapr_ics_resend;
> >>      xic->icp_get = spapr_icp_get;
> >> +    xic->irq_test = spapr_irq_test;
> >> +    xic->irq_alloc_block = spapr_irq_alloc_block;
> >> +    xic->irq_free_block = spapr_irq_free_block;
> >> +
> >>      ispc->print_info = spapr_pic_print_info;
> >>      /* Force NUMA node memory size to be a multiple of
> >>       * SPAPR_MEMORY_BLOCK_SIZE (256M) since that's the granularity
> >> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> >> index 28d248abad61..30e7f2e0a7dd 100644
> >> --- a/include/hw/ppc/xics.h
> >> +++ b/include/hw/ppc/xics.h
> >> @@ -175,6 +175,10 @@ typedef struct XICSFabricClass {
> >>      ICSState *(*ics_get)(XICSFabric *xi, int irq);
> >>      void (*ics_resend)(XICSFabric *xi);
> >>      ICPState *(*icp_get)(XICSFabric *xi, int server);
> >> +    /* IRQ allocator helpers */
> >> +    bool (*irq_test)(XICSFabric *xi, int irq);
> >> +    int (*irq_alloc_block)(XICSFabric *xi, int count, int align);
> >> +    void (*irq_free_block)(XICSFabric *xi, int irq, int num);
> >>  } XICSFabricClass;
> >>  
> >>  #define XICS_IRQS_SPAPR               1024
> > 
> 

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