[PULL 0/4] target-arm queue

[PULL 0/4] target-arm queue

[Peter Maydell]

Just 4 bug fixes here...

thanks
-- PMM

The following changes since commit e9d2db818ff934afb366aea566d0b33acf7bced1:

  Merge tag 'for-upstream' of https://gitlab.com/bonzini/qemu into staging (2024-08-01 07:31:49 +1000)

are available in the Git repository at:

  https://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20240801

for you to fetch changes up to 5e8e4f098d872818aa9a138a171200068b81c8d1:

  target/xtensa: Correct assert condition in handle_interrupt() (2024-08-01 10:59:01 +0100)

----------------------------------------------------------------
target-arm queue:
 * hw/arm/mps2-tz.c: fix RX/TX interrupts order
 * accel/kvm/kvm-all: Fixes the missing break in vCPU unpark logic
 * target/arm: Handle denormals correctly for FMOPA (widening)
 * target/xtensa: Correct assert condition in handle_interrupt()

----------------------------------------------------------------
Marco Palumbi (1):
      hw/arm/mps2-tz.c: fix RX/TX interrupts order

Peter Maydell (2):
      target/arm: Handle denormals correctly for FMOPA (widening)
      target/xtensa: Correct assert condition in handle_interrupt()

Salil Mehta (1):
      accel/kvm/kvm-all: Fixes the missing break in vCPU unpark logic

 target/arm/tcg/helper-sme.h    |  2 +-
 accel/kvm/kvm-all.c            |  1 +
 hw/arm/mps2-tz.c               |  6 +++---
 target/arm/tcg/sme_helper.c    | 39 +++++++++++++++++++++++++++------------
 target/arm/tcg/translate-sme.c | 25 +++++++++++++++++++++++--
 target/xtensa/exc_helper.c     |  2 +-
 6 files changed, 56 insertions(+), 19 deletions(-)

[PULL 1/4] hw/arm/mps2-tz.c: fix RX/TX interrupts order

[Peter Maydell]

From: Marco Palumbi <Marco.Palumbi@tii.ae>

The order of the RX and TX interrupts are swapped.
This commit fixes the order as per the following documents:
 * https://developer.arm.com/documentation/dai0505/latest/
 * https://developer.arm.com/documentation/dai0521/latest/
 * https://developer.arm.com/documentation/dai0524/latest/
 * https://developer.arm.com/documentation/dai0547/latest/

Cc: qemu-stable@nongnu.org
Signed-off-by: Marco Palumbi <Marco.Palumbi@tii.ae>
Message-id: 20240730073123.72992-1-marco@palumbi.it
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/mps2-tz.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
index a2d18afd792..aec57c0d686 100644
--- a/hw/arm/mps2-tz.c
+++ b/hw/arm/mps2-tz.c
@@ -435,7 +435,7 @@ static MemoryRegion *make_uart(MPS2TZMachineState *mms, void *opaque,
                                const char *name, hwaddr size,
                                const int *irqs, const PPCExtraData *extradata)
 {
-    /* The irq[] array is tx, rx, combined, in that order */
+    /* The irq[] array is rx, tx, combined, in that order */
     MPS2TZMachineClass *mmc = MPS2TZ_MACHINE_GET_CLASS(mms);
     CMSDKAPBUART *uart = opaque;
     int i = uart - &mms->uart[0];
@@ -447,8 +447,8 @@ static MemoryRegion *make_uart(MPS2TZMachineState *mms, void *opaque,
     qdev_prop_set_uint32(DEVICE(uart), "pclk-frq", mmc->apb_periph_frq);
     sysbus_realize(SYS_BUS_DEVICE(uart), &error_fatal);
     s = SYS_BUS_DEVICE(uart);
-    sysbus_connect_irq(s, 0, get_sse_irq_in(mms, irqs[0]));
-    sysbus_connect_irq(s, 1, get_sse_irq_in(mms, irqs[1]));
+    sysbus_connect_irq(s, 0, get_sse_irq_in(mms, irqs[1]));
+    sysbus_connect_irq(s, 1, get_sse_irq_in(mms, irqs[0]));
     sysbus_connect_irq(s, 2, qdev_get_gpio_in(orgate_dev, i * 2));
     sysbus_connect_irq(s, 3, qdev_get_gpio_in(orgate_dev, i * 2 + 1));
     sysbus_connect_irq(s, 4, get_sse_irq_in(mms, irqs[2]));
-- 
2.34.1

[PULL 2/4] accel/kvm/kvm-all: Fixes the missing break in vCPU unpark logic

[Peter Maydell]

From: Salil Mehta <salil.mehta@huawei.com>

Loop should exit prematurely on successfully finding out the parked vCPU (struct
KVMParkedVcpu) in the 'struct KVMState' maintained 'kvm_parked_vcpus' list of
parked vCPUs.

Fixes: Coverity CID 1558552
Fixes: 08c3286822 ("accel/kvm: Extract common KVM vCPU {creation,parking} code")
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Message-id: 20240725145132.99355-1-salil.mehta@huawei.com
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Message-ID: <CAFEAcA-3_d1c7XSXWkFubD-LsW5c5i95e6xxV09r2C9yGtzcdA@mail.gmail.com>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 accel/kvm/kvm-all.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index e1d13863063..75d11a07b2b 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -362,6 +362,7 @@ int kvm_unpark_vcpu(KVMState *s, unsigned long vcpu_id)
             QLIST_REMOVE(cpu, node);
             kvm_fd = cpu->kvm_fd;
             g_free(cpu);
+            break;
         }
     }
 
-- 
2.34.1

[PULL 3/4] target/arm: Handle denormals correctly for FMOPA (widening)

[Peter Maydell]

The FMOPA (widening) SME instruction takes pairs of half-precision
floating point values, widens them to single-precision, does a
two-way dot product and accumulates the results into a
single-precision destination.  We don't quite correctly handle the
FPCR bits FZ and FZ16 which control flushing of denormal inputs and
outputs.  This is because at the moment we pass a single float_status
value to the helper function, which then uses that configuration for
all the fp operations it does.  However, because the inputs to this
operation are float16 and the outputs are float32 we need to use the
fp_status_f16 for the float16 input widening but the normal fp_status
for everything else.  Otherwise we will apply the flushing control
FPCR.FZ16 to the 32-bit output rather than the FPCR.FZ control, and
incorrectly flush a denormal output to zero when we should not (or
vice-versa).

(In commit 207d30b5fdb5b we tried to fix the FZ handling but
didn't get it right, switching from "use FPCR.FZ for everything" to
"use FPCR.FZ16 for everything".)

Pass the CPU env to the sme_fmopa_h helper instead of an fp_status
pointer, and have the helper pass an extra fp_status into the
f16_dotadd() function so that we can use the right status for the
right parts of this operation.

Cc: qemu-stable@nongnu.org
Fixes: 207d30b5fdb5 ("target/arm: Use FPST_F16 for SME FMOPA (widening)")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2373
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/tcg/helper-sme.h    |  2 +-
 target/arm/tcg/sme_helper.c    | 39 +++++++++++++++++++++++-----------
 target/arm/tcg/translate-sme.c | 25 ++++++++++++++++++++--
 3 files changed, 51 insertions(+), 15 deletions(-)

diff --git a/target/arm/tcg/helper-sme.h b/target/arm/tcg/helper-sme.h
index 27eef49a11e..d22bf9d21b0 100644
--- a/target/arm/tcg/helper-sme.h
+++ b/target/arm/tcg/helper-sme.h
@@ -121,7 +121,7 @@ DEF_HELPER_FLAGS_5(sme_addha_d, TCG_CALL_NO_RWG, void, ptr, ptr, ptr, ptr, i32)
 DEF_HELPER_FLAGS_5(sme_addva_d, TCG_CALL_NO_RWG, void, ptr, ptr, ptr, ptr, i32)
 
 DEF_HELPER_FLAGS_7(sme_fmopa_h, TCG_CALL_NO_RWG,
-                   void, ptr, ptr, ptr, ptr, ptr, ptr, i32)
+                   void, ptr, ptr, ptr, ptr, ptr, env, i32)
 DEF_HELPER_FLAGS_7(sme_fmopa_s, TCG_CALL_NO_RWG,
                    void, ptr, ptr, ptr, ptr, ptr, ptr, i32)
 DEF_HELPER_FLAGS_7(sme_fmopa_d, TCG_CALL_NO_RWG,
diff --git a/target/arm/tcg/sme_helper.c b/target/arm/tcg/sme_helper.c
index 3ba826a6ceb..02106809ce1 100644
--- a/target/arm/tcg/sme_helper.c
+++ b/target/arm/tcg/sme_helper.c
@@ -992,12 +992,23 @@ static inline uint32_t f16mop_adj_pair(uint32_t pair, uint32_t pg, uint32_t neg)
 }
 
 static float32 f16_dotadd(float32 sum, uint32_t e1, uint32_t e2,
-                          float_status *s_std, float_status *s_odd)
+                          float_status *s_f16, float_status *s_std,
+                          float_status *s_odd)
 {
-    float64 e1r = float16_to_float64(e1 & 0xffff, true, s_std);
-    float64 e1c = float16_to_float64(e1 >> 16, true, s_std);
-    float64 e2r = float16_to_float64(e2 & 0xffff, true, s_std);
-    float64 e2c = float16_to_float64(e2 >> 16, true, s_std);
+    /*
+     * We need three different float_status for different parts of this
+     * operation:
+     *  - the input conversion of the float16 values must use the
+     *    f16-specific float_status, so that the FPCR.FZ16 control is applied
+     *  - operations on float32 including the final accumulation must use
+     *    the normal float_status, so that FPCR.FZ is applied
+     *  - we have pre-set-up copy of s_std which is set to round-to-odd,
+     *    for the multiply (see below)
+     */
+    float64 e1r = float16_to_float64(e1 & 0xffff, true, s_f16);
+    float64 e1c = float16_to_float64(e1 >> 16, true, s_f16);
+    float64 e2r = float16_to_float64(e2 & 0xffff, true, s_f16);
+    float64 e2c = float16_to_float64(e2 >> 16, true, s_f16);
     float64 t64;
     float32 t32;
 
@@ -1019,20 +1030,23 @@ static float32 f16_dotadd(float32 sum, uint32_t e1, uint32_t e2,
 }
 
 void HELPER(sme_fmopa_h)(void *vza, void *vzn, void *vzm, void *vpn,
-                         void *vpm, void *vst, uint32_t desc)
+                         void *vpm, CPUARMState *env, uint32_t desc)
 {
     intptr_t row, col, oprsz = simd_maxsz(desc);
     uint32_t neg = simd_data(desc) * 0x80008000u;
     uint16_t *pn = vpn, *pm = vpm;
-    float_status fpst_odd, fpst_std;
+    float_status fpst_odd, fpst_std, fpst_f16;
 
     /*
-     * Make a copy of float_status because this operation does not
-     * update the cumulative fp exception status.  It also produces
-     * default nans.  Make a second copy with round-to-odd -- see above.
+     * Make copies of fp_status and fp_status_f16, because this operation
+     * does not update the cumulative fp exception status.  It also
+     * produces default NaNs. We also need a second copy of fp_status with
+     * round-to-odd -- see above.
      */
-    fpst_std = *(float_status *)vst;
+    fpst_f16 = env->vfp.fp_status_f16;
+    fpst_std = env->vfp.fp_status;
     set_default_nan_mode(true, &fpst_std);
+    set_default_nan_mode(true, &fpst_f16);
     fpst_odd = fpst_std;
     set_float_rounding_mode(float_round_to_odd, &fpst_odd);
 
@@ -1052,7 +1066,8 @@ void HELPER(sme_fmopa_h)(void *vza, void *vzn, void *vzm, void *vpn,
                         uint32_t m = *(uint32_t *)(vzm + H1_4(col));
 
                         m = f16mop_adj_pair(m, pcol, 0);
-                        *a = f16_dotadd(*a, n, m, &fpst_std, &fpst_odd);
+                        *a = f16_dotadd(*a, n, m,
+                                        &fpst_f16, &fpst_std, &fpst_odd);
                     }
                     col += 4;
                     pcol >>= 4;
diff --git a/target/arm/tcg/translate-sme.c b/target/arm/tcg/translate-sme.c
index a50a419af27..ae42ddef7b3 100644
--- a/target/arm/tcg/translate-sme.c
+++ b/target/arm/tcg/translate-sme.c
@@ -334,8 +334,29 @@ static bool do_outprod_fpst(DisasContext *s, arg_op *a, MemOp esz,
     return true;
 }
 
-TRANS_FEAT(FMOPA_h, aa64_sme, do_outprod_fpst, a,
-           MO_32, FPST_FPCR_F16, gen_helper_sme_fmopa_h)
+static bool do_outprod_env(DisasContext *s, arg_op *a, MemOp esz,
+                           gen_helper_gvec_5_ptr *fn)
+{
+    int svl = streaming_vec_reg_size(s);
+    uint32_t desc = simd_desc(svl, svl, a->sub);
+    TCGv_ptr za, zn, zm, pn, pm;
+
+    if (!sme_smza_enabled_check(s)) {
+        return true;
+    }
+
+    za = get_tile(s, esz, a->zad);
+    zn = vec_full_reg_ptr(s, a->zn);
+    zm = vec_full_reg_ptr(s, a->zm);
+    pn = pred_full_reg_ptr(s, a->pn);
+    pm = pred_full_reg_ptr(s, a->pm);
+
+    fn(za, zn, zm, pn, pm, tcg_env, tcg_constant_i32(desc));
+    return true;
+}
+
+TRANS_FEAT(FMOPA_h, aa64_sme, do_outprod_env, a,
+           MO_32, gen_helper_sme_fmopa_h)
 TRANS_FEAT(FMOPA_s, aa64_sme, do_outprod_fpst, a,
            MO_32, FPST_FPCR, gen_helper_sme_fmopa_s)
 TRANS_FEAT(FMOPA_d, aa64_sme_f64f64, do_outprod_fpst, a,
-- 
2.34.1

Re: [PULL 3/4] target/arm: Handle denormals correctly for FMOPA (widening)

[Michael Tokarev]

01.08.2024 17:23, Peter Maydell wrote:
> The FMOPA (widening) SME instruction takes pairs of half-precision
> floating point values, widens them to single-precision, does a
> two-way dot product and accumulates the results into a
> single-precision destination.  We don't quite correctly handle the
> FPCR bits FZ and FZ16 which control flushing of denormal inputs and
> outputs.  This is because at the moment we pass a single float_status
> value to the helper function, which then uses that configuration for
> all the fp operations it does.  However, because the inputs to this
> operation are float16 and the outputs are float32 we need to use the
> fp_status_f16 for the float16 input widening but the normal fp_status
> for everything else.  Otherwise we will apply the flushing control
> FPCR.FZ16 to the 32-bit output rather than the FPCR.FZ control, and
> incorrectly flush a denormal output to zero when we should not (or
> vice-versa).
> 
> (In commit 207d30b5fdb5b we tried to fix the FZ handling but
> didn't get it right, switching from "use FPCR.FZ for everything" to
> "use FPCR.FZ16 for everything".)
> 
> Pass the CPU env to the sme_fmopa_h helper instead of an fp_status
> pointer, and have the helper pass an extra fp_status into the
> f16_dotadd() function so that we can use the right status for the
> right parts of this operation.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: 207d30b5fdb5 ("target/arm: Use FPST_F16 for SME FMOPA (widening)")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2373

I know it's too late already, but it looks like this Fixes needs to be:
Fixes: 3916841ac75 ("target/arm: Implement FMOPA, FMOPS (widening)")

FWIW.

Thanks,

/mjt

-- 
GPG Key transition (from rsa2048 to rsa4096) since 2024-04-24.
New key: rsa4096/61AD3D98ECDF2C8E  9D8B E14E 3F2A 9DD7 9199  28F1 61AD 3D98 ECDF 2C8E
Old key: rsa2048/457CE0A0804465C5  6EE1 95D1 886E 8FFB 810D  4324 457C E0A0 8044 65C5
Transition statement: http://www.corpit.ru/mjt/gpg-transition-2024.txt

Re: [PULL 3/4] target/arm: Handle denormals correctly for FMOPA (widening)

[Peter Maydell]

On Fri, 2 Aug 2024 at 08:45, Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> 01.08.2024 17:23, Peter Maydell wrote:
> > The FMOPA (widening) SME instruction takes pairs of half-precision
> > floating point values, widens them to single-precision, does a
> > two-way dot product and accumulates the results into a
> > single-precision destination.  We don't quite correctly handle the
> > FPCR bits FZ and FZ16 which control flushing of denormal inputs and
> > outputs.  This is because at the moment we pass a single float_status
> > value to the helper function, which then uses that configuration for
> > all the fp operations it does.  However, because the inputs to this
> > operation are float16 and the outputs are float32 we need to use the
> > fp_status_f16 for the float16 input widening but the normal fp_status
> > for everything else.  Otherwise we will apply the flushing control
> > FPCR.FZ16 to the 32-bit output rather than the FPCR.FZ control, and
> > incorrectly flush a denormal output to zero when we should not (or
> > vice-versa).
> >
> > (In commit 207d30b5fdb5b we tried to fix the FZ handling but
> > didn't get it right, switching from "use FPCR.FZ for everything" to
> > "use FPCR.FZ16 for everything".)
> >
> > Pass the CPU env to the sme_fmopa_h helper instead of an fp_status
> > pointer, and have the helper pass an extra fp_status into the
> > f16_dotadd() function so that we can use the right status for the
> > right parts of this operation.
> >
> > Cc: qemu-stable@nongnu.org
> > Fixes: 207d30b5fdb5 ("target/arm: Use FPST_F16 for SME FMOPA (widening)")
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2373
>
> I know it's too late already, but it looks like this Fixes needs to be:
> Fixes: 3916841ac75 ("target/arm: Implement FMOPA, FMOPS (widening)")

It's fixing a mistake in 207d30b5fdb5, which is in turn fixing
a mistake in 3916841ac75 (but didn't quite get it right).

-- PMM

[PULL 4/4] target/xtensa: Correct assert condition in handle_interrupt()

[Peter Maydell]

In commit ad18376b90c8101 we added an assert that the level value was
in-bounds for the array we're about to index into.  However, the
assert condition is wrong -- env->config->interrupt_vector is an
array of uint32_t, so we should bounds check the index against
ARRAY_SIZE(...), not against sizeof().

Resolves: Coverity CID 1507131
Fixes: ad18376b90c8101 ("target/xtensa: Assert that interrupt level is within bounds")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Acked-by: Max Filippov <jcmvbkbc@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-id: 20240731172246.3682311-1-peter.maydell@linaro.org
---
 target/xtensa/exc_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/xtensa/exc_helper.c b/target/xtensa/exc_helper.c
index 0514c2c1f32..ca629f071d1 100644
--- a/target/xtensa/exc_helper.c
+++ b/target/xtensa/exc_helper.c
@@ -171,7 +171,7 @@ static void handle_interrupt(CPUXtensaState *env)
 
         if (level > 1) {
             /* env->config->nlevel check should have ensured this */
-            assert(level < sizeof(env->config->interrupt_vector));
+            assert(level < ARRAY_SIZE(env->config->interrupt_vector));
 
             env->sregs[EPC1 + level - 1] = env->pc;
             env->sregs[EPS2 + level - 2] = env->sregs[PS];
-- 
2.34.1

Re: [PULL 0/4] target-arm queue

[Richard Henderson]

On 8/2/24 00:23, Peter Maydell wrote:
> Just 4 bug fixes here...
> 
> thanks
> -- PMM
> 
> The following changes since commit e9d2db818ff934afb366aea566d0b33acf7bced1:
> 
>    Merge tag 'for-upstream' ofhttps://gitlab.com/bonzini/qemu into staging (2024-08-01 07:31:49 +1000)
> 
> are available in the Git repository at:
> 
>    https://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20240801
> 
> for you to fetch changes up to 5e8e4f098d872818aa9a138a171200068b81c8d1:
> 
>    target/xtensa: Correct assert condition in handle_interrupt() (2024-08-01 10:59:01 +0100)
> 
> ----------------------------------------------------------------
> target-arm queue:
>   * hw/arm/mps2-tz.c: fix RX/TX interrupts order
>   * accel/kvm/kvm-all: Fixes the missing break in vCPU unpark logic
>   * target/arm: Handle denormals correctly for FMOPA (widening)
>   * target/xtensa: Correct assert condition in handle_interrupt()

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

r~