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

[Cédric Le Goater]

On 11/23/2017 12:07 PM, David Gibson wrote:
> 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.

I have changed the approach in the latest XIVE patchset and I am not 
using any handlers of any sort anymore. It does not seem necessary
but if it is, you will have a global view of what XIVE requires to 
decide with direction to take.

I will send the patchset later in the day.

Thanks,

C.  

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