Re: [PULL 19/31] target/s390x: Return exception from mmu_translate_real

[Peter Maydell <peter.maydell@linaro.org>]

On Thu, 10 Oct 2019 at 12:35, David Hildenbrand <david@redhat.com> wrote:
>
> From: Richard Henderson <richard.henderson@linaro.org>
>
> Do not raise the exception directly within mmu_translate_real,
> but pass it back so that caller may do so.
>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> Message-Id: <20191001171614.8405-8-richard.henderson@linaro.org>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Hi; Coverity complains about dead code in this patch:



> --- a/target/s390x/mmu_helper.c
> +++ b/target/s390x/mmu_helper.c
> @@ -554,14 +554,11 @@ void s390_cpu_virt_mem_handle_exc(S390CPU *cpu, uintptr_t ra)
>   * @param rw     0 = read, 1 = write, 2 = code fetch
>   * @param addr   the translated address is stored to this pointer
>   * @param flags  the PAGE_READ/WRITE/EXEC flags are stored to this pointer
> - * @return       0 if the translation was successful, < 0 if a fault occurred
> + * @return       0 = success, != 0, the exception to raise
>   */
>  int mmu_translate_real(CPUS390XState *env, target_ulong raddr, int rw,
> -                       target_ulong *addr, int *flags)
> +                       target_ulong *addr, int *flags, uint64_t *tec)
>  {
> -    /* Code accesses have an undefined ilc, let's use 2 bytes. */
> -    uint64_t tec = (raddr & TARGET_PAGE_MASK) |
> -                   (rw == MMU_DATA_STORE ? FS_WRITE : FS_READ);
>      const bool lowprot_enabled = env->cregs[0] & CR0_LOWPROT;
>
>      *flags = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> @@ -570,9 +567,10 @@ int mmu_translate_real(CPUS390XState *env, target_ulong raddr, int rw,
>          *flags |= PAGE_WRITE_INV;
>          if (is_low_address(raddr) && rw == MMU_DATA_STORE) {
>              /* LAP sets bit 56 */
> -            tec |= 0x80;
> -            trigger_access_exception(env, PGM_PROTECTION, ILEN_AUTO, tec);
> -            return -EACCES;
> +            *tec = (raddr & TARGET_PAGE_MASK)
> +                 | (rw == MMU_DATA_STORE ? FS_WRITE : FS_READ)

We're inside a condition which includes 'rw == MMU_DATA_STORE',
so checking it again here is unnecessary, and the 'false'
part of this ?: conditional is dead-code.

> +                 | 0x80;
> +            return PGM_PROTECTION;
>          }
>      }

thanks
-- PMM