On 09/11/2018 09:34 AM, Greg Kurz wrote:
> On Tue, 11 Sep 2018 07:55:02 +0200
> Cédric Le Goater <c...@kaod.org> wrote:
>
>> The number of MSI interrupts a sPAPR machine can allocate is in direct
>> relation with the number of interrupts of the sPAPRIrq backend. Define
>> statically this value at the sPAPRIrq class level and use it for the
>> "ibm,pe-total-#msi" property of the sPAPR PHB.
>>
>> According to the PAPR specs, "ibm,pe-total-#msi" defines the maximum
>> number of MSIs that are available to the PE. We choose to advertise
>> the maximum number of MSIs that are available to the machine for
>> simplicity of the model and to avoid segmenting the MSI interrupt pool
>> which can be easily shared. If the pool limit is reached, it can be
>> extended dynamically.
>>
>> Finally, remove XICS_IRQS_SPAPR which is now unused.
>>
>> Signed-off-by: Cédric Le Goater <c...@kaod.org>
>> ---
>
> Looks good to me. Just one comment below.
>
>> include/hw/ppc/spapr_irq.h | 1 +
>> include/hw/ppc/xics.h | 2 --
>> hw/ppc/spapr_irq.c | 9 +++++++--
>> hw/ppc/spapr_pci.c | 5 +++--
>> 4 files changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
>> index 0e98c4474bb2..650f810ad2aa 100644
>> --- a/include/hw/ppc/spapr_irq.h
>> +++ b/include/hw/ppc/spapr_irq.h
>> @@ -31,6 +31,7 @@ void spapr_irq_msi_reset(sPAPRMachineState *spapr);
>>
>> typedef struct sPAPRIrq {
>> uint32_t nr_irqs;
>> + uint32_t nr_msis;
>>
>> void (*init)(sPAPRMachineState *spapr, Error **errp);
>> int (*claim)(sPAPRMachineState *spapr, int irq, bool lsi, Error **errp);
>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
>> index 9c2916c9b23a..9958443d1984 100644
>> --- a/include/hw/ppc/xics.h
>> +++ b/include/hw/ppc/xics.h
>> @@ -181,8 +181,6 @@ typedef struct XICSFabricClass {
>> ICPState *(*icp_get)(XICSFabric *xi, int server);
>> } XICSFabricClass;
>>
>> -#define XICS_IRQS_SPAPR 1024
>> -
>> void spapr_dt_xics(int nr_servers, void *fdt, uint32_t phandle);
>>
>> ICPState *xics_icp_get(XICSFabric *xi, int server);
>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
>> index 0cbb5dd39368..fe8be5f5217a 100644
>> --- a/hw/ppc/spapr_irq.c
>> +++ b/hw/ppc/spapr_irq.c
>> @@ -99,7 +99,7 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr,
>> Error **errp)
>>
>> /* Initialize the MSI IRQ allocator. */
>> if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
>> - spapr_irq_msi_init(spapr, XICS_IRQ_BASE + nr_irqs - SPAPR_IRQ_MSI);
>> + spapr_irq_msi_init(spapr, smc->irq->nr_msis);
>> }
>>
>> if (kvm_enabled()) {
>> @@ -195,8 +195,13 @@ static void spapr_irq_print_info_xics(sPAPRMachineState
>> *spapr, Monitor *mon)
>> ics_pic_print_info(spapr->ics, mon);
>> }
>>
>> +#define SPAPR_IRQ_XICS_NR_IRQS 0x400
>> +#define SPAPR_IRQ_XICS_NR_MSIS \
>> + (XICS_IRQ_BASE + SPAPR_IRQ_XICS_NR_IRQS - SPAPR_IRQ_MSI)
>> +
>> sPAPRIrq spapr_irq_xics = {
>> - .nr_irqs = XICS_IRQS_SPAPR,
>> + .nr_irqs = SPAPR_IRQ_XICS_NR_IRQS,
>> + .nr_msis = SPAPR_IRQ_XICS_NR_MSIS,
>>
>> .init = spapr_irq_init_xics,
>> .claim = spapr_irq_claim_xics,
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 6bcb4f419b6b..bb736177e76c 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -2121,6 +2121,7 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
>> sPAPRTCETable *tcet;
>> PCIBus *bus = PCI_HOST_BRIDGE(phb)->bus;
>> sPAPRFDT s_fdt;
>> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine());
>>
>
> It is a bit unfortunate to add another user of qdev_get_machine()...
>
>> /* Start populating the FDT */
>> nodename = g_strdup_printf("pci@%" PRIx64, phb->buid);
>> @@ -2138,8 +2139,8 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
>> _FDT(fdt_setprop(fdt, bus_off, "ranges", &ranges, sizeof_ranges));
>> _FDT(fdt_setprop(fdt, bus_off, "reg", &bus_reg, sizeof(bus_reg)));
>> _FDT(fdt_setprop_cell(fdt, bus_off, "ibm,pci-config-space-type", 0x1));
>> - /* TODO: fine tune the total count of allocatable MSIs per PHB */
>> - _FDT(fdt_setprop_cell(fdt, bus_off, "ibm,pe-total-#msi",
>> XICS_IRQS_SPAPR));
>> + _FDT(fdt_setprop_cell(fdt, bus_off, "ibm,pe-total-#msi",
>> + smc->irq->nr_msis));
>>
>
> ... and to expose machine class internals. Since spapr_populate_pci_dt() is
> only
> called from the core machine code, maybe have the caller to pass the number of
> MSIs ?
yes we could add an extra parameter to spapr_populate_pci_dt()
> Anyway, this can be done in a followup patch so:
>
> Reviewed-by: Greg Kurz <gr...@kaod.org>
Thanks,
C.
>
>> /* Dynamic DMA window */
>> if (phb->ddw_enabled) {
>
