Re: [PATCH] target/arm: Fix ptw parameters in S1_ptw_translate() for debug contexts

[Peter Maydell]

On Thu, 6 Jul 2023 at 15:12, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> Arm TF-A fails to boot via semihosting following a recent change to the
> MMU code. Semihosting attempts to read parameters passed by TF-A in
> secure RAM via cpu_memory_rw_debug(). While performing the S1
> translation, we call S1_ptw_translate() on the page table descriptor
> address, with an MMU index of ARMMMUIdx_Phys_S. At the moment
> S1_ptw_translate() doesn't interpret this as a secure access, and as a
> result we attempt to read the page table descriptor from the non-secure
> address space, which fails.
>
> Fixes: fe4a5472ccd6 ("target/arm: Use get_phys_addr_with_struct in 
> S1_ptw_translate")
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> I'm not entirely sure why the semihosting parameters are accessed
> through stage-1 translation rather than directly as physical addresses,
> but I'm not familiar with semihosting.

The semihosting ABI says the guest code should pass "a pointer
to the parameter block". It doesn't say explicitly, but the
straightforward interpretation is "a pointer that the guest
itself could dereference to read/write the values", which means
a virtual address, not a physical one. It would be pretty
painful for the guest to have to figure out "what is the
physaddr for this virtual address" to pass it to the semihosting
call.

Do you have a repro case for this bug? Did it work
before commit fe4a5472ccd6 ?

> ---
>  target/arm/ptw.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 9aaff1546a..e3a738c28e 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -465,10 +465,8 @@ static bool S1_ptw_translate(CPUARMState *env, 
> S1Translate *ptw,
>          S1Translate s2ptw = {
>              .in_mmu_idx = s2_mmu_idx,
>              .in_ptw_idx = ptw_idx_for_stage_2(env, s2_mmu_idx),
> -            .in_secure = s2_mmu_idx == ARMMMUIdx_Stage2_S,
> -            .in_space = (s2_mmu_idx == ARMMMUIdx_Stage2_S ? ARMSS_Secure
> -                         : space == ARMSS_Realm ? ARMSS_Realm
> -                         : ARMSS_NonSecure),
> +            .in_secure = is_secure,
> +            .in_space = space,

If the problem is fe4a5472ccd6 then this seems an odd change to
be making, because in_secure and in_space were set that way
before that commit too...

>              .in_debug = true,
>          };
>          GetPhysAddrResult s2 = { };

thanks
-- PMM