[Qemu-devel] [PATCH 0/3] spapr: introduce a new sPAPRIrq backend

[Qemu-devel] [PATCH 0/3] spapr: introduce a new sPAPRIrq backend

[Cédric Le Goater]

Hello,

This series adds a new sPAPRIrq backend increasing the number of
available IRQ numbers in pseries-3.1 machines. This change is an
opportunity to also fix the "ibm,pe-total-#msi" and remove the old
XICS_IRQS_SPAPR definition.

Thanks,

C.

Cédric Le Goater (3):
  spapr: introduce a spapr_irq class 'nr_msis' attribute
  spapr: increase the size of the IRQ number space
  spapr_pci: fix "ibm,pe-total-#msi" value

 include/hw/ppc/spapr_irq.h |  2 ++
 include/hw/ppc/xics.h      |  2 --
 hw/ppc/spapr.c             |  1 +
 hw/ppc/spapr_irq.c         | 22 ++++++++++++++++++++--
 hw/ppc/spapr_pci.c         |  5 +++--
 5 files changed, 26 insertions(+), 6 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/3] spapr: introduce a spapr_irq class 'nr_msis' attribute

[Cédric Le Goater]

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.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/spapr_irq.h | 1 +
 hw/ppc/spapr_irq.c         | 9 +++++++--
 2 files changed, 8 insertions(+), 2 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/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index 0cbb5dd39368..d369ac96f5cd 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     XICS_IRQS_SPAPR
+#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,
-- 
2.17.1

[Qemu-devel] [PATCH 2/3] spapr: increase the size of the IRQ number space

[Cédric Le Goater]

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.

For the old backend, provide a 'nr_msis' value covering the full IRQ
number space as it does not use the bitmap allocator to allocate MSI
interrupt numbers.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/spapr_irq.h |  1 +
 hw/ppc/spapr.c             |  1 +
 hw/ppc/spapr_irq.c         | 15 ++++++++++++++-
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
index 650f810ad2aa..a467ce696ee4 100644
--- a/include/hw/ppc/spapr_irq.h
+++ b/include/hw/ppc/spapr_irq.h
@@ -41,6 +41,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 4a9dd4d9bc14..eba7d60a30a7 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3971,6 +3971,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 d369ac96f5cd..b14d7bce00ea 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -195,7 +195,7 @@ static void spapr_irq_print_info_xics(sPAPRMachineState *spapr, Monitor *mon)
     ics_pic_print_info(spapr->ics, mon);
 }
 
-#define SPAPR_IRQ_XICS_NR_IRQS     XICS_IRQS_SPAPR
+#define SPAPR_IRQ_XICS_NR_IRQS     0x1000
 #define SPAPR_IRQ_XICS_NR_MSIS     \
     (XICS_IRQ_BASE + SPAPR_IRQ_XICS_NR_IRQS - SPAPR_IRQ_MSI)
 
@@ -289,3 +289,16 @@ int spapr_irq_find(sPAPRMachineState *spapr, int num, bool align, Error **errp)
 
     return first + ics->offset;
 }
+
+#define SPAPR_IRQ_XICS_LEGACY_NR_IRQS     XICS_IRQS_SPAPR
+
+sPAPRIrq spapr_irq_xics_legacy = {
+    .nr_irqs     = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
+    .nr_msis     = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
+
+    .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,
+};
-- 
2.17.1

[Qemu-devel] [PATCH 3/3] spapr_pci: fix "ibm,pe-total-#msi" value

[Cédric Le Goater]

The "ibm,pe-total-#msi" property of the sPAPR PHB defines the number
of allocatable MSI interrupts. This is currently set to XICS_IRQS_SPAPR
which covers the full IRQ number space of the machine. This is wrong.

Fix the definition by using the 'nr_msis' attribute of the sPAPRIrq
class and remove XICS_IRQS_SPAPR which is now unused.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/xics.h | 2 --
 hw/ppc/spapr_irq.c    | 2 +-
 hw/ppc/spapr_pci.c    | 5 +++--
 3 files changed, 4 insertions(+), 5 deletions(-)

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 b14d7bce00ea..e77b94cc685e 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -290,7 +290,7 @@ int spapr_irq_find(sPAPRMachineState *spapr, int num, bool align, Error **errp)
     return first + ics->offset;
 }
 
-#define SPAPR_IRQ_XICS_LEGACY_NR_IRQS     XICS_IRQS_SPAPR
+#define SPAPR_IRQ_XICS_LEGACY_NR_IRQS     0x400
 
 sPAPRIrq spapr_irq_xics_legacy = {
     .nr_irqs     = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
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());
 
     /* 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));
 
     /* Dynamic DMA window */
     if (phb->ddw_enabled) {
-- 
2.17.1

Re: [Qemu-devel] [PATCH 0/3] spapr: introduce a new sPAPRIrq backend

[Greg Kurz]

On Mon, 10 Sep 2018 13:02:19 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> Hello,
> 
> This series adds a new sPAPRIrq backend increasing the number of
> available IRQ numbers in pseries-3.1 machines. This change is an
> opportunity to also fix the "ibm,pe-total-#msi" and remove the old
> XICS_IRQS_SPAPR definition.
> 

This cleanup looks sane but I still have a concern with the semantics of
"ibm,pe-total-#msi".

According to LoPAPR:

"ibm,pe-total-#msi"

property name defines the maximum number of Message Signaled Interrupts (MSI plus MSI-X) that are available
to the PE below this device tree node. This number only indicates the number of available interrupts, not the num-
ber assigned. The number assigned for an IOA may be obtained by Function 0 (Query only) of the ibm,change-msi
RTAS call.
prop-encoded-array: Maximum number of interrupts encoded as with encode-int.

IIUC, the PHB is given ibm,pe-total-#msi MSIs that it can assign to devices.

But we currently have only one global allocator in the machine, so having
each PHB advertising the full range of the allocator still looks weird.

Shouldn't this be divided by the number of PHBs ? Or should we have one
separate allocator for each PHB ?

> Thanks,
> 
> C.
> 
> Cédric Le Goater (3):
>   spapr: introduce a spapr_irq class 'nr_msis' attribute
>   spapr: increase the size of the IRQ number space
>   spapr_pci: fix "ibm,pe-total-#msi" value
> 
>  include/hw/ppc/spapr_irq.h |  2 ++
>  include/hw/ppc/xics.h      |  2 --
>  hw/ppc/spapr.c             |  1 +
>  hw/ppc/spapr_irq.c         | 22 ++++++++++++++++++++--
>  hw/ppc/spapr_pci.c         |  5 +++--
>  5 files changed, 26 insertions(+), 6 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/3] spapr: introduce a new sPAPRIrq backend

[Cédric Le Goater]

On 09/10/2018 05:02 PM, Greg Kurz wrote:
> On Mon, 10 Sep 2018 13:02:19 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> Hello,
>>
>> This series adds a new sPAPRIrq backend increasing the number of
>> available IRQ numbers in pseries-3.1 machines. This change is an
>> opportunity to also fix the "ibm,pe-total-#msi" and remove the old
>> XICS_IRQS_SPAPR definition.
>>
> 
> This cleanup looks sane but I still have a concern with the semantics of
> "ibm,pe-total-#msi".
> 
> According to LoPAPR:
> 
> "ibm,pe-total-#msi"
> 
> property name defines the maximum number of Message Signaled Interrupts (MSI plus MSI-X) that are available
> to the PE below this device tree node. This number only indicates the number of available interrupts, not the num-
> ber assigned. The number assigned for an IOA may be obtained by Function 0 (Query only) of the ibm,change-msi
> RTAS call.
> prop-encoded-array: Maximum number of interrupts encoded as with encode-int.
> 
> IIUC, the PHB is given ibm,pe-total-#msi MSIs that it can assign to devices.
> 
> But we currently have only one global allocator in the machine, so having
> each PHB advertising the full range of the allocator still looks weird.

yes. Multiple PHBs share the same IRQ number space and in this
case the advertised number "ibm,pe-total-#msi" does not reflect 
the maximum number of allocatable interrupts per PHB.

The patch improves only the value for one PHB and, as of today, 
it is still wrong when Multiple PHBs are involved.

> Shouldn't this be divided by the number of PHBs ? Or should we have one
> separate allocator for each PHB ?

That would mean segmenting the IRQ number space and I am not 
fond of this solution because we have plenty of space to share:

	0xd00 MSIs

When we find a scenario reaching this limit, I think what we 
should do is to dynamically extend the IRQ number space in QEMU 
and in KVM. It should not be a problem.

We could also downsize "ibm,pe-total-#msi". It is quite big
today. some controllers do have a lot of IRQs but no more 
that a hundred. I might be wrong. 

C.

>> Thanks,
>>
>> C.
>>
>> Cédric Le Goater (3):
>>   spapr: introduce a spapr_irq class 'nr_msis' attribute
>>   spapr: increase the size of the IRQ number space
>>   spapr_pci: fix "ibm,pe-total-#msi" value
>>
>>  include/hw/ppc/spapr_irq.h |  2 ++
>>  include/hw/ppc/xics.h      |  2 --
>>  hw/ppc/spapr.c             |  1 +
>>  hw/ppc/spapr_irq.c         | 22 ++++++++++++++++++++--
>>  hw/ppc/spapr_pci.c         |  5 +++--
>>  5 files changed, 26 insertions(+), 6 deletions(-)
>>
> 

Re: [Qemu-devel] [PATCH 1/3] spapr: introduce a spapr_irq class 'nr_msis' attribute

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2391 bytes --]

On Mon, Sep 10, 2018 at 01:02:20PM +0200, Cédric Le Goater wrote:
11;rgb:ffff/ffff/ffff> 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.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/ppc/spapr_irq.h | 1 +
>  hw/ppc/spapr_irq.c         | 9 +++++++--
>  2 files changed, 8 insertions(+), 2 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/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 0cbb5dd39368..d369ac96f5cd 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     XICS_IRQS_SPAPR
> +#define SPAPR_IRQ_XICS_NR_MSIS     \
> +    (XICS_IRQ_BASE + SPAPR_IRQ_XICS_NR_IRQS - SPAPR_IRQ_MSI)

Uh.. I'm not quite understanding how XICS_IRQ_BASE gets into this.

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 2/3] spapr: increase the size of the IRQ number space

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3146 bytes --]

On Mon, Sep 10, 2018 at 01:02:21PM +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.
> 
> For the old backend, provide a 'nr_msis' value covering the full IRQ
> number space as it does not use the bitmap allocator to allocate MSI
> interrupt numbers.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/ppc/spapr_irq.h |  1 +
>  hw/ppc/spapr.c             |  1 +
>  hw/ppc/spapr_irq.c         | 15 ++++++++++++++-
>  3 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index 650f810ad2aa..a467ce696ee4 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -41,6 +41,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 4a9dd4d9bc14..eba7d60a30a7 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3971,6 +3971,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 d369ac96f5cd..b14d7bce00ea 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -195,7 +195,7 @@ static void spapr_irq_print_info_xics(sPAPRMachineState *spapr, Monitor *mon)
>      ics_pic_print_info(spapr->ics, mon);
>  }
>  
> -#define SPAPR_IRQ_XICS_NR_IRQS     XICS_IRQS_SPAPR
> +#define SPAPR_IRQ_XICS_NR_IRQS     0x1000
>  #define SPAPR_IRQ_XICS_NR_MSIS     \
>      (XICS_IRQ_BASE + SPAPR_IRQ_XICS_NR_IRQS - SPAPR_IRQ_MSI)
>  
> @@ -289,3 +289,16 @@ int spapr_irq_find(sPAPRMachineState *spapr, int num, bool align, Error **errp)
>  
>      return first + ics->offset;
>  }
> +
> +#define SPAPR_IRQ_XICS_LEGACY_NR_IRQS     XICS_IRQS_SPAPR

AFAICT this is the last user of XICS_IRQS_SPAPR, so better to put the
constant right here, rather than copying it from a define with a
misleading name.

> +
> +sPAPRIrq spapr_irq_xics_legacy = {
> +    .nr_irqs     = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
> +    .nr_msis     = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
> +
> +    .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,
> +};

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 3/3] spapr_pci: fix "ibm, pe-total-#msi" value

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3089 bytes --]

On Mon, Sep 10, 2018 at 01:02:22PM +0200, Cédric Le Goater wrote:
> The "ibm,pe-total-#msi" property of the sPAPR PHB defines the number
> of allocatable MSI interrupts. This is currently set to XICS_IRQS_SPAPR
> which covers the full IRQ number space of the machine. This is wrong.
> 
> Fix the definition by using the 'nr_msis' attribute of the sPAPRIrq
> class and remove XICS_IRQS_SPAPR which is now unused.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Oh.. right.. I think it makes more sense to just merge this into the
first patch.

> ---
>  include/hw/ppc/xics.h | 2 --
>  hw/ppc/spapr_irq.c    | 2 +-
>  hw/ppc/spapr_pci.c    | 5 +++--
>  3 files changed, 4 insertions(+), 5 deletions(-)
> 
> 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 b14d7bce00ea..e77b94cc685e 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -290,7 +290,7 @@ int spapr_irq_find(sPAPRMachineState *spapr, int num, bool align, Error **errp)
>      return first + ics->offset;
>  }
>  
> -#define SPAPR_IRQ_XICS_LEGACY_NR_IRQS     XICS_IRQS_SPAPR
> +#define SPAPR_IRQ_XICS_LEGACY_NR_IRQS     0x400
>  
>  sPAPRIrq spapr_irq_xics_legacy = {
>      .nr_irqs     = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
> 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());
>  
>      /* 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));
>  
>      /* 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 0/3] spapr: introduce a new sPAPRIrq backend

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2849 bytes --]

On Mon, Sep 10, 2018 at 07:24:47PM +0200, Cédric Le Goater wrote:
> On 09/10/2018 05:02 PM, Greg Kurz wrote:
> > On Mon, 10 Sep 2018 13:02:19 +0200
> > Cédric Le Goater <clg@kaod.org> wrote:
> > 
> >> Hello,
> >>
> >> This series adds a new sPAPRIrq backend increasing the number of
> >> available IRQ numbers in pseries-3.1 machines. This change is an
> >> opportunity to also fix the "ibm,pe-total-#msi" and remove the old
> >> XICS_IRQS_SPAPR definition.
> >>
> > 
> > This cleanup looks sane but I still have a concern with the semantics of
> > "ibm,pe-total-#msi".
> > 
> > According to LoPAPR:
> > 
> > "ibm,pe-total-#msi"
> > 
> > property name defines the maximum number of Message Signaled Interrupts (MSI plus MSI-X) that are available
> > to the PE below this device tree node. This number only indicates the number of available interrupts, not the num-
> > ber assigned. The number assigned for an IOA may be obtained by Function 0 (Query only) of the ibm,change-msi
> > RTAS call.
> > prop-encoded-array: Maximum number of interrupts encoded as with encode-int.
> > 
> > IIUC, the PHB is given ibm,pe-total-#msi MSIs that it can assign to devices.
> > 
> > But we currently have only one global allocator in the machine, so having
> > each PHB advertising the full range of the allocator still looks weird.
> 
> yes. Multiple PHBs share the same IRQ number space and in this
> case the advertised number "ibm,pe-total-#msi" does not reflect 
> the maximum number of allocatable interrupts per PHB.
> 
> The patch improves only the value for one PHB and, as of today, 
> it is still wrong when Multiple PHBs are involved.
> 
> > Shouldn't this be divided by the number of PHBs ? Or should we have one
> > separate allocator for each PHB ?
> 
> That would mean segmenting the IRQ number space and I am not 
> fond of this solution because we have plenty of space to share:
> 
> 	0xd00 MSIs
> 
> When we find a scenario reaching this limit, I think what we 
> should do is to dynamically extend the IRQ number space in QEMU 
> and in KVM. It should not be a problem.
> 
> We could also downsize "ibm,pe-total-#msi". It is quite big
> today. some controllers do have a lot of IRQs but no more 
> that a hundred. I might be wrong.

Yeah, this is my take as well.  The spec sort of implies, but doesn't
explicitly state a per-PHB allocation of MSIs.  But there's not really
a good reason to partition the irqs that way.  So it may be a bit fast
and loose w.r.t. PAPR, but as long as we have plenty of MSIs available
I think using a shared pool is unlikely to cause problems in practice.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 1/3] spapr: introduce a spapr_irq class 'nr_msis' attribute

[Cédric Le Goater]

On 09/11/2018 03:48 AM, David Gibson wrote:
> On Mon, Sep 10, 2018 at 01:02:20PM +0200, Cédric Le Goater wrote:
> 11;rgb:ffff/ffff/ffff> 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.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  include/hw/ppc/spapr_irq.h | 1 +
>>  hw/ppc/spapr_irq.c         | 9 +++++++--
>>  2 files changed, 8 insertions(+), 2 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/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
>> index 0cbb5dd39368..d369ac96f5cd 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     XICS_IRQS_SPAPR
>> +#define SPAPR_IRQ_XICS_NR_MSIS     \
>> +    (XICS_IRQ_BASE + SPAPR_IRQ_XICS_NR_IRQS - SPAPR_IRQ_MSI)
> 
> Uh.. I'm not quite understanding how XICS_IRQ_BASE gets into this.

because the IRQ ranges of the new static IRQ number space start at 
the sPAPR IRQ number offset.

XICS_IRQ_BASE		0x1000
SPAPR_IRQ_XICS_NR_IRQS	0x400

SPAPR_IRQ_MSI		0x1300

0x1000 + 0x400 - 0x1300 = 0x100

we could use SPAPR_IRQ_EPOW instead or some other value defining 
the IRQ0 number.

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

Re: [Qemu-devel] [PATCH 3/3] spapr_pci: fix "ibm, pe-total-#msi" value

[Cédric Le Goater]

On 09/11/2018 03:50 AM, David Gibson wrote:
> On Mon, Sep 10, 2018 at 01:02:22PM +0200, Cédric Le Goater wrote:
>> The "ibm,pe-total-#msi" property of the sPAPR PHB defines the number
>> of allocatable MSI interrupts. This is currently set to XICS_IRQS_SPAPR
>> which covers the full IRQ number space of the machine. This is wrong.
>>
>> Fix the definition by using the 'nr_msis' attribute of the sPAPRIrq
>> class and remove XICS_IRQS_SPAPR which is now unused.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> Oh.. right.. I think it makes more sense to just merge this into the
> first patch.

OK. I will also remove XICS_IRQS_SPAPR in the first patch then

Thanks,

C.

>> ---
>>  include/hw/ppc/xics.h | 2 --
>>  hw/ppc/spapr_irq.c    | 2 +-
>>  hw/ppc/spapr_pci.c    | 5 +++--
>>  3 files changed, 4 insertions(+), 5 deletions(-)
>>
>> 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 b14d7bce00ea..e77b94cc685e 100644
>> --- a/hw/ppc/spapr_irq.c
>> +++ b/hw/ppc/spapr_irq.c
>> @@ -290,7 +290,7 @@ int spapr_irq_find(sPAPRMachineState *spapr, int num, bool align, Error **errp)
>>      return first + ics->offset;
>>  }
>>  
>> -#define SPAPR_IRQ_XICS_LEGACY_NR_IRQS     XICS_IRQS_SPAPR
>> +#define SPAPR_IRQ_XICS_LEGACY_NR_IRQS     0x400
>>  
>>  sPAPRIrq spapr_irq_xics_legacy = {
>>      .nr_irqs     = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
>> 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());
>>  
>>      /* 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));
>>  
>>      /* Dynamic DMA window */
>>      if (phb->ddw_enabled) {
> 

Re: [Qemu-devel] [PATCH 0/3] spapr: introduce a new sPAPRIrq backend

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 2949 bytes --]

On Tue, 11 Sep 2018 11:52:46 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Sep 10, 2018 at 07:24:47PM +0200, Cédric Le Goater wrote:
> > On 09/10/2018 05:02 PM, Greg Kurz wrote:  
> > > On Mon, 10 Sep 2018 13:02:19 +0200
> > > Cédric Le Goater <clg@kaod.org> wrote:
> > >   
> > >> Hello,
> > >>
> > >> This series adds a new sPAPRIrq backend increasing the number of
> > >> available IRQ numbers in pseries-3.1 machines. This change is an
> > >> opportunity to also fix the "ibm,pe-total-#msi" and remove the old
> > >> XICS_IRQS_SPAPR definition.
> > >>  
> > > 
> > > This cleanup looks sane but I still have a concern with the semantics of
> > > "ibm,pe-total-#msi".
> > > 
> > > According to LoPAPR:
> > > 
> > > "ibm,pe-total-#msi"
> > > 
> > > property name defines the maximum number of Message Signaled Interrupts (MSI plus MSI-X) that are available
> > > to the PE below this device tree node. This number only indicates the number of available interrupts, not the num-
> > > ber assigned. The number assigned for an IOA may be obtained by Function 0 (Query only) of the ibm,change-msi
> > > RTAS call.
> > > prop-encoded-array: Maximum number of interrupts encoded as with encode-int.
> > > 
> > > IIUC, the PHB is given ibm,pe-total-#msi MSIs that it can assign to devices.
> > > 
> > > But we currently have only one global allocator in the machine, so having
> > > each PHB advertising the full range of the allocator still looks weird.  
> > 
> > yes. Multiple PHBs share the same IRQ number space and in this
> > case the advertised number "ibm,pe-total-#msi" does not reflect 
> > the maximum number of allocatable interrupts per PHB.
> > 
> > The patch improves only the value for one PHB and, as of today, 
> > it is still wrong when Multiple PHBs are involved.
> >   
> > > Shouldn't this be divided by the number of PHBs ? Or should we have one
> > > separate allocator for each PHB ?  
> > 
> > That would mean segmenting the IRQ number space and I am not 
> > fond of this solution because we have plenty of space to share:
> > 
> > 	0xd00 MSIs
> > 
> > When we find a scenario reaching this limit, I think what we 
> > should do is to dynamically extend the IRQ number space in QEMU 
> > and in KVM. It should not be a problem.
> > 
> > We could also downsize "ibm,pe-total-#msi". It is quite big
> > today. some controllers do have a lot of IRQs but no more 
> > that a hundred. I might be wrong.  
> 
> Yeah, this is my take as well.  The spec sort of implies, but doesn't
> explicitly state a per-PHB allocation of MSIs.  But there's not really
> a good reason to partition the irqs that way.  So it may be a bit fast
> and loose w.r.t. PAPR, but as long as we have plenty of MSIs available
> I think using a shared pool is unlikely to cause problems in practice.
> 

Fair enough. Thanks for the clarification.

Cheers,

--
Greg

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 1/3] spapr: introduce a spapr_irq class 'nr_msis' attribute

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2816 bytes --]

On Tue, Sep 11, 2018 at 06:41:24AM +0200, Cédric Le Goater wrote:
> On 09/11/2018 03:48 AM, David Gibson wrote:
> > On Mon, Sep 10, 2018 at 01:02:20PM +0200, Cédric Le Goater wrote:
> > 11;rgb:ffff/ffff/ffff> 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.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>  include/hw/ppc/spapr_irq.h | 1 +
> >>  hw/ppc/spapr_irq.c         | 9 +++++++--
> >>  2 files changed, 8 insertions(+), 2 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/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> >> index 0cbb5dd39368..d369ac96f5cd 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     XICS_IRQS_SPAPR
> >> +#define SPAPR_IRQ_XICS_NR_MSIS     \
> >> +    (XICS_IRQ_BASE + SPAPR_IRQ_XICS_NR_IRQS - SPAPR_IRQ_MSI)
> > 
> > Uh.. I'm not quite understanding how XICS_IRQ_BASE gets into this.
> 
> because the IRQ ranges of the new static IRQ number space start at 
> the sPAPR IRQ number offset.
> 
> XICS_IRQ_BASE		0x1000
> SPAPR_IRQ_XICS_NR_IRQS	0x400
> 
> SPAPR_IRQ_MSI		0x1300
> 
> 0x1000 + 0x400 - 0x1300 = 0x100

Duh, sorry, that's kind of obvious.  Apparently my brain wasn't
working the other day.

> we could use SPAPR_IRQ_EPOW instead or some other value defining 
> the IRQ0 number.

No, it's fine.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]