Re: [PATCH] target/riscv: Fix incorrect PTE merge in walk_pte

[Ralf Ramsauer <ralf.ramsauer@oth-regensburg.de>]

On 01/04/2022 14:22, Ralf Ramsauer wrote:
> Two non-subsequent PTEs can be mapped to subsequent paddrs. In this
> case, walk_pte will erroneously merge them.
> 
> Enforce the split up, by tracking the virtual base address.
> 
> Let's say we have the mapping:
> 0x81200000 -> 0x89623000 (4K)
> 0x8120f000 -> 0x89624000 (4K)
> 
> Before, walk_pte would have shown:
> 
> vaddr            paddr            size             attr
> ---------------- ---------------- ---------------- -------
> 0000000081200000 0000000089623000 0000000000002000 rwxu-ad
> 
> as it only checks for subsequent paddrs. With this patch, it becomes:
> 
> vaddr            paddr            size             attr
> ---------------- ---------------- ---------------- -------
> 0000000081200000 0000000089623000 0000000000001000 rwxu-ad
> 000000008120f000 0000000089624000 0000000000001000 rwxu-ad
> 
> Signed-off-by: Ralf Ramsauer <ralf.ramsauer@oth-regensburg.de>
> ---
>   target/riscv/monitor.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/target/riscv/monitor.c b/target/riscv/monitor.c
> index 7efb4b62c1..60e3edd0ad 100644
> --- a/target/riscv/monitor.c
> +++ b/target/riscv/monitor.c
> @@ -84,6 +84,7 @@ static void walk_pte(Monitor *mon, hwaddr base, target_ulong start,
>   {
>       hwaddr pte_addr;
>       hwaddr paddr;
> +    target_ulong last_start = -1;
>       target_ulong pgsize;
>       target_ulong pte;
>       int ptshift;
> @@ -116,13 +117,15 @@ static void walk_pte(Monitor *mon, hwaddr base, target_ulong start,
>                    * contiguous mapped block details.
>                    */
>                   if ((*last_attr != attr) ||
> -                    (*last_paddr + *last_size != paddr)) {
> +                    (*last_paddr + *last_size != paddr) ||
> +                    (last_start + *last_size != start)) {
>                       print_pte(mon, va_bits, *vbase, *pbase,
>                                 *last_paddr + *last_size - *pbase, *last_attr);
>   
>                       *vbase = start;
>                       *pbase = paddr;
>                       *last_attr = attr;
> +                    last_start = start;
>                   }

Yikes, there's a small bug in my patch that I failed to see:
last_addr = start should be outside the curly brackets, otherwise it 
will rip up too much regions.

I'll return with a V2.

Thanks
   Ralf

>   
>                   *last_paddr = paddr;