[PATCH 0/4] target/arm bug fix

[PATCH 0/4] target/arm bug fix

[LIU Zhiwei]

I found some bugs in target/arm.

The first one is about SVE first-fault or no-fault load/store.
The second is SIMD fcmla(by element).
The third is about CPTR_EL2.

I am not sure I really understand this code. Please confirm the patch set and let me know if I am wrong.

LIU Zhiwei (4):
  target/arm: Fixup special cross page case for sve continuous
    load/store
  target/arm: Fixup contiguous first-fault and no-fault loads
  target/arm: Fixup SIMD fcmla(by element) in 4H arrangement
  target/arm: adjust CPTR_EL2 according to HCR_EL2.E2H

 target/arm/helper.c     | 55 ++++++++++++++++++++++++++++++++++-------
 target/arm/sve_helper.c | 42 ++++++++++++++++++++-----------
 target/arm/vec_helper.c |  8 ++++++
 3 files changed, 82 insertions(+), 23 deletions(-)

-- 
2.23.0

[PATCH 1/4] target/arm: Fixup special cross page case for sve continuous load/store

[LIU Zhiwei]

If the split element is also the first active element of the vector,
mem_off_first[0] should equal to mem_off_split.

Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
---
 target/arm/sve_helper.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c
index 5f037c3a8f..91d1d24725 100644
--- a/target/arm/sve_helper.c
+++ b/target/arm/sve_helper.c
@@ -4282,9 +4282,10 @@ static bool sve_cont_ldst_pages(SVEContLdSt *info, SVEContFault fault,
          * to generate faults for the second page.  For no-fault,
          * we have work only if the second page is valid.
          */
-        if (info->mem_off_first[0] < info->mem_off_split) {
-            nofault = FAULT_FIRST;
-            have_work = false;
+        if (info->mem_off_first[0] == info->mem_off_split) {
+            if (nofault) {
+                have_work = false;
+            }
         }
     } else {
         /*
-- 
2.23.0

Re: [PATCH 1/4] target/arm: Fixup special cross page case for sve continuous load/store

[Richard Henderson]

On 12/6/20 10:46 PM, LIU Zhiwei wrote:
> If the split element is also the first active element of the vector,
> mem_off_first[0] should equal to mem_off_split.
> 
> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
> ---
>  target/arm/sve_helper.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c
> index 5f037c3a8f..91d1d24725 100644
> --- a/target/arm/sve_helper.c
> +++ b/target/arm/sve_helper.c
> @@ -4282,9 +4282,10 @@ static bool sve_cont_ldst_pages(SVEContLdSt *info, SVEContFault fault,
>           * to generate faults for the second page.  For no-fault,
>           * we have work only if the second page is valid.
>           */
> -        if (info->mem_off_first[0] < info->mem_off_split) {
> -            nofault = FAULT_FIRST;
> -            have_work = false;
> +        if (info->mem_off_first[0] == info->mem_off_split) {
> +            if (nofault) {
> +                have_work = false;
> +            }

There are two separate bugs here.  The first is as you describe, and applies
only to the first line.

The second is the assignment of an enumerator to a boolean, and the incorrect
unconditional clearing of have_work.  The change could more consisely be

-  nofault = FAULT_FIRST;
-  have_work = false;
+  /* For nofault, we only have work if the second page is valid. */
+  have_work = !nofault;

We can either split the two changes, or improve the patch comment.


r~

[PATCH 2/4] target/arm: Fixup contiguous first-fault and no-fault loads

[LIU Zhiwei]

First-fault or no-fault doesn't mean only access one page.

When cross pages, for first-fault, if there is no fault in the first access,
the second page should be accessed. And for no-fault, the second page
should always be accessed.

Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
---
 target/arm/sve_helper.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c
index 91d1d24725..700a8a7585 100644
--- a/target/arm/sve_helper.c
+++ b/target/arm/sve_helper.c
@@ -4916,17 +4916,6 @@ void sve_ldnfff1_r(CPUARMState *env, void *vg, const target_ulong addr,
         } while (reg_off <= reg_last && (reg_off & 63));
     } while (reg_off <= reg_last);
 
-    /*
-     * MemSingleNF is allowed to fail for any reason.  We have special
-     * code above to handle the first element crossing a page boundary.
-     * As an implementation choice, decline to handle a cross-page element
-     * in any other position.
-     */
-    reg_off = info.reg_off_split;
-    if (reg_off >= 0) {
-        goto do_fault;
-    }
-
  second_page:
     reg_off = info.reg_off_first[1];
     if (likely(reg_off < 0)) {
@@ -4934,6 +4923,30 @@ void sve_ldnfff1_r(CPUARMState *env, void *vg, const target_ulong addr,
         return;
     }
 
+    mem_off = info.mem_off_first[1];
+    reg_last = info.reg_off_last[1];
+    host = info.page[1].host;
+
+    do {
+        uint64_t pg = *(uint64_t *)(vg + (reg_off >> 3));
+        do {
+            if ((pg >> (reg_off & 63)) & 1) {
+                if (unlikely(flags & TLB_WATCHPOINT) &&
+                    (cpu_watchpoint_address_matches
+                     (env_cpu(env), addr + mem_off, 1 << msz)
+                     & BP_MEM_READ)) {
+                    goto do_fault;
+                }
+                if (mtedesc && !mte_probe1(env, mtedesc, addr + mem_off)) {
+                    goto do_fault;
+                }
+                host_fn(vd, reg_off, host + mem_off);
+            }
+            reg_off += 1 << esz;
+            mem_off += 1 << msz;
+        } while (reg_off <= reg_last && (reg_off & 63));
+    } while (reg_off <= reg_last);
+
     /*
      * MemSingleNF is allowed to fail for any reason.  As an implementation
      * choice, decline to handle elements on the second page.  This should
-- 
2.23.0

Re: [PATCH 2/4] target/arm: Fixup contiguous first-fault and no-fault loads

[Richard Henderson]

On 12/6/20 10:46 PM, LIU Zhiwei wrote:
> First-fault or no-fault doesn't mean only access one page.

But the implementation is *allowed* to access only one page.
Thus the comment:

> -    /*
> -     * MemSingleNF is allowed to fail for any reason.  We have special
> -     * code above to handle the first element crossing a page boundary.
> -     * As an implementation choice, decline to handle a cross-page element
> -     * in any other position.
> -     */


r~

Re: [PATCH 2/4] target/arm: Fixup contiguous first-fault and no-fault loads

[LIU Zhiwei]

[-- Attachment #1: Type: text/plain, Size: 2450 bytes --]



On 2020/12/9 4:16, Richard Henderson wrote:
> On 12/6/20 10:46 PM, LIU Zhiwei wrote:
>> First-fault or no-fault doesn't mean only access one page.
> But the implementation is *allowed* to access only one page.
> Thus the comment:
>
>> -    /*
>> -     * MemSingleNF is allowed to fail for any reason.  We have special
>> -     * code above to handle the first element crossing a page boundary.
>> -     * As an implementation choice, decline to handle a cross-page element
>> -     * in any other position.
>> -     */
 From the pseudo code,  I see  that we should handle the first element 
in first-fault follow Mem. And the other elements in this function 
should follow  MemNF.

I have some questions here：

1. Why  do special process to the first element if it crosses pages in 
no-fault? Because it's not aligned?

// MemNF[] - non-assignment form

// =============================

(bits(8*size), boolean) MemNF[bits(64) address, integer size, AccType 
acctype]

    assert size IN {1, 2, 4, 8, 16};

    bits(8*size) value;

    aligned = (address == Align(address, size));

    A = SCTLR[].A;

    if !aligned && (A == '1') then

         return (bits(8*size) UNKNOWN, TRUE);

    atomic = aligned || size == 1;

    if !atomic then

         (value<7:0>, bad) = MemSingleNF[address, 1, acctype, aligned];

         if bad then

             return (bits(8*size) UNKNOWN, TRUE);

         // For subsequent bytes it is CONSTRAINED UNPREDICTABLE whether
    an unaligned Device memory

         // access will generate an Alignment Fault, as to get this far
    means the first byte did

         // not, so we must be changing to a new translation page.

         if !aligned then

         c = ConstrainUnpredictable(Unpredictable_DEVPAGE2);

         assert c IN {Constraint_FAULT, Constraint_NONE};

         if c == Constraint_NONE then aligned = TRUE;

         for i = 1 to size-1

             (value<8*i+7:8*i>, bad) = MemSingleNF[address+i, 1,
    acctype, aligned];

         if bad then

             return (bits(8*size) UNKNOWN, TRUE);

    else

         (value, bad) = MemSingleNF[address, size, acctype, aligned];

         if bad then

             return (bits(8*size) UNKNOWN, TRUE);

    return (value, FALSE);



2.  Why it doesn't access the second page like the first page? I think 
they should obey the same MemSingleNF implementation.

> r~


[-- Attachment #2: Type: text/html, Size: 4581 bytes --]

[PATCH 3/4] target/arm: Fixup SIMD fcmla(by element) in 4H arrangement

[LIU Zhiwei]

For SIMD fcmla(by element), if the number of elements is less than
the number of elements within one segment，i.e. 4H arrangement,
we should not calculate the entire segment.

Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
---
 target/arm/vec_helper.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/target/arm/vec_helper.c b/target/arm/vec_helper.c
index 7174030377..44b8165323 100644
--- a/target/arm/vec_helper.c
+++ b/target/arm/vec_helper.c
@@ -544,6 +544,10 @@ void HELPER(gvec_fcmlah_idx)(void *vd, void *vn, void *vm,
     neg_real <<= 15;
     neg_imag <<= 15;
 
+    /* Adjust eltspersegment for simd 4H */
+    if (eltspersegment > elements) {
+        eltspersegment = elements;
+    }
     for (i = 0; i < elements; i += eltspersegment) {
         float16 mr = m[H2(i + 2 * index + 0)];
         float16 mi = m[H2(i + 2 * index + 1)];
@@ -610,6 +614,10 @@ void HELPER(gvec_fcmlas_idx)(void *vd, void *vn, void *vm,
     neg_real <<= 31;
     neg_imag <<= 31;
 
+    /* Adjust eltspersegment for simd 4H */
+    if (eltspersegment > elements) {
+        eltspersegment = elements;
+    }
     for (i = 0; i < elements; i += eltspersegment) {
         float32 mr = m[H4(i + 2 * index + 0)];
         float32 mi = m[H4(i + 2 * index + 1)];
-- 
2.23.0

Re: [PATCH 3/4] target/arm: Fixup SIMD fcmla(by element) in 4H arrangement

[Richard Henderson]

On 12/6/20 10:46 PM, LIU Zhiwei wrote:
> For SIMD fcmla(by element), if the number of elements is less than
> the number of elements within one segment，i.e. 4H arrangement,
> we should not calculate the entire segment.
> 
> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
> ---
>  target/arm/vec_helper.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/target/arm/vec_helper.c b/target/arm/vec_helper.c
> index 7174030377..44b8165323 100644
> --- a/target/arm/vec_helper.c
> +++ b/target/arm/vec_helper.c
> @@ -544,6 +544,10 @@ void HELPER(gvec_fcmlah_idx)(void *vd, void *vn, void *vm,
>      neg_real <<= 15;
>      neg_imag <<= 15;
>  
> +    /* Adjust eltspersegment for simd 4H */
> +    if (eltspersegment > elements) {
> +        eltspersegment = elements;
> +    }

Ok.  Maybe better to fold this back to the initialization using MIN.

>      for (i = 0; i < elements; i += eltspersegment) {
>          float16 mr = m[H2(i + 2 * index + 0)];
>          float16 mi = m[H2(i + 2 * index + 1)];
> @@ -610,6 +614,10 @@ void HELPER(gvec_fcmlas_idx)(void *vd, void *vn, void *vm,
>      neg_real <<= 31;
>      neg_imag <<= 31;
>  
> +    /* Adjust eltspersegment for simd 4H */
> +    if (eltspersegment > elements) {
> +        eltspersegment = elements;
> +    }

Incorrect: this function only computes 4S.

>      for (i = 0; i < elements; i += eltspersegment) {
>          float32 mr = m[H4(i + 2 * index + 0)];
>          float32 mi = m[H4(i + 2 * index + 1)];
> 


r~

[PATCH 4/4] target/arm: adjust CPTR_EL2 according to HCR_EL2.E2H

[LIU Zhiwei]

From DDI0487Fc_armv8_arm.pdf, the CPTR_EL2 has two kinds
of layouts according to HCR_EL2.E2H.

When HCR_EL2.E2H is 1, fp_exception_el should refer to
HCR_EL2.FPEN and sve_exception_el should refer to HCR_EL2.ZEN.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
---
 target/arm/helper.c | 55 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 46 insertions(+), 9 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 38cd35c049..6cc9f2bb50 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6147,11 +6147,30 @@ int sve_exception_el(CPUARMState *env, int el)
      * they will be zero when EL2 is not present.
      */
     if (el <= 2 && !arm_is_secure_below_el3(env)) {
-        if (env->cp15.cptr_el[2] & CPTR_TZ) {
-            return 2;
-        }
-        if (env->cp15.cptr_el[2] & CPTR_TFP) {
-            return 0;
+        /* Since we exclude secure first, we may read HCR_EL2 directly. */
+        if (env->cp15.hcr_el2 & HCR_E2H) {
+            int zen = extract32(env->cp15.cptr_el[2], 16, 2);
+            switch (zen) {
+            case 0:
+            case 2:
+                return 2;
+            case 1:
+                if (env->cp15.hcr_el2 & HCR_TGE) {
+                    if (el == 0) {
+                        return 2;
+                    }
+                }
+                break;
+            case 3:
+                break;
+            }
+        } else {
+            if (env->cp15.cptr_el[2] & CPTR_TZ) {
+                return 2;
+            }
+            if (env->cp15.cptr_el[2] & CPTR_TFP) {
+                return 0;
+            }
         }
     }
 
@@ -12635,12 +12654,30 @@ int fp_exception_el(CPUARMState *env, int cur_el)
      */
 
     /* CPTR_EL2 : present in v7VE or v8 */
-    if (cur_el <= 2 && extract32(env->cp15.cptr_el[2], 10, 1)
-        && !arm_is_secure_below_el3(env)) {
+    if ((cur_el <= 2) && !arm_is_secure_below_el3(env)) {
         /* Trap FP ops at EL2, NS-EL1 or NS-EL0 to EL2 */
-        return 2;
+        if ((arm_hcr_el2_eff(env) & HCR_E2H) == HCR_E2H) {
+            int fpen = extract32(env->cp15.cptr_el[2], 20, 2);
+            switch (fpen) {
+            case 0:
+            case 2:
+                return 2;
+            case 1:
+                if ((arm_hcr_el2_eff(env) & HCR_TGE) == HCR_TGE) {
+                    if (cur_el == 0) {
+                        return 2;
+                    }
+                }
+                break;
+            case 3:
+                break;
+            }
+        } else {
+            if (extract32(env->cp15.cptr_el[2], 10, 1)) {
+                return 2;
+            }
+        }
     }
-
     /* CPTR_EL3 : present in v8 */
     if (extract32(env->cp15.cptr_el[3], 10, 1)) {
         /* Trap all FP ops to EL3 */
-- 
2.23.0

Re: [PATCH 4/4] target/arm: adjust CPTR_EL2 according to HCR_EL2.E2H

[Richard Henderson]

On 12/6/20 10:46 PM, LIU Zhiwei wrote:
> From DDI0487Fc_armv8_arm.pdf, the CPTR_EL2 has two kinds
> of layouts according to HCR_EL2.E2H.
> 
> When HCR_EL2.E2H is 1, fp_exception_el should refer to
> HCR_EL2.FPEN and sve_exception_el should refer to HCR_EL2.ZEN.
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
> ---
>  target/arm/helper.c | 55 +++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 46 insertions(+), 9 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 38cd35c049..6cc9f2bb50 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6147,11 +6147,30 @@ int sve_exception_el(CPUARMState *env, int el)
>       * they will be zero when EL2 is not present.
>       */
>      if (el <= 2 && !arm_is_secure_below_el3(env)) {
> -        if (env->cp15.cptr_el[2] & CPTR_TZ) {
> -            return 2;
> -        }
> -        if (env->cp15.cptr_el[2] & CPTR_TFP) {
> -            return 0;
> +        /* Since we exclude secure first, we may read HCR_EL2 directly. */
> +        if (env->cp15.hcr_el2 & HCR_E2H) {

We have already pulled the correct value into a local variable here: hcr_el2.
No need for the comment.

> +            int zen = extract32(env->cp15.cptr_el[2], 16, 2);
> +            switch (zen) {
> +            case 0:
> +            case 2:
> +                return 2;
> +            case 1:
> +                if (env->cp15.hcr_el2 & HCR_TGE) {

Likewise.

> +                    if (el == 0) {
> +                        return 2;
> +                    }
> +                }
> +                break;
> +            case 3:
> +                break;
> +            }
> +        } else {
> +            if (env->cp15.cptr_el[2] & CPTR_TZ) {
> +                return 2;
> +            }
> +            if (env->cp15.cptr_el[2] & CPTR_TFP) {
> +                return 0;
> +            }
>          }
>      }
>  
> @@ -12635,12 +12654,30 @@ int fp_exception_el(CPUARMState *env, int cur_el)
>       */
>  
>      /* CPTR_EL2 : present in v7VE or v8 */
> -    if (cur_el <= 2 && extract32(env->cp15.cptr_el[2], 10, 1)
> -        && !arm_is_secure_below_el3(env)) {
> +    if ((cur_el <= 2) && !arm_is_secure_below_el3(env)) {
>          /* Trap FP ops at EL2, NS-EL1 or NS-EL0 to EL2 */
> -        return 2;
> +        if ((arm_hcr_el2_eff(env) & HCR_E2H) == HCR_E2H) {

There is a prior use of arm_hcr_el2_eff in this function.  It should be called
once and stored in a local variable, just like in sve_exception_el.

No need for == in testing a single bit.

> +            int fpen = extract32(env->cp15.cptr_el[2], 20, 2);
> +            switch (fpen) {
> +            case 0:
> +            case 2:
> +                return 2;
> +            case 1:
> +                if ((arm_hcr_el2_eff(env) & HCR_TGE) == HCR_TGE) {

Likewise.

> +                    if (cur_el == 0) {
> +                        return 2;
> +                    }
> +                }
> +                break;
> +            case 3:
> +                break;
> +            }
> +        } else {
> +            if (extract32(env->cp15.cptr_el[2], 10, 1)) {

Use CPTR_TFP.

> +                return 2;
> +            }
> +        }
>      }
> -

Whitespace removal.

>      /* CPTR_EL3 : present in v8 */
>      if (extract32(env->cp15.cptr_el[3], 10, 1)) {
>          /* Trap all FP ops to EL3 */
> 

Would you also change this to use CPTR_TFP please?


r~