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

[Richard Henderson <richard.henderson@linaro.org>]

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~