[PATCH 0/7] various ppc fixes

[PATCH 0/7] various ppc fixes

[Nicholas Piggin]

This fixes LPC serirq Coverity issues introduced in the merge
window that Cedric reported. Also fixes for an assorted bunch of 
emulation issues recently turned up when running PowerVM firmware
on the model.

Thanks,
Nick

Nicholas Piggin (7):
  ppc/pnv: Fix LPC serirq routing calculation
  ppc/pnv: Fix LPC POWER8 register sanity check
  target/ppc: Fix mtDPDES targeting SMT siblings
  target/ppc: PMIs are level triggered
  target/ppc: Fix doorbell delivery to threads in powersave
  target/ppc: Fix HFSCR facility checks
  target/ppc: Fix VRMA to not check virtual page class key protection

 target/ppc/cpu.h         |  5 +++--
 hw/ppc/pnv_lpc.c         | 14 ++++++++++----
 target/ppc/excp_helper.c | 21 +++++++++++++--------
 target/ppc/misc_helper.c |  2 +-
 target/ppc/mmu-hash64.c  |  9 ++++++++-
 5 files changed, 35 insertions(+), 16 deletions(-)

-- 
2.45.2

[PATCH 1/7] ppc/pnv: Fix LPC serirq routing calculation

[Nicholas Piggin]

The serirq routing table is split over two registers, the calculation
for the high irqs in the second register did not subtract the irq
offset. This was spotted by Coverity as a shift-by-negative. Fix this
and change the open-coded shifting and masking to use extract32()
function so it's less error-prone.

This went unnoticed because irqs >= 14 are not used in a standard
QEMU/OPAL boot, changing the first QEMU serial-isa irq to 14 to test
does demonstrate serial irqs aren't received, and that this change
fixes that.

Reported-by: Cédric Le Goater <clg@redhat.com>
Resolves: Coverity CID 1558829 (partially)
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/cpu.h |  1 +
 hw/ppc/pnv_lpc.c | 10 ++++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 321ed2da75..bd32a1a5f8 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -40,6 +40,7 @@
 
 #define PPC_BIT_NR(bit)         (63 - (bit))
 #define PPC_BIT(bit)            (0x8000000000000000ULL >> (bit))
+#define PPC_BIT32_NR(bit)       (31 - (bit))
 #define PPC_BIT32(bit)          (0x80000000 >> (bit))
 #define PPC_BIT8(bit)           (0x80 >> (bit))
 #define PPC_BITMASK(bs, be)     ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
index f8aad955b5..80b79dfbbc 100644
--- a/hw/ppc/pnv_lpc.c
+++ b/hw/ppc/pnv_lpc.c
@@ -435,13 +435,19 @@ static void pnv_lpc_eval_serirq_routes(PnvLpcController *lpc)
         return;
     }
 
+    /*
+     * Each of the ISA irqs is routed to one of the 4 SERIRQ irqs with 2
+     * bits, split across 2 OPB registers.
+     */
     for (irq = 0; irq <= 13; irq++) {
-        int serirq = (lpc->opb_irq_route1 >> (31 - 5 - (irq * 2))) & 0x3;
+        int serirq = extract32(lpc->opb_irq_route1,
+                                    PPC_BIT32_NR(5 + irq * 2), 2);
         lpc->irq_to_serirq_route[irq] = serirq;
     }
 
     for (irq = 14; irq < ISA_NUM_IRQS; irq++) {
-        int serirq = (lpc->opb_irq_route0 >> (31 - 9 - (irq * 2))) & 0x3;
+        int serirq = extract32(lpc->opb_irq_route0,
+                               PPC_BIT32_NR(9 + (irq - 14) * 2), 2);
         lpc->irq_to_serirq_route[irq] = serirq;
     }
 }
-- 
2.45.2

[PATCH 2/7] ppc/pnv: Fix LPC POWER8 register sanity check

[Nicholas Piggin]

POWER8 does not have the ISA IRQ -> SERIRQ routing system of later
CPUs, instead all ISA IRQs are sent to the CPU via a single PSI
interrupt. There is a sanity check in the POWER8 case to ensure the
routing bits have not been set, because that would indicate a
programming error.

Those bits were incorrectly specified because of ppc bit numbering
fun. Coverity detected this as an always-zero expression.

Reported-by: Cédric Le Goater <clg@redhat.com>
Resolves: Coverity CID 1558829 (partially)
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/pnv_lpc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
index 80b79dfbbc..8c203d2059 100644
--- a/hw/ppc/pnv_lpc.c
+++ b/hw/ppc/pnv_lpc.c
@@ -427,8 +427,8 @@ static void pnv_lpc_eval_serirq_routes(PnvLpcController *lpc)
     int irq;
 
     if (!lpc->psi_has_serirq) {
-        if ((lpc->opb_irq_route0 & PPC_BITMASK(8, 13)) ||
-            (lpc->opb_irq_route1 & PPC_BITMASK(4, 31))) {
+        if ((lpc->opb_irq_route0 & PPC_BITMASK32(8, 13)) ||
+            (lpc->opb_irq_route1 & PPC_BITMASK32(4, 31))) {
             qemu_log_mask(LOG_GUEST_ERROR,
                 "OPB: setting serirq routing on POWER8 system, ignoring.\n");
         }
-- 
2.45.2

[PATCH 3/7] target/ppc: Fix mtDPDES targeting SMT siblings

[Nicholas Piggin]

A typo in the loop over SMT threads to set irq level for doorbells
when storing to DPDES meant everything was aimed at the CPU executing
the instruction.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/misc_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
index 1b83971375..f0ca80153b 100644
--- a/target/ppc/misc_helper.c
+++ b/target/ppc/misc_helper.c
@@ -288,7 +288,7 @@ void helper_store_dpdes(CPUPPCState *env, target_ulong val)
         PowerPCCPU *ccpu = POWERPC_CPU(ccs);
         uint32_t thread_id = ppc_cpu_tir(ccpu);
 
-        ppc_set_irq(cpu, PPC_INTERRUPT_DOORBELL, val & (0x1 << thread_id));
+        ppc_set_irq(ccpu, PPC_INTERRUPT_DOORBELL, val & (0x1 << thread_id));
     }
     bql_unlock();
 }
-- 
2.45.2

[PATCH 4/7] target/ppc: PMIs are level triggered

[Nicholas Piggin]

In Book-S / Power processors, the performance monitor interrupts are
driven by the MMCR0[PMAO] bit, which is level triggered and not cleared
by the interrupt.

Others may have different performance monitor architecture, but none of
those are implemented by QEMU.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/excp_helper.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index f33fc36db2..701abe1b6d 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -2187,7 +2187,6 @@ static void p7_deliver_interrupt(CPUPPCState *env, int interrupt)
         powerpc_excp(cpu, POWERPC_EXCP_DECR);
         break;
     case PPC_INTERRUPT_PERFM:
-        env->pending_interrupts &= ~PPC_INTERRUPT_PERFM;
         powerpc_excp(cpu, POWERPC_EXCP_PERFM);
         break;
     case 0:
@@ -2250,7 +2249,6 @@ static void p8_deliver_interrupt(CPUPPCState *env, int interrupt)
         powerpc_excp(cpu, POWERPC_EXCP_SDOOR_HV);
         break;
     case PPC_INTERRUPT_PERFM:
-        env->pending_interrupts &= ~PPC_INTERRUPT_PERFM;
         powerpc_excp(cpu, POWERPC_EXCP_PERFM);
         break;
     case PPC_INTERRUPT_EBB: /* EBB exception */
@@ -2330,7 +2328,6 @@ static void p9_deliver_interrupt(CPUPPCState *env, int interrupt)
         powerpc_excp(cpu, POWERPC_EXCP_SDOOR_HV);
         break;
     case PPC_INTERRUPT_PERFM:
-        env->pending_interrupts &= ~PPC_INTERRUPT_PERFM;
         powerpc_excp(cpu, POWERPC_EXCP_PERFM);
         break;
     case PPC_INTERRUPT_EBB: /* EBB exception */
@@ -2444,7 +2441,6 @@ static void ppc_deliver_interrupt(CPUPPCState *env, int interrupt)
         powerpc_excp(cpu, POWERPC_EXCP_SDOOR_HV);
         break;
     case PPC_INTERRUPT_PERFM:
-        env->pending_interrupts &= ~PPC_INTERRUPT_PERFM;
         powerpc_excp(cpu, POWERPC_EXCP_PERFM);
         break;
     case PPC_INTERRUPT_THERM:  /* Thermal interrupt */
-- 
2.45.2

[PATCH 5/7] target/ppc: Fix doorbell delivery to threads in powersave

[Nicholas Piggin]

Doorbell exceptions are not not cleared when they cause a wake from
powersave state, only when they take the corresponding interrupt.
The sreset-on-wake logic must avoid clearing the interrupt in this
case.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/excp_helper.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 701abe1b6d..b619a6adde 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -2237,7 +2237,9 @@ static void p8_deliver_interrupt(CPUPPCState *env, int interrupt)
         powerpc_excp(cpu, POWERPC_EXCP_DECR);
         break;
     case PPC_INTERRUPT_DOORBELL:
-        env->pending_interrupts &= ~PPC_INTERRUPT_DOORBELL;
+        if (!env->resume_as_sreset) {
+            env->pending_interrupts &= ~PPC_INTERRUPT_DOORBELL;
+        }
         if (is_book3s_arch2x(env)) {
             powerpc_excp(cpu, POWERPC_EXCP_SDOOR);
         } else {
@@ -2245,7 +2247,9 @@ static void p8_deliver_interrupt(CPUPPCState *env, int interrupt)
         }
         break;
     case PPC_INTERRUPT_HDOORBELL:
-        env->pending_interrupts &= ~PPC_INTERRUPT_HDOORBELL;
+        if (!env->resume_as_sreset) {
+            env->pending_interrupts &= ~PPC_INTERRUPT_HDOORBELL;
+        }
         powerpc_excp(cpu, POWERPC_EXCP_SDOOR_HV);
         break;
     case PPC_INTERRUPT_PERFM:
@@ -2301,6 +2305,7 @@ static void p9_deliver_interrupt(CPUPPCState *env, int interrupt)
 
     case PPC_INTERRUPT_HDECR: /* Hypervisor decrementer exception */
         /* HDEC clears on delivery */
+        /* XXX: should not see an HDEC if resume_as_sreset. assert? */
         env->pending_interrupts &= ~PPC_INTERRUPT_HDECR;
         powerpc_excp(cpu, POWERPC_EXCP_HDECR);
         break;
@@ -2320,11 +2325,15 @@ static void p9_deliver_interrupt(CPUPPCState *env, int interrupt)
         powerpc_excp(cpu, POWERPC_EXCP_DECR);
         break;
     case PPC_INTERRUPT_DOORBELL:
-        env->pending_interrupts &= ~PPC_INTERRUPT_DOORBELL;
+        if (!env->resume_as_sreset) {
+            env->pending_interrupts &= ~PPC_INTERRUPT_DOORBELL;
+        }
         powerpc_excp(cpu, POWERPC_EXCP_SDOOR);
         break;
     case PPC_INTERRUPT_HDOORBELL:
-        env->pending_interrupts &= ~PPC_INTERRUPT_HDOORBELL;
+        if (!env->resume_as_sreset) {
+            env->pending_interrupts &= ~PPC_INTERRUPT_HDOORBELL;
+        }
         powerpc_excp(cpu, POWERPC_EXCP_SDOOR_HV);
         break;
     case PPC_INTERRUPT_PERFM:
-- 
2.45.2

[PATCH 6/7] target/ppc: Fix HFSCR facility checks

[Nicholas Piggin]

The HFSCR defines were being encoded as bit masks, but the users
expect (and analogous FSCR defines are) bit numbers.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/cpu.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index bd32a1a5f8..f7a2da2bbe 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -635,8 +635,8 @@ FIELD(MSR, LE, MSR_LE, 1)
 #define PSSCR_EC          PPC_BIT(43) /* Exit Criterion */
 
 /* HFSCR bits */
-#define HFSCR_MSGP     PPC_BIT(53) /* Privileged Message Send Facilities */
-#define HFSCR_BHRB     PPC_BIT(59) /* BHRB Instructions */
+#define HFSCR_MSGP     PPC_BIT_NR(53) /* Privileged Message Send Facilities */
+#define HFSCR_BHRB     PPC_BIT_NR(59) /* BHRB Instructions */
 #define HFSCR_IC_MSGP  0xA
 
 #define DBCR0_ICMP (1 << 27)
-- 
2.45.2

[PATCH 7/7] target/ppc: Fix VRMA to not check virtual page class key protection

[Nicholas Piggin]

Hash virtual real mode addressing is defined by the architecture
to not perform virtual page class key protection checks.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/mmu-hash64.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
index 5e1983e334..c8c2f8910a 100644
--- a/target/ppc/mmu-hash64.c
+++ b/target/ppc/mmu-hash64.c
@@ -993,6 +993,7 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
     int exec_prot, pp_prot, amr_prot, prot;
     int need_prot;
     hwaddr raddr;
+    bool vrma = false;
 
     /*
      * Note on LPCR usage: 970 uses HID4, but our special variant of
@@ -1022,6 +1023,7 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
             }
         } else if (ppc_hash64_use_vrma(env)) {
             /* Emulated VRMA mode */
+            vrma = true;
             slb = &vrma_slbe;
             if (build_vrma_slbe(cpu, slb) != 0) {
                 /* Invalid VRMA setup, machine check */
@@ -1136,7 +1138,12 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
 
     exec_prot = ppc_hash64_pte_noexec_guard(cpu, pte);
     pp_prot = ppc_hash64_pte_prot(mmu_idx, slb, pte);
-    amr_prot = ppc_hash64_amr_prot(cpu, pte);
+    if (vrma) {
+        /* VRMA does not check keys */
+        amr_prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
+    } else {
+        amr_prot = ppc_hash64_amr_prot(cpu, pte);
+    }
     prot = exec_prot & pp_prot & amr_prot;
 
     need_prot = check_prot_access_type(PAGE_RWX, access_type);
-- 
2.45.2

Re: [PATCH 3/7] target/ppc: Fix mtDPDES targeting SMT siblings

[Philippe Mathieu-Daudé]

On 6/8/24 15:13, Nicholas Piggin wrote:
> A typo in the loop over SMT threads to set irq level for doorbells
> when storing to DPDES meant everything was aimed at the CPU executing
> the instruction.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   target/ppc/misc_helper.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Fixes: d24e80b2ae ("target/ppc: Add msgsnd/p and DPDES SMT support")
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 7/7] target/ppc: Fix VRMA to not check virtual page class key protection

[BALATON Zoltan]

On Tue, 6 Aug 2024, Nicholas Piggin wrote:
> Hash virtual real mode addressing is defined by the architecture
> to not perform virtual page class key protection checks.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> target/ppc/mmu-hash64.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index 5e1983e334..c8c2f8910a 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -993,6 +993,7 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
>     int exec_prot, pp_prot, amr_prot, prot;
>     int need_prot;
>     hwaddr raddr;
> +    bool vrma = false;
>
>     /*
>      * Note on LPCR usage: 970 uses HID4, but our special variant of
> @@ -1022,6 +1023,7 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
>             }
>         } else if (ppc_hash64_use_vrma(env)) {
>             /* Emulated VRMA mode */
> +            vrma = true;
>             slb = &vrma_slbe;
>             if (build_vrma_slbe(cpu, slb) != 0) {
>                 /* Invalid VRMA setup, machine check */
> @@ -1136,7 +1138,12 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
>
>     exec_prot = ppc_hash64_pte_noexec_guard(cpu, pte);
>     pp_prot = ppc_hash64_pte_prot(mmu_idx, slb, pte);
> -    amr_prot = ppc_hash64_amr_prot(cpu, pte);
> +    if (vrma) {
> +        /* VRMA does not check keys */
> +        amr_prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;

This can be shortened as PAGE_RWX which I think is simpler but does not 
have to be, only if you want. With that you could also shorten the if to a 
ternary operator as

amr_prot = vrma ? PAGE_RWX : ppc_hash64_amr_prot(cpu, pte);

and save some lines.

Regards,
BALATON Zoltan

> +    } else {
> +        amr_prot = ppc_hash64_amr_prot(cpu, pte);
> +    }
>     prot = exec_prot & pp_prot & amr_prot;
>
>     need_prot = check_prot_access_type(PAGE_RWX, access_type);
>

Re: [PATCH 0/7] various ppc fixes

[Richard Henderson]

On 8/6/24 23:13, Nicholas Piggin wrote:
> Nicholas Piggin (7):
>    ppc/pnv: Fix LPC serirq routing calculation
>    ppc/pnv: Fix LPC POWER8 register sanity check
>    target/ppc: Fix mtDPDES targeting SMT siblings
>    target/ppc: PMIs are level triggered
>    target/ppc: Fix doorbell delivery to threads in powersave
>    target/ppc: Fix HFSCR facility checks
>    target/ppc: Fix VRMA to not check virtual page class key protection

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 1/7] ppc/pnv: Fix LPC serirq routing calculation

[Cédric Le Goater]

On 8/6/24 15:13, Nicholas Piggin wrote:
> The serirq routing table is split over two registers, the calculation
> for the high irqs in the second register did not subtract the irq
> offset. This was spotted by Coverity as a shift-by-negative. Fix this
> and change the open-coded shifting and masking to use extract32()
> function so it's less error-prone.
> 
> This went unnoticed because irqs >= 14 are not used in a standard
> QEMU/OPAL boot, changing the first QEMU serial-isa irq to 14 to test
> does demonstrate serial irqs aren't received, and that this change
> fixes that.
> 
> Reported-by: Cédric Le Goater <clg@redhat.com>
> Resolves: Coverity CID 1558829 (partially)
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   target/ppc/cpu.h |  1 +
>   hw/ppc/pnv_lpc.c | 10 ++++++++--
>   2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 321ed2da75..bd32a1a5f8 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -40,6 +40,7 @@
>   
>   #define PPC_BIT_NR(bit)         (63 - (bit))
>   #define PPC_BIT(bit)            (0x8000000000000000ULL >> (bit))
> +#define PPC_BIT32_NR(bit)       (31 - (bit))
>   #define PPC_BIT32(bit)          (0x80000000 >> (bit))
>   #define PPC_BIT8(bit)           (0x80 >> (bit))
>   #define PPC_BITMASK(bs, be)     ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index f8aad955b5..80b79dfbbc 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -435,13 +435,19 @@ static void pnv_lpc_eval_serirq_routes(PnvLpcController *lpc)
>           return;
>       }
>   
> +    /*
> +     * Each of the ISA irqs is routed to one of the 4 SERIRQ irqs with 2
> +     * bits, split across 2 OPB registers.
> +     */
>       for (irq = 0; irq <= 13; irq++) {
> -        int serirq = (lpc->opb_irq_route1 >> (31 - 5 - (irq * 2))) & 0x3;
> +        int serirq = extract32(lpc->opb_irq_route1,
> +                                    PPC_BIT32_NR(5 + irq * 2), 2);
>           lpc->irq_to_serirq_route[irq] = serirq;
>       }
>   
>       for (irq = 14; irq < ISA_NUM_IRQS; irq++) {
> -        int serirq = (lpc->opb_irq_route0 >> (31 - 9 - (irq * 2))) & 0x3;
> +        int serirq = extract32(lpc->opb_irq_route0,
> +                               PPC_BIT32_NR(9 + (irq - 14) * 2), 2);
>           lpc->irq_to_serirq_route[irq] = serirq;
>       }
>   }

Re: [PATCH 2/7] ppc/pnv: Fix LPC POWER8 register sanity check

[Cédric Le Goater]

On 8/6/24 15:13, Nicholas Piggin wrote:
> POWER8 does not have the ISA IRQ -> SERIRQ routing system of later
> CPUs, instead all ISA IRQs are sent to the CPU via a single PSI
> interrupt. There is a sanity check in the POWER8 case to ensure the
> routing bits have not been set, because that would indicate a
> programming error.
> 
> Those bits were incorrectly specified because of ppc bit numbering
> fun. Coverity detected this as an always-zero expression.
> 
> Reported-by: Cédric Le Goater <clg@redhat.com>
> Resolves: Coverity CID 1558829 (partially)
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/ppc/pnv_lpc.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index 80b79dfbbc..8c203d2059 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -427,8 +427,8 @@ static void pnv_lpc_eval_serirq_routes(PnvLpcController *lpc)
>       int irq;
>   
>       if (!lpc->psi_has_serirq) {
> -        if ((lpc->opb_irq_route0 & PPC_BITMASK(8, 13)) ||
> -            (lpc->opb_irq_route1 & PPC_BITMASK(4, 31))) {
> +        if ((lpc->opb_irq_route0 & PPC_BITMASK32(8, 13)) ||
> +            (lpc->opb_irq_route1 & PPC_BITMASK32(4, 31))) {
>               qemu_log_mask(LOG_GUEST_ERROR,
>                   "OPB: setting serirq routing on POWER8 system, ignoring.\n");
>           }

Re: [PATCH 1/7] ppc/pnv: Fix LPC serirq routing calculation

[Cédric Le Goater]

On 8/6/24 15:13, Nicholas Piggin wrote:
> The serirq routing table is split over two registers, the calculation
> for the high irqs in the second register did not subtract the irq
> offset. This was spotted by Coverity as a shift-by-negative. Fix this
> and change the open-coded shifting and masking to use extract32()
> function so it's less error-prone.
> 
> This went unnoticed because irqs >= 14 are not used in a standard
> QEMU/OPAL boot, changing the first QEMU serial-isa irq to 14 to test
> does demonstrate serial irqs aren't received, and that this change
> fixes that.
> 
> Reported-by: Cédric Le Goater <clg@redhat.com>
> Resolves: Coverity CID 1558829 (partially)
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   target/ppc/cpu.h |  1 +
>   hw/ppc/pnv_lpc.c | 10 ++++++++--
>   2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 321ed2da75..bd32a1a5f8 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -40,6 +40,7 @@
>   
>   #define PPC_BIT_NR(bit)         (63 - (bit))
>   #define PPC_BIT(bit)            (0x8000000000000000ULL >> (bit))
> +#define PPC_BIT32_NR(bit)       (31 - (bit))
>   #define PPC_BIT32(bit)          (0x80000000 >> (bit))
>   #define PPC_BIT8(bit)           (0x80 >> (bit))
>   #define PPC_BITMASK(bs, be)     ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index f8aad955b5..80b79dfbbc 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -435,13 +435,19 @@ static void pnv_lpc_eval_serirq_routes(PnvLpcController *lpc)
>           return;
>       }
>   
> +    /*
> +     * Each of the ISA irqs is routed to one of the 4 SERIRQ irqs with 2
> +     * bits, split across 2 OPB registers.
> +     */
>       for (irq = 0; irq <= 13; irq++) {
> -        int serirq = (lpc->opb_irq_route1 >> (31 - 5 - (irq * 2))) & 0x3;
> +        int serirq = extract32(lpc->opb_irq_route1,
> +                                    PPC_BIT32_NR(5 + irq * 2), 2);
>           lpc->irq_to_serirq_route[irq] = serirq;
>       }
>   
>       for (irq = 14; irq < ISA_NUM_IRQS; irq++) {
> -        int serirq = (lpc->opb_irq_route0 >> (31 - 9 - (irq * 2))) & 0x3;
> +        int serirq = extract32(lpc->opb_irq_route0,
> +                               PPC_BIT32_NR(9 + (irq - 14) * 2), 2);
>           lpc->irq_to_serirq_route[irq] = serirq;
>       }
>   }