Re: [Qemu-ppc] [PATCH v2 1/2] spapr: introduce a spapr_irq class 'nr_msis' attribute

[David Gibson]

On Tue, Sep 11, 2018 at 09:56:06AM +0200, Cédric Le Goater wrote:
> On 09/11/2018 09:34 AM, Greg Kurz wrote:
> > On Tue, 11 Sep 2018 07:55:02 +0200
> > Cédric Le Goater <address@hidden> 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 <address@hidden>
> >> ---
> > 
> > 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("address@hidden" 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()

I'd like to see this as a followup.
> 
> > Anyway, this can be done in a followup patch so:
> > 
> > Reviewed-by: Greg Kurz <address@hidden>
> 
> Thanks,
> 
> C.
> > 
> >>      /* Dynamic DMA window */
> >>      if (phb->ddw_enabled) {
> > 
> 

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