[PULL 0/3] ppc queue

[PULL 0/3] ppc queue

[Daniel Henrique Barboza]

The following changes since commit 3e4abe2c92964aadd35344a635b0f32cb487fd5c:

  Merge tag 'pull-block-2022-07-27' of https://gitlab.com/vsementsov/qemu into staging (2022-07-27 20:10:15 -0700)

are available in the Git repository at:

  https://gitlab.com/danielhb/qemu.git pull-ppc-20220728

for you to fetch changes up to 0c9717ff35d2fe46fa9cb91566fe2afbed9f4f2a:

  target/ppc: Implement new wait variants (2022-07-28 13:30:41 -0300)

----------------------------------------------------------------
ppc patch queue for 2022-07-28:

Short queue with 2 Coverity fixes and one fix of the
'wait' insns that is causing hangs if the guest kernel uses
the most up to date wait opcode.

- target/ppc:
  - implement new wait variants to fix guest hang when using the new opcode
- ppc440_uc: initialize length passed to cpu_physical_memory_map()
- spapr_nvdimm: check if spapr_drc_index() returns NULL

----------------------------------------------------------------
Daniel Henrique Barboza (1):
      hw/ppc: check if spapr_drc_index() returns NULL in spapr_nvdimm.c

Nicholas Piggin (1):
      target/ppc: Implement new wait variants

Peter Maydell (1):
      hw/ppc/ppc440_uc: Initialize length passed to cpu_physical_memory_map()

 hw/ppc/ppc440_uc.c     |  5 ++-
 hw/ppc/spapr_nvdimm.c  | 18 +++++++---
 target/ppc/internal.h  |  3 ++
 target/ppc/translate.c | 96 +++++++++++++++++++++++++++++++++++++++++++++-----
 4 files changed, 109 insertions(+), 13 deletions(-)

[PULL 1/3] hw/ppc: check if spapr_drc_index() returns NULL in spapr_nvdimm.c

[Daniel Henrique Barboza]

spapr_nvdimm_flush_completion_cb() and flush_worker_cb() are using the
DRC object returned by spapr_drc_index() without checking it for NULL.
In this case we would be dereferencing a NULL pointer when doing
SPAPR_NVDIMM(drc->dev) and PC_DIMM(drc->dev).

This can happen if, during a scm_flush(), the DRC object is wrongly
freed/released (e.g. a bug in another part of the code).
spapr_drc_index() would then return NULL in the callbacks.

Fixes: Coverity CID 1487108, 1487178
Reviewed-by: Greg Kurz <groug@kaod.org>
Message-Id: <20220409200856.283076-2-danielhb413@gmail.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_nvdimm.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index c4c97da5de..04a64cada3 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -447,9 +447,15 @@ static int flush_worker_cb(void *opaque)
 {
     SpaprNVDIMMDeviceFlushState *state = opaque;
     SpaprDrc *drc = spapr_drc_by_index(state->drcidx);
-    PCDIMMDevice *dimm = PC_DIMM(drc->dev);
-    HostMemoryBackend *backend = MEMORY_BACKEND(dimm->hostmem);
-    int backend_fd = memory_region_get_fd(&backend->mr);
+    PCDIMMDevice *dimm;
+    HostMemoryBackend *backend;
+    int backend_fd;
+
+    g_assert(drc != NULL);
+
+    dimm = PC_DIMM(drc->dev);
+    backend = MEMORY_BACKEND(dimm->hostmem);
+    backend_fd = memory_region_get_fd(&backend->mr);
 
     if (object_property_get_bool(OBJECT(backend), "pmem", NULL)) {
         MemoryRegion *mr = host_memory_backend_get_memory(dimm->hostmem);
@@ -475,7 +481,11 @@ static void spapr_nvdimm_flush_completion_cb(void *opaque, int hcall_ret)
 {
     SpaprNVDIMMDeviceFlushState *state = opaque;
     SpaprDrc *drc = spapr_drc_by_index(state->drcidx);
-    SpaprNVDIMMDevice *s_nvdimm = SPAPR_NVDIMM(drc->dev);
+    SpaprNVDIMMDevice *s_nvdimm;
+
+    g_assert(drc != NULL);
+
+    s_nvdimm = SPAPR_NVDIMM(drc->dev);
 
     state->hcall_ret = hcall_ret;
     QLIST_REMOVE(state, node);
-- 
2.36.1

[PULL 2/3] hw/ppc/ppc440_uc: Initialize length passed to cpu_physical_memory_map()

[Daniel Henrique Barboza]

From: Peter Maydell <peter.maydell@linaro.org>

In dcr_write_dma(), there is code that uses cpu_physical_memory_map()
to implement a DMA transfer.  That function takes a 'plen' argument,
which points to a hwaddr which is used for both input and output: the
caller must set it to the size of the range it wants to map, and on
return it is updated to the actual length mapped. The dcr_write_dma()
code fails to initialize rlen and wlen, so will end up mapping an
unpredictable amount of memory.

Initialize the length values correctly, and check that we managed to
map the entire range before using the fast-path memmove().

This was spotted by Coverity, which points out that we never
initialized the variables before using them.

Fixes: Coverity CID 1487137, 1487150
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20220726182341.1888115-2-peter.maydell@linaro.org>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/ppc440_uc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
index a1ecf6dd1c..11fdb88c22 100644
--- a/hw/ppc/ppc440_uc.c
+++ b/hw/ppc/ppc440_uc.c
@@ -904,14 +904,17 @@ static void dcr_write_dma(void *opaque, int dcrn, uint32_t val)
                     int width, i, sidx, didx;
                     uint8_t *rptr, *wptr;
                     hwaddr rlen, wlen;
+                    hwaddr xferlen;
 
                     sidx = didx = 0;
                     width = 1 << ((val & DMA0_CR_PW) >> 25);
+                    xferlen = count * width;
+                    wlen = rlen = xferlen;
                     rptr = cpu_physical_memory_map(dma->ch[chnl].sa, &rlen,
                                                    false);
                     wptr = cpu_physical_memory_map(dma->ch[chnl].da, &wlen,
                                                    true);
-                    if (rptr && wptr) {
+                    if (rptr && rlen == xferlen && wptr && wlen == xferlen) {
                         if (!(val & DMA0_CR_DEC) &&
                             val & DMA0_CR_SAI && val & DMA0_CR_DAI) {
                             /* optimise common case */
-- 
2.36.1

[PULL 3/3] target/ppc: Implement new wait variants

[Daniel Henrique Barboza]

From: Nicholas Piggin <npiggin@gmail.com>

ISA v2.06 adds new variations of wait, specified by the WC field. These
are not all compatible with the prior wait implementation, because they
add additional conditions that cause the processor to resume, which can
cause software to hang or run very slowly.

At this moment, with the current wait implementation and a pseries guest
using mainline kernel with new wait upcodes [1], QEMU hangs during boot if
more than one CPU is present:

 qemu-system-ppc64 -M pseries,x-vof=on -cpu POWER10 -smp 2 -nographic
-kernel zImage.pseries -no-reboot

QEMU will exit (as there's no filesystem) if the test "passes", or hang
during boot if it hits the bug.

ISA v3.0 changed the wait opcode and removed the new variants (retaining
the WC field but making non-zero values reserved).

ISA v3.1 added new WC values to the new wait opcode, and added a PL
field.

This patch implements the new wait encoding and supports WC variants
with no-op implementations, which provides basic correctness as
explained in comments.

[1] https://lore.kernel.org/all/20220720132132.903462-1-npiggin@gmail.com/

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Víctor Colombo <victor.colombo@eldorado.org.br>
Tested-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Message-Id: <20220720133352.904263-1-npiggin@gmail.com>
[danielhb: added information about the bug being fixed]
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/internal.h  |  3 ++
 target/ppc/translate.c | 96 ++++++++++++++++++++++++++++++++++++++----
 2 files changed, 91 insertions(+), 8 deletions(-)

diff --git a/target/ppc/internal.h b/target/ppc/internal.h
index 467f3046c8..337a362205 100644
--- a/target/ppc/internal.h
+++ b/target/ppc/internal.h
@@ -165,6 +165,9 @@ EXTRACT_HELPER_SPLIT_3(DX, 10, 6, 6, 5, 16, 1, 1, 0, 0)
 /* darn */
 EXTRACT_HELPER(L, 16, 2);
 #endif
+/* wait */
+EXTRACT_HELPER(WC, 21, 2);
+EXTRACT_HELPER(PL, 16, 2);
 
 /***                            Jump target decoding                       ***/
 /* Immediate address */
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 5a18ee577f..388337f81b 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -4071,12 +4071,91 @@ static void gen_sync(DisasContext *ctx)
 /* wait */
 static void gen_wait(DisasContext *ctx)
 {
-    TCGv_i32 t0 = tcg_const_i32(1);
-    tcg_gen_st_i32(t0, cpu_env,
-                   -offsetof(PowerPCCPU, env) + offsetof(CPUState, halted));
-    tcg_temp_free_i32(t0);
-    /* Stop translation, as the CPU is supposed to sleep from now */
-    gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);
+    uint32_t wc;
+
+    if (ctx->insns_flags & PPC_WAIT) {
+        /* v2.03-v2.07 define an older incompatible 'wait' encoding. */
+
+        if (ctx->insns_flags2 & PPC2_PM_ISA206) {
+            /* v2.06 introduced the WC field. WC > 0 may be treated as no-op. */
+            wc = WC(ctx->opcode);
+        } else {
+            wc = 0;
+        }
+
+    } else if (ctx->insns_flags2 & PPC2_ISA300) {
+        /* v3.0 defines a new 'wait' encoding. */
+        wc = WC(ctx->opcode);
+        if (ctx->insns_flags2 & PPC2_ISA310) {
+            uint32_t pl = PL(ctx->opcode);
+
+            /* WC 1,2 may be treated as no-op. WC 3 is reserved. */
+            if (wc == 3) {
+                gen_invalid(ctx);
+                return;
+            }
+
+            /* PL 1-3 are reserved. If WC=2 then the insn is treated as noop. */
+            if (pl > 0 && wc != 2) {
+                gen_invalid(ctx);
+                return;
+            }
+
+        } else { /* ISA300 */
+            /* WC 1-3 are reserved */
+            if (wc > 0) {
+                gen_invalid(ctx);
+                return;
+            }
+        }
+
+    } else {
+        warn_report("wait instruction decoded with wrong ISA flags.");
+        gen_invalid(ctx);
+        return;
+    }
+
+    /*
+     * wait without WC field or with WC=0 waits for an exception / interrupt
+     * to occur.
+     */
+    if (wc == 0) {
+        TCGv_i32 t0 = tcg_const_i32(1);
+        tcg_gen_st_i32(t0, cpu_env,
+                       -offsetof(PowerPCCPU, env) + offsetof(CPUState, halted));
+        tcg_temp_free_i32(t0);
+        /* Stop translation, as the CPU is supposed to sleep from now */
+        gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);
+    }
+
+    /*
+     * Other wait types must not just wait until an exception occurs because
+     * ignoring their other wake-up conditions could cause a hang.
+     *
+     * For v2.06 and 2.07, wc=1,2,3 are architected but may be implemented as
+     * no-ops.
+     *
+     * wc=1 and wc=3 explicitly allow the instruction to be treated as a no-op.
+     *
+     * wc=2 waits for an implementation-specific condition, such could be
+     * always true, so it can be implemented as a no-op.
+     *
+     * For v3.1, wc=1,2 are architected but may be implemented as no-ops.
+     *
+     * wc=1 (waitrsv) waits for an exception or a reservation to be lost.
+     * Reservation-loss may have implementation-specific conditions, so it
+     * can be implemented as a no-op.
+     *
+     * wc=2 waits for an exception or an amount of time to pass. This
+     * amount is implementation-specific so it can be implemented as a
+     * no-op.
+     *
+     * ISA v3.1 allows for execution to resume "in the rare case of
+     * an implementation-dependent event", so in any case software must
+     * not depend on the architected resumption condition to become
+     * true, so no-op implementations should be architecturally correct
+     * (if suboptimal).
+     */
 }
 
 #if defined(TARGET_PPC64)
@@ -6691,8 +6770,9 @@ GEN_HANDLER2(stdcx_, "stdcx.", 0x1F, 0x16, 0x06, 0x00000000, PPC_64B),
 GEN_HANDLER_E(stqcx_, 0x1F, 0x16, 0x05, 0, PPC_NONE, PPC2_LSQ_ISA207),
 #endif
 GEN_HANDLER(sync, 0x1F, 0x16, 0x12, 0x039FF801, PPC_MEM_SYNC),
-GEN_HANDLER(wait, 0x1F, 0x1E, 0x01, 0x03FFF801, PPC_WAIT),
-GEN_HANDLER_E(wait, 0x1F, 0x1E, 0x00, 0x039FF801, PPC_NONE, PPC2_ISA300),
+/* ISA v3.0 changed the extended opcode from 62 to 30 */
+GEN_HANDLER(wait, 0x1F, 0x1E, 0x01, 0x039FF801, PPC_WAIT),
+GEN_HANDLER_E(wait, 0x1F, 0x1E, 0x00, 0x039CF801, PPC_NONE, PPC2_ISA300),
 GEN_HANDLER(b, 0x12, 0xFF, 0xFF, 0x00000000, PPC_FLOW),
 GEN_HANDLER(bc, 0x10, 0xFF, 0xFF, 0x00000000, PPC_FLOW),
 GEN_HANDLER(bcctr, 0x13, 0x10, 0x10, 0x00000000, PPC_FLOW),
-- 
2.36.1

Re: [PULL 0/3] ppc queue

[Richard Henderson]

On 7/28/22 09:55, Daniel Henrique Barboza wrote:
>    https://gitlab.com/danielhb/qemu.git  pull-ppc-20220728

fatal: couldn't find remote ref pull-ppc-20220728


Did you forget to push the tag to gitlab?


r~

Re: [PULL 0/3] ppc queue

[Daniel Henrique Barboza]

On 7/28/22 17:18, Richard Henderson wrote:
> On 7/28/22 09:55, Daniel Henrique Barboza wrote:
>>    https://gitlab.com/danielhb/qemu.git  pull-ppc-20220728
> 
> fatal: couldn't find remote ref pull-ppc-20220728
> 
> 
> Did you forget to push the tag to gitlab?

I guess I mistyped the credentials when running make-pullreq.sh and the
tag wasn't pushed. Can you try again? It is pushed now:

https://gitlab.com/danielhb/qemu/-/commits/pull-ppc-20220728


Thanks,


Daniel

> 
> 
> r~

Re: [PULL 0/3] ppc queue

[Richard Henderson]

On 7/28/22 09:55, Daniel Henrique Barboza wrote:
> The following changes since commit 3e4abe2c92964aadd35344a635b0f32cb487fd5c:
> 
>    Merge tag 'pull-block-2022-07-27' of https://gitlab.com/vsementsov/qemu into staging (2022-07-27 20:10:15 -0700)
> 
> are available in the Git repository at:
> 
>    https://gitlab.com/danielhb/qemu.git pull-ppc-20220728
> 
> for you to fetch changes up to 0c9717ff35d2fe46fa9cb91566fe2afbed9f4f2a:
> 
>    target/ppc: Implement new wait variants (2022-07-28 13:30:41 -0300)
> 
> ----------------------------------------------------------------
> ppc patch queue for 2022-07-28:
> 
> Short queue with 2 Coverity fixes and one fix of the
> 'wait' insns that is causing hangs if the guest kernel uses
> the most up to date wait opcode.
> 
> - target/ppc:
>    - implement new wait variants to fix guest hang when using the new opcode
> - ppc440_uc: initialize length passed to cpu_physical_memory_map()
> - spapr_nvdimm: check if spapr_drc_index() returns NULL

Applied, thanks.  Please update https://wiki.qemu.org/ChangeLog/7.1 as appropriate.


r~


> 
> ----------------------------------------------------------------
> Daniel Henrique Barboza (1):
>        hw/ppc: check if spapr_drc_index() returns NULL in spapr_nvdimm.c
> 
> Nicholas Piggin (1):
>        target/ppc: Implement new wait variants
> 
> Peter Maydell (1):
>        hw/ppc/ppc440_uc: Initialize length passed to cpu_physical_memory_map()
> 
>   hw/ppc/ppc440_uc.c     |  5 ++-
>   hw/ppc/spapr_nvdimm.c  | 18 +++++++---
>   target/ppc/internal.h  |  3 ++
>   target/ppc/translate.c | 96 +++++++++++++++++++++++++++++++++++++++++++++-----
>   4 files changed, 109 insertions(+), 13 deletions(-)