Re: [Qemu-devel] [PATCH v2 1/2] target/xtensa: convert to do_transaction_failed

[Peter Maydell]

On 29 August 2018 at 02:16, Max Filippov <address@hidden> wrote:
> Signed-off-by: Max Filippov <address@hidden>
> ---
> Changes v1->v2:
> - change ldl_phys to address_space_ldl in get_pte and check transaction
>   for success;

> --- a/target/xtensa/helper.c
> +++ b/target/xtensa/helper.c
> @@ -642,11 +642,27 @@ static int get_pte(CPUXtensaState *env, uint32_t vaddr, 
> uint32_t *pte)
>      int ret = get_physical_addr_mmu(env, false, pt_vaddr, 0, 0,
>              &paddr, &page_size, &access, false);
>
> -    qemu_log_mask(CPU_LOG_MMU, "%s: trying autorefill(%08x) -> %08x\n",
> -                  __func__, vaddr, ret ? ~0 : paddr);
> +    if (ret == 0) {
> +        qemu_log_mask(CPU_LOG_MMU,
> +                      "%s: autorefill(%08x): PTE va = %08x, pa = %08x\n",
> +                      __func__, vaddr, pt_vaddr, paddr);
> +    } else {
> +        qemu_log_mask(CPU_LOG_MMU,
> +                      "%s: autorefill(%08x): PTE va = %08x, failed (%d)\n",
> +                      __func__, vaddr, pt_vaddr, ret);
> +    }
>
>      if (ret == 0) {
> -        *pte = ldl_phys(cs->as, paddr);
> +        MemTxResult result;
> +
> +        *pte = address_space_ldl(cs->as, paddr, MEMTXATTRS_UNSPECIFIED,
> +                                 &result);
> +        if (result != MEMTX_OK) {
> +            qemu_log_mask(CPU_LOG_MMU,
> +                          "%s: couldn't load PTE: transaction failed (%u)\n",
> +                          __func__, (unsigned)result);
> +            ret = 1;
> +        }

I don't know xtensa, but it seems a bit odd that this function now returns:
 * 0 on success
 * an exception cause code as returned from get_physical_addr_mmu() if
   that fails
 * 1, if the physical load fails

The callsite seems to only care about 0 or not-0, but it feels like
it might be cleaner to have it either return an exception code cause
in all cases, or just return a bool ?

>      }
>      return ret;
>  }

But that's a small nit, and the patch looks good overall, so
Reviewed-by: Peter Maydell <address@hidden>

thanks
-- PMM