Re: [Qemu-devel] [RFC 2/3] target/m68k: In get_physical_address() check for memory access failures

[Laurent Vivier]

On 10/12/2018 17:56, Peter Maydell wrote:
> In get_physical_address(), use address_space_ldl() and
> address_space_stl() instead of ldl_phys() and stl_phys().
> This allows us to check whether the memory access failed.
> For the moment, we simply return -1 in this case;
> add a TODO comment that we should ideally generate the
> appropriate kind of fault.
>
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>   target/m68k/helper.c | 62 +++++++++++++++++++++++++++++++++++++-------
>   1 file changed, 52 insertions(+), 10 deletions(-)
>
> diff --git a/target/m68k/helper.c b/target/m68k/helper.c
> index 374e4861886..b5fa2f8056d 100644
> --- a/target/m68k/helper.c
> +++ b/target/m68k/helper.c
> @@ -660,6 +660,7 @@ static int get_physical_address(CPUM68KState *env, hwaddr 
> *physical,
>       bool debug = access_type & ACCESS_DEBUG;
>       int page_bits;
>       int i;
> +    MemTxResult txres;
>   
>       /* Transparent Translation (physical = logical) */
>       for (i = 0; i < M68K_MAX_TTR; i++) {
> @@ -689,12 +690,19 @@ static int get_physical_address(CPUM68KState *env, hwaddr 
> *physical,
>       /* Root Index */
>       entry = M68K_POINTER_BASE(next) | M68K_ROOT_INDEX(address);
>   
> -    next = ldl_phys(cs->as, entry);
> +    next = address_space_ldl(cs->as, entry, MEMTXATTRS_UNSPECIFIED, &txres);
> +    if (txres != MEMTX_OK) {
> +        goto txfail;
> +    }
>       if (!M68K_UDT_VALID(next)) {
>           return -1;
>       }
>       if (!(next & M68K_DESC_USED) && !debug) {
> -        stl_phys(cs->as, entry, next | M68K_DESC_USED);
> +        address_space_stl(cs->as, entry, next | M68K_DESC_USED,
> +                          MEMTXATTRS_UNSPECIFIED, &txres);
> +        if (txres != MEMTX_OK) {
> +            goto txfail;
> +        }
>       }
>       if (next & M68K_DESC_WRITEPROT) {
>           if (access_type & ACCESS_PTEST) {
> @@ -709,12 +717,19 @@ static int get_physical_address(CPUM68KState *env, hwaddr 
> *physical,
>       /* Pointer Index */
>       entry = M68K_POINTER_BASE(next) | M68K_POINTER_INDEX(address);
>   
> -    next = ldl_phys(cs->as, entry);
> +    next = address_space_ldl(cs->as, entry, MEMTXATTRS_UNSPECIFIED, &txres);
> +    if (txres != MEMTX_OK) {
> +        goto txfail;
> +    }
>       if (!M68K_UDT_VALID(next)) {
>           return -1;
>       }
>       if (!(next & M68K_DESC_USED) && !debug) {
> -        stl_phys(cs->as, entry, next | M68K_DESC_USED);
> +        address_space_stl(cs->as, entry, next | M68K_DESC_USED,
> +                          MEMTXATTRS_UNSPECIFIED, &txres);
> +        if (txres != MEMTX_OK) {
> +            goto txfail;
> +        }
>       }
>       if (next & M68K_DESC_WRITEPROT) {
>           if (access_type & ACCESS_PTEST) {
> @@ -733,27 +748,46 @@ static int get_physical_address(CPUM68KState *env, hwaddr 
> *physical,
>           entry = M68K_4K_PAGE_BASE(next) | M68K_4K_PAGE_INDEX(address);
>       }
>   
> -    next = ldl_phys(cs->as, entry);
> +    next = address_space_ldl(cs->as, entry, MEMTXATTRS_UNSPECIFIED, &txres);
> +    if (txres != MEMTX_OK) {
> +        goto txfail;
> +    }
>   
>       if (!M68K_PDT_VALID(next)) {
>           return -1;
>       }
>       if (M68K_PDT_INDIRECT(next)) {
> -        next = ldl_phys(cs->as, M68K_INDIRECT_POINTER(next));
> +        next = address_space_ldl(cs->as, M68K_INDIRECT_POINTER(next),
> +                                 MEMTXATTRS_UNSPECIFIED, &txres);
> +        if (txres != MEMTX_OK) {
> +            goto txfail;
> +        }
>       }
>       if (access_type & ACCESS_STORE) {
>           if (next & M68K_DESC_WRITEPROT) {
>               if (!(next & M68K_DESC_USED) && !debug) {
> -                stl_phys(cs->as, entry, next | M68K_DESC_USED);
> +                address_space_stl(cs->as, entry, next | M68K_DESC_USED,
> +                                  MEMTXATTRS_UNSPECIFIED, &txres);
> +                if (txres != MEMTX_OK) {
> +                    goto txfail;
> +                }
>               }
>           } else if ((next & (M68K_DESC_MODIFIED | M68K_DESC_USED)) !=
>                              (M68K_DESC_MODIFIED | M68K_DESC_USED) && !debug) {
> -                stl_phys(cs->as, entry,
> -                         next | (M68K_DESC_MODIFIED | M68K_DESC_USED));
> +            address_space_stl(cs->as, entry,
> +                              next | (M68K_DESC_MODIFIED | M68K_DESC_USED),
> +                              MEMTXATTRS_UNSPECIFIED, &txres);
> +            if (txres != MEMTX_OK) {
> +                goto txfail;
> +            }
>           }
>       } else {
>           if (!(next & M68K_DESC_USED) && !debug) {
> -            stl_phys(cs->as, entry, next | M68K_DESC_USED);
> +            address_space_stl(cs->as, entry, next | M68K_DESC_USED,
> +                              MEMTXATTRS_UNSPECIFIED, &txres);
> +            if (txres != MEMTX_OK) {
> +                goto txfail;
> +            }
>           }
>       }
>   
> @@ -785,6 +819,14 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical,
>       }
>   
>       return 0;
> +
> +txfail:
> +    /*
> +     * A page table load/store failed. TODO: we should really raise a
> +     * suitable guest fault here if this is not a debug access.
> +     * For now just return that the translation failed.
> +     */
> +    return -1;
>   }
>   
>   hwaddr m68k_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)

Reviewed-by: Laurent Vivier <address@hidden>