[PATCH v1 0/4] targets/microblaze: Handle signed division overflows

[PATCH v1 0/4] targets/microblaze: Handle signed division overflows

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>

First a few preparatory clean-ups followed by the detection and
contional exception of signed division overflow.

Spec:
https://docs.amd.com/r/en-US/ug984-vivado-microblaze-ref/idiv

Cheers,
Edgar

Edgar E. Iglesias (4):
  target/microblaze: Remove unused arg from check_divz()
  target/microblaze: div: Rename and reorder function args
  target/microblaze: Break out raise_divzero()
  target/microblaze: Handle signed division overflows

 target/microblaze/cpu.h       |  1 +
 target/microblaze/op_helper.c | 53 ++++++++++++++++++++++-------------
 target/microblaze/translate.c | 12 ++------
 3 files changed, 36 insertions(+), 30 deletions(-)

-- 
2.43.0

[PATCH v1 1/4] target/microblaze: Remove unused arg from check_divz()

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>

Remove unused arg from check_divz(). No functional change.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
---
 target/microblaze/op_helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/target/microblaze/op_helper.c b/target/microblaze/op_helper.c
index b8365b3b1d..470526ee92 100644
--- a/target/microblaze/op_helper.c
+++ b/target/microblaze/op_helper.c
@@ -69,7 +69,7 @@ void helper_raise_exception(CPUMBState *env, uint32_t index)
     cpu_loop_exit(cs);
 }
 
-static bool check_divz(CPUMBState *env, uint32_t a, uint32_t b, uintptr_t ra)
+static bool check_divz(CPUMBState *env, uint32_t b, uintptr_t ra)
 {
     if (unlikely(b == 0)) {
         env->msr |= MSR_DZ;
@@ -89,7 +89,7 @@ static bool check_divz(CPUMBState *env, uint32_t a, uint32_t b, uintptr_t ra)
 
 uint32_t helper_divs(CPUMBState *env, uint32_t a, uint32_t b)
 {
-    if (!check_divz(env, a, b, GETPC())) {
+    if (!check_divz(env, b, GETPC())) {
         return 0;
     }
     return (int32_t)a / (int32_t)b;
@@ -97,7 +97,7 @@ uint32_t helper_divs(CPUMBState *env, uint32_t a, uint32_t b)
 
 uint32_t helper_divu(CPUMBState *env, uint32_t a, uint32_t b)
 {
-    if (!check_divz(env, a, b, GETPC())) {
+    if (!check_divz(env, b, GETPC())) {
         return 0;
     }
     return a / b;
-- 
2.43.0

Re: [PATCH v1 1/4] target/microblaze: Remove unused arg from check_divz()

[Richard Henderson]

On 8/25/25 08:27, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias"<edgar.iglesias@amd.com>
> 
> Remove unused arg from check_divz(). No functional change.
> 
> Signed-off-by: Edgar E. Iglesias<edgar.iglesias@amd.com>
> ---
>   target/microblaze/op_helper.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)

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

r~

[PATCH v1 2/4] target/microblaze: div: Rename and reorder function args

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>

Rename and reorder function args to better match with spec
and pseudo code.

No functional change.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
---
 target/microblaze/op_helper.c | 18 +++++++++---------
 target/microblaze/translate.c | 12 ++----------
 2 files changed, 11 insertions(+), 19 deletions(-)

diff --git a/target/microblaze/op_helper.c b/target/microblaze/op_helper.c
index 470526ee92..092977b3e1 100644
--- a/target/microblaze/op_helper.c
+++ b/target/microblaze/op_helper.c
@@ -69,9 +69,9 @@ void helper_raise_exception(CPUMBState *env, uint32_t index)
     cpu_loop_exit(cs);
 }
 
-static bool check_divz(CPUMBState *env, uint32_t b, uintptr_t ra)
+static bool check_divz(CPUMBState *env, uint32_t divisor, uintptr_t pc)
 {
-    if (unlikely(b == 0)) {
+    if (unlikely(divisor == 0)) {
         env->msr |= MSR_DZ;
 
         if ((env->msr & MSR_EE) &&
@@ -80,27 +80,27 @@ static bool check_divz(CPUMBState *env, uint32_t b, uintptr_t ra)
 
             env->esr = ESR_EC_DIVZERO;
             cs->exception_index = EXCP_HW_EXCP;
-            cpu_loop_exit_restore(cs, ra);
+            cpu_loop_exit_restore(cs, pc);
         }
         return false;
     }
     return true;
 }
 
-uint32_t helper_divs(CPUMBState *env, uint32_t a, uint32_t b)
+uint32_t helper_divs(CPUMBState *env, uint32_t ra, uint32_t rb)
 {
-    if (!check_divz(env, b, GETPC())) {
+    if (!check_divz(env, ra, GETPC())) {
         return 0;
     }
-    return (int32_t)a / (int32_t)b;
+    return (int32_t)rb / (int32_t)ra;
 }
 
-uint32_t helper_divu(CPUMBState *env, uint32_t a, uint32_t b)
+uint32_t helper_divu(CPUMBState *env, uint32_t ra, uint32_t rb)
 {
-    if (!check_divz(env, b, GETPC())) {
+    if (!check_divz(env, ra, GETPC())) {
         return 0;
     }
-    return a / b;
+    return rb / ra;
 }
 
 /* raise FPU exception.  */
diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
index 5098a1db4d..2f5fd5c271 100644
--- a/target/microblaze/translate.c
+++ b/target/microblaze/translate.c
@@ -450,16 +450,8 @@ DO_TYPEA0_CFG(flt, use_fpu >= 2, true, gen_flt)
 DO_TYPEA0_CFG(fint, use_fpu >= 2, true, gen_fint)
 DO_TYPEA0_CFG(fsqrt, use_fpu >= 2, true, gen_fsqrt)
 
-/* Does not use ENV_WRAPPER3, because arguments are swapped as well. */
-static void gen_idiv(TCGv_i32 out, TCGv_i32 ina, TCGv_i32 inb)
-{
-    gen_helper_divs(out, tcg_env, inb, ina);
-}
-
-static void gen_idivu(TCGv_i32 out, TCGv_i32 ina, TCGv_i32 inb)
-{
-    gen_helper_divu(out, tcg_env, inb, ina);
-}
+ENV_WRAPPER3(gen_idiv, gen_helper_divs)
+ENV_WRAPPER3(gen_idivu, gen_helper_divu)
 
 DO_TYPEA_CFG(idiv, use_div, true, gen_idiv)
 DO_TYPEA_CFG(idivu, use_div, true, gen_idivu)
-- 
2.43.0

Re: [PATCH v1 2/4] target/microblaze: div: Rename and reorder function args

[Richard Henderson]

On 8/25/25 08:27, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
> 
> Rename and reorder function args to better match with spec
> and pseudo code.
> 
> No functional change.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
> ---
>   target/microblaze/op_helper.c | 18 +++++++++---------
>   target/microblaze/translate.c | 12 ++----------
>   2 files changed, 11 insertions(+), 19 deletions(-)
> 
> diff --git a/target/microblaze/op_helper.c b/target/microblaze/op_helper.c
> index 470526ee92..092977b3e1 100644
> --- a/target/microblaze/op_helper.c
> +++ b/target/microblaze/op_helper.c
> @@ -69,9 +69,9 @@ void helper_raise_exception(CPUMBState *env, uint32_t index)
>       cpu_loop_exit(cs);
>   }
>   
> -static bool check_divz(CPUMBState *env, uint32_t b, uintptr_t ra)
> +static bool check_divz(CPUMBState *env, uint32_t divisor, uintptr_t pc)

The name GETPC notwithstanding, I don't think ra -> pc is a good rename.

PC often gets confused about whether that's a host or guest value.
RA (return address) is less so; it gets used often in core tcg.

If you really don't like RA, then perhaps "retaddr" (also with quite a bit of usage) or 
"unwind_pc" (no existing usage, but very descriptive).


r~

Re: [PATCH v1 2/4] target/microblaze: div: Rename and reorder function args

[Philippe Mathieu-Daudé]

On 25/8/25 01:32, Richard Henderson wrote:
> On 8/25/25 08:27, Edgar E. Iglesias wrote:
>> From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
>>
>> Rename and reorder function args to better match with spec
>> and pseudo code.
>>
>> No functional change.
>>
>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
>> ---
>>   target/microblaze/op_helper.c | 18 +++++++++---------
>>   target/microblaze/translate.c | 12 ++----------
>>   2 files changed, 11 insertions(+), 19 deletions(-)
>>
>> diff --git a/target/microblaze/op_helper.c b/target/microblaze/ 
>> op_helper.c
>> index 470526ee92..092977b3e1 100644
>> --- a/target/microblaze/op_helper.c
>> +++ b/target/microblaze/op_helper.c
>> @@ -69,9 +69,9 @@ void helper_raise_exception(CPUMBState *env, 
>> uint32_t index)
>>       cpu_loop_exit(cs);
>>   }
>> -static bool check_divz(CPUMBState *env, uint32_t b, uintptr_t ra)
>> +static bool check_divz(CPUMBState *env, uint32_t divisor, uintptr_t pc)
> 
> The name GETPC notwithstanding, I don't think ra -> pc is a good rename.
> 
> PC often gets confused about whether that's a host or guest value.
> RA (return address) is less so; it gets used often in core tcg.
> 
> If you really don't like RA, then perhaps "retaddr" (also with quite a 
> bit of usage) or "unwind_pc" (no existing usage, but very descriptive).

+1 for 'unwind_pc'.

While thinking more about it, I noticed we don't document the
prototypes in "exec/cpu-common.h". Doing so we could rename
s/pc/unwind_pc/ and document @unwind_pc. Also these helpers
would be better declared in a TCG specific header. Anyway,
note for Richard, orthogonal to this patch ;)

Re: [PATCH v1 2/4] target/microblaze: div: Rename and reorder function args

[Edgar E. Iglesias]

On Mon, Aug 25, 2025 at 12:11:25PM +0200, Philippe Mathieu-Daudé wrote:
> On 25/8/25 01:32, Richard Henderson wrote:
> > On 8/25/25 08:27, Edgar E. Iglesias wrote:
> > > From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
> > > 
> > > Rename and reorder function args to better match with spec
> > > and pseudo code.
> > > 
> > > No functional change.
> > > 
> > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
> > > ---
> > >   target/microblaze/op_helper.c | 18 +++++++++---------
> > >   target/microblaze/translate.c | 12 ++----------
> > >   2 files changed, 11 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/target/microblaze/op_helper.c b/target/microblaze/
> > > op_helper.c
> > > index 470526ee92..092977b3e1 100644
> > > --- a/target/microblaze/op_helper.c
> > > +++ b/target/microblaze/op_helper.c
> > > @@ -69,9 +69,9 @@ void helper_raise_exception(CPUMBState *env,
> > > uint32_t index)
> > >       cpu_loop_exit(cs);
> > >   }
> > > -static bool check_divz(CPUMBState *env, uint32_t b, uintptr_t ra)
> > > +static bool check_divz(CPUMBState *env, uint32_t divisor, uintptr_t pc)
> > 
> > The name GETPC notwithstanding, I don't think ra -> pc is a good rename.
> > 
> > PC often gets confused about whether that's a host or guest value.
> > RA (return address) is less so; it gets used often in core tcg.
> > 
> > If you really don't like RA, then perhaps "retaddr" (also with quite a
> > bit of usage) or "unwind_pc" (no existing usage, but very descriptive).
> 
> +1 for 'unwind_pc'.


Sounds good!

Thanks,
Edgar

[PATCH v1 3/4] target/microblaze: Break out raise_divzero()

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>

Break out raise_divzero(). No functional change.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
---
 target/microblaze/op_helper.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/target/microblaze/op_helper.c b/target/microblaze/op_helper.c
index 092977b3e1..d9444aee29 100644
--- a/target/microblaze/op_helper.c
+++ b/target/microblaze/op_helper.c
@@ -69,27 +69,24 @@ void helper_raise_exception(CPUMBState *env, uint32_t index)
     cpu_loop_exit(cs);
 }
 
-static bool check_divz(CPUMBState *env, uint32_t divisor, uintptr_t pc)
+/* Raises ESR_EC_DIVZERO if exceptions are enabled.  */
+static void raise_divzero(CPUMBState *env, uint32_t esr, uintptr_t pc)
 {
-    if (unlikely(divisor == 0)) {
-        env->msr |= MSR_DZ;
-
-        if ((env->msr & MSR_EE) &&
-            env_archcpu(env)->cfg.div_zero_exception) {
-            CPUState *cs = env_cpu(env);
-
-            env->esr = ESR_EC_DIVZERO;
-            cs->exception_index = EXCP_HW_EXCP;
-            cpu_loop_exit_restore(cs, pc);
-        }
-        return false;
+    env->msr |= MSR_DZ;
+
+    if ((env->msr & MSR_EE) && env_archcpu(env)->cfg.div_zero_exception) {
+        CPUState *cs = env_cpu(env);
+
+        env->esr = esr;
+        cs->exception_index = EXCP_HW_EXCP;
+        cpu_loop_exit_restore(cs, pc);
     }
-    return true;
 }
 
 uint32_t helper_divs(CPUMBState *env, uint32_t ra, uint32_t rb)
 {
-    if (!check_divz(env, ra, GETPC())) {
+    if (!ra) {
+        raise_divzero(env, ESR_EC_DIVZERO, GETPC());
         return 0;
     }
     return (int32_t)rb / (int32_t)ra;
@@ -97,7 +94,8 @@ uint32_t helper_divs(CPUMBState *env, uint32_t ra, uint32_t rb)
 
 uint32_t helper_divu(CPUMBState *env, uint32_t ra, uint32_t rb)
 {
-    if (!check_divz(env, ra, GETPC())) {
+    if (!ra) {
+        raise_divzero(env, ESR_EC_DIVZERO, GETPC());
         return 0;
     }
     return rb / ra;
-- 
2.43.0

Re: [PATCH v1 3/4] target/microblaze: Break out raise_divzero()

[Richard Henderson]

On 8/25/25 08:27, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
> 
> Break out raise_divzero(). No functional change.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
> ---
>   target/microblaze/op_helper.c | 30 ++++++++++++++----------------
>   1 file changed, 14 insertions(+), 16 deletions(-)

You could usefully squash this with the previous.  It obviates the b -> divisor rename, 
and it perhaps clarifies why you don't like using "ra" in two different contexts.

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


r~

Re: [PATCH v1 3/4] target/microblaze: Break out raise_divzero()

[Edgar E. Iglesias]

On Mon, Aug 25, 2025 at 09:36:44AM +1000, Richard Henderson wrote:
> On 8/25/25 08:27, Edgar E. Iglesias wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
> > 
> > Break out raise_divzero(). No functional change.
> > 
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
> > ---
> >   target/microblaze/op_helper.c | 30 ++++++++++++++----------------
> >   1 file changed, 14 insertions(+), 16 deletions(-)
> 
> You could usefully squash this with the previous.  It obviates the b ->
> divisor rename, and it perhaps clarifies why you don't like using "ra" in
> two different contexts.

Yes, that was what prompted me to rename ra in the first place.

I'll squash this with the former patch and rename pc to unwind_pc,
keeping your RB tag. Let me know if that wasn't your intention.

Cheers,
Edgar


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

[PATCH v1 4/4] target/microblaze: Handle signed division overflows

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>

Handle signed division overflows as specified in UG984:
https://docs.amd.com/r/en-US/ug984-vivado-microblaze-ref/idiv

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
---
 target/microblaze/cpu.h       |  1 +
 target/microblaze/op_helper.c | 15 +++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/target/microblaze/cpu.h b/target/microblaze/cpu.h
index 3ce28b302f..7dd86653f0 100644
--- a/target/microblaze/cpu.h
+++ b/target/microblaze/cpu.h
@@ -87,6 +87,7 @@ typedef struct CPUArchState CPUMBState;
 #define          ESR_ESS_FSL_OFFSET     5
 
 #define          ESR_ESS_MASK  (0x7f << 5)
+#define          ESR_ESS_DEC_OF  (1 << 20) /* DEC: 0=DBZ, 1=OF */
 
 #define          ESR_EC_FSL             0
 #define          ESR_EC_UNALIGNED_DATA  1
diff --git a/target/microblaze/op_helper.c b/target/microblaze/op_helper.c
index d9444aee29..e5f13824d8 100644
--- a/target/microblaze/op_helper.c
+++ b/target/microblaze/op_helper.c
@@ -89,6 +89,21 @@ uint32_t helper_divs(CPUMBState *env, uint32_t ra, uint32_t rb)
         raise_divzero(env, ESR_EC_DIVZERO, GETPC());
         return 0;
     }
+
+    /*
+     * Check for division overflows.
+     *
+     * Spec: https://docs.amd.com/r/en-US/ug984-vivado-microblaze-ref/idiv
+     * UG984, Chapter 5 MicroBlaze Instruction Set Architecture, idiv.
+     *
+     * If the U bit is clear, the value of rA is -1, and the value of rB is
+     * -2147483648 (divide overflow), the DZO bit in MSR will be set and
+     * the value in rD will be -2147483648, unless an exception is generated.
+     */
+    if ((int32_t)ra == -1 && (int32_t)rb == INT32_MIN) {
+        raise_divzero(env, ESR_EC_DIVZERO | ESR_ESS_DEC_OF, GETPC());
+        return INT32_MIN;
+    }
     return (int32_t)rb / (int32_t)ra;
 }
 
-- 
2.43.0

Re: [PATCH v1 4/4] target/microblaze: Handle signed division overflows

[Richard Henderson]

On 8/25/25 08:27, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
> 
> Handle signed division overflows as specified in UG984:
> https://docs.amd.com/r/en-US/ug984-vivado-microblaze-ref/idiv
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
> ---
>   target/microblaze/cpu.h       |  1 +
>   target/microblaze/op_helper.c | 15 +++++++++++++++
>   2 files changed, 16 insertions(+)
> 
> diff --git a/target/microblaze/cpu.h b/target/microblaze/cpu.h
> index 3ce28b302f..7dd86653f0 100644
> --- a/target/microblaze/cpu.h
> +++ b/target/microblaze/cpu.h
> @@ -87,6 +87,7 @@ typedef struct CPUArchState CPUMBState;
>   #define          ESR_ESS_FSL_OFFSET     5
>   
>   #define          ESR_ESS_MASK  (0x7f << 5)
> +#define          ESR_ESS_DEC_OF  (1 << 20) /* DEC: 0=DBZ, 1=OF */

That's bit 20 big-endian, so bit (1 << 11).

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


r~

Re: [PATCH v1 4/4] target/microblaze: Handle signed division overflows

[Edgar E. Iglesias]

On Mon, Aug 25, 2025 at 09:44:34AM +1000, Richard Henderson wrote:
> On 8/25/25 08:27, Edgar E. Iglesias wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
> > 
> > Handle signed division overflows as specified in UG984:
> > https://docs.amd.com/r/en-US/ug984-vivado-microblaze-ref/idiv
> > 
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
> > ---
> >   target/microblaze/cpu.h       |  1 +
> >   target/microblaze/op_helper.c | 15 +++++++++++++++
> >   2 files changed, 16 insertions(+)
> > 
> > diff --git a/target/microblaze/cpu.h b/target/microblaze/cpu.h
> > index 3ce28b302f..7dd86653f0 100644
> > --- a/target/microblaze/cpu.h
> > +++ b/target/microblaze/cpu.h
> > @@ -87,6 +87,7 @@ typedef struct CPUArchState CPUMBState;
> >   #define          ESR_ESS_FSL_OFFSET     5
> >   #define          ESR_ESS_MASK  (0x7f << 5)
> > +#define          ESR_ESS_DEC_OF  (1 << 20) /* DEC: 0=DBZ, 1=OF */
> 
> That's bit 20 big-endian, so bit (1 << 11).

Fixed for v2, thanks!

> 
> Otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> 
> r~