Re: [Qemu-ppc] [PATCH v6 4/4] spapr: increase the size of the IRQ number space

[Cédric Le Goater]

On 09/10/2018 08:12 AM, David Gibson wrote:
> On Mon, Jul 30, 2018 at 04:11:34PM +0200, Cédric Le Goater wrote:
>> The new layout using static IRQ number does not leave much space to
>> the dynamic MSI range, only 0x100 IRQ numbers. Increase the total
>> number of IRQS for newer machines and introduce a legacy XICS backend
>> for pre-3.1 machines to maintain compatibility.
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
> 
> Sorry, I got sidetracked and forgot about reviewing this patch.

No problem. It is giving us time to work on OpenCAPI passthrough
and challenge a bit more the static IRQ layout and the sPAPR XIVE 
interrupt mode.  

> Now that the number of irqs is set in the backend, I think the
> reference to XICS_IRQS_SPAPR setting ibm,pe-total-#msi in
> spapr_populate_pci_dt() needs to be change to look at the backend
> instead...

Yes. The number is in direct relation with the msi allocator.  

>> ---
>>  include/hw/ppc/spapr_irq.h |  1 +
>>  hw/ppc/spapr.c             |  1 +
>>  hw/ppc/spapr_irq.c         | 12 +++++++++++-
>>  3 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
>> index 0e98c4474bb2..626160ba475e 100644
>> --- a/include/hw/ppc/spapr_irq.h
>> +++ b/include/hw/ppc/spapr_irq.h
>> @@ -40,6 +40,7 @@ typedef struct sPAPRIrq {
>>  } sPAPRIrq;
>>  
>>  extern sPAPRIrq spapr_irq_xics;
>> +extern sPAPRIrq spapr_irq_xics_legacy;
>>  
>>  int spapr_irq_claim(sPAPRMachineState *spapr, int irq, bool lsi, Error 
>> **errp);
>>  void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num);
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index d9f8cca49208..5ae62b0682d2 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -3947,6 +3947,7 @@ static void 
>> spapr_machine_3_0_class_options(MachineClass *mc)
>>      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_3_0);
>>  
>>      smc->legacy_irq_allocation = true;
>> +    smc->irq = &spapr_irq_xics_legacy;
>>  }
>>  
>>  DEFINE_SPAPR_MACHINE(3_0, "3.0", false);
>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
>> index 0cbb5dd39368..620c49b38455 100644
>> --- a/hw/ppc/spapr_irq.c
>> +++ b/hw/ppc/spapr_irq.c
>> @@ -196,7 +196,7 @@ static void spapr_irq_print_info_xics(sPAPRMachineState 
>> *spapr, Monitor *mon)
>>  }
>>  
>>  sPAPRIrq spapr_irq_xics = {
>> -    .nr_irqs     = XICS_IRQS_SPAPR,
>> +    .nr_irqs     = 0x1000,
>>  
>>      .init        = spapr_irq_init_xics,
>>      .claim       = spapr_irq_claim_xics,
>> @@ -284,3 +284,13 @@ int spapr_irq_find(sPAPRMachineState *spapr, int num, 
>> bool align, Error **errp)
>>  
>>      return first + ics->offset;
>>  }
>> +
>> +sPAPRIrq spapr_irq_xics_legacy = {
>> +    .nr_irqs     = XICS_IRQS_SPAPR,
> 
> .. and with that done, I think it makes to just inline the old value
> here and remove the XICS_IRQS_SPAPR #define, since its name is no
> longer accurate.

yes

Thanks,

C.

>> +
>> +    .init        = spapr_irq_init_xics,
>> +    .claim       = spapr_irq_claim_xics,
>> +    .free        = spapr_irq_free_xics,
>> +    .qirq        = spapr_qirq_xics,
>> +    .print_info  = spapr_irq_print_info_xics,
>> +};
>