Re: [PATCH 14/17] hw/core: Implement new cpu_translate_for_debug()

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 17/4/26 19:31, Peter Maydell wrote:
> In cpu_memory_rw_debug() we need to do a virtual-to-physical address
> translation for debug access.  Currently we assume that the
> translation is valid for an entire guest page, but this may not be
> true if the target implements some protection regions that have
> sub-page granularity. (Currently the only such target is the Arm
> CPUs when using an MPU, as in R-profile and M-profile.)
> 
> For TCG's emulated accesses, we handle sub-page granularity by the
> CPU filling in the lg_page_size field of the CPUTLBEntryFull struct
> to tell us how large the region covered by the result is.  But we
> didn't extend this to the debug-access code path, with the result
> that debug accesses might incorrectly fail because they are looking
> at the mapping for the address rounded down to a page boundary.
> 
> Provide a cpu_translate_for_debug() function which reports to the
> caller not just the physical address and attributes of the
> translation but also the lg_page_size for which it is valid.  The
> fallback implementation calls cpu_get_phys_addr_attrs_debug() and
> assumes target-page-sized validity.
> 
> NB: the "return true on valid access, false on failure" follows
> the same convention as TCGCPUOps::tlb_fill_align() (though it
> is the opposite of what we use in some other places, e.g.
> in target/arm's get_phys_addr_* functions).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/core/cpu-system.c             | 32 ++++++++++++++++++++++++++++++++
>   include/hw/core/cpu.h            | 32 ++++++++++++++++++++++++++++++++
>   include/hw/core/sysemu-cpu-ops.h | 27 +++++++++++++++++++++++++--
>   3 files changed, 89 insertions(+), 2 deletions(-)


> +    /**
> +     * @translate_for_debug: Callback for translating a virtual address into
> +     * a physical address for debug purposes.
> +     * The implementation should fill in @result with the physical address,
> +     * transaction attributes, and log2 of the size of the aligned block of
> +     * memory that the translation is valid for.
> +     * This must be able to handle a non-page-aligned address, and will
> +     * return the physical address corresponding to that address.
> +     * The attributes must include the debug flag being set.
> +     * Returns false on translation failure; on success returns true and
> +     * fills in @result.
> +     *
> +     * This is the preferred method to implement for new CPUs.
> +     */
> +    bool (*translate_for_debug)(CPUState *cpu, vaddr addr,
> +                                TranslateForDebugResult *result);

Since this call shouldn't modify the internal CPU state, could
we use 'const CPUState *cpu'? Maybe not possible due to callees
still taking non-const. Regardless:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>