[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 1/2] ppc/spapr: Introduce SPAPR_IRQ_NR_IPIS to refer IRQ r

From: Cédric Le Goater
Subject: Re: [PATCH v3 1/2] ppc/spapr: Introduce SPAPR_IRQ_NR_IPIS to refer IRQ range for CPU IPIs.
Date: Fri, 24 Nov 2023 09:09:04 +0100
User-agent: Mozilla Thunderbird

On 11/24/23 09:01, Harsh Prateek Bora wrote:

On 11/23/23 19:42, Cédric Le Goater wrote:
On 11/23/23 10:31, Harsh Prateek Bora wrote:

On 11/23/23 14:20, Cédric Le Goater wrote:
On 11/23/23 06:57, Harsh Prateek Bora wrote:
spapr_irq_init currently uses existing macro SPAPR_XIRQ_BASE to refer to
the range of CPU IPIs during initialization of nr-irqs property.
It is more appropriate to have its own define which can be further
reused as appropriate for correct interpretation.

Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
Suggested-by: Cedric Le Goater <clg@kaod.org>

One comment below

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks, responding below ..

  include/hw/ppc/spapr_irq.h | 14 +++++++++++++-
  hw/ppc/spapr_irq.c         |  6 ++++--
  2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
index c22a72c9e2..4fd2d5853d 100644
--- a/include/hw/ppc/spapr_irq.h
+++ b/include/hw/ppc/spapr_irq.h
@@ -14,9 +14,21 @@
  #include "qom/object.h"
- * IRQ range offsets per device type
+ * The XIVE IRQ backend uses the same layout as the XICS backend but
+ * covers the full range of the IRQ number space. The IRQ numbers for
+ * the CPU IPIs are allocated at the bottom of this space, below 4K,
+ * to preserve compatibility with XICS which does not use that range.
+ */
+ * CPU IPI range (XIVE only)
  #define SPAPR_IRQ_IPI        0x0
+#define SPAPR_IRQ_NR_IPIS    0x1000
+ * IRQ range offsets per device type
+ */
  #define SPAPR_XIRQ_BASE      XICS_IRQ_BASE /* 0x1000 */
  #define SPAPR_IRQ_EPOW       (SPAPR_XIRQ_BASE + 0x0000)
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index a0d1e1298e..97b2fc42ab 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -23,6 +23,8 @@
  #include "trace.h"

I would have put the check in include/hw/ppc/spapr_irq.h but since
SPAPR_XIRQ_BASE is only used in hw/ppc/spapr_irq.c which is always
compiled, this is fine. You might want to change that in case a
respin is asked for.

I had initially tried keeping it in spapr_irq.h , but that would give a build 
break for XICS_IRQ_BASE not defined since that gets defined in spapr_xics.h and 
is included later in some files, however, the QEMU_BUILD_BUG_ON expects it to 
be defined before it reaches here.

ah. good catch. this went unnoticed and is a bit ugly. We should fix
in some ways. May with a define SPAPR_XIRQ_BASE to 0x1000 simply ?

Hmm, I can do that if a re-spin is reqd, or can be done as a separate
patch later also along with other improvements.

yes. This is food for thoughts for further improvements.



Also, we could probably define the ICS offset to SPAPR_XIRQ_BASE
directly under spapr_irq_init() and get rid of ics_instance_init().
The HW IRQ Number offset in the PNV ICS instances is assigned
dynamically by the OS (see pnv_phb3). So it should befine to do
the same for spapr. In which case we can get rid of XICS_IRQ_BASE.

Hmm, I am not so familiar with XICS yet, so not sure if we really need
to do that, but it can be done along with other improvements if needed.







  static const TypeInfo spapr_intc_info = {
      .name = TYPE_SPAPR_INTC,
      .parent = TYPE_INTERFACE,
@@ -329,7 +331,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
          int i;
          dev = qdev_new(TYPE_SPAPR_XIVE);
-        qdev_prop_set_uint32(dev, "nr-irqs", smc->nr_xirqs + SPAPR_XIRQ_BASE);
+        qdev_prop_set_uint32(dev, "nr-irqs", smc->nr_xirqs + 
           * 8 XIVE END structures per CPU. One for each available
           * priority
@@ -356,7 +358,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
      spapr->qirqs = qemu_allocate_irqs(spapr_set_irq, spapr,
-                                      smc->nr_xirqs + SPAPR_XIRQ_BASE);
+                                      smc->nr_xirqs + SPAPR_IRQ_NR_IPIS);
       * Mostly we don't actually need this until reset, except that not

reply via email to

[Prev in Thread] Current Thread [Next in Thread]