Re: [Qemu-devel] [Qemu-arm] [PATCH] accel/tcg: Check whether TLB entry is RAM consistently with how we set it up

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

[-- Attachment #1: Type: text/plain, Size: 6923 bytes --]

Hi Peter,

On 07/13/2018 12:09 PM, Peter Maydell wrote:
> We set up TLB entries in tlb_set_page_with_attrs(), where we have
> some logic for determining whether the TLB entry is considered
> to be RAM-backed, and thus has a valid addend field. When we
> look at the TLB entry in get_page_addr_code(), we use different
> logic for determining whether to treat the page as RAM-backed
> and use the addend field. This is confusing, and in fact buggy,
> because the code in tlb_set_page_with_attrs() correctly decides
> that rom_device memory regions not in romd mode are not RAM-backed,
> but the code in get_page_addr_code() thinks they are RAM-backed.
> This typically results in "Bad ram pointer" assertion if the
> guest tries to execute from such a memory region.
> 
> Fix this by making get_page_addr_code() just look at the
> TLB_MMIO bit in the code_address field of the TLB, which
> tlb_set_page_with_attrs() sets if and only if the addend
> field is not valid for code execution.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This patch is based on:
>  * [PATCH for-3.0 0/2] accel/tcg: fix get_page_addr_code() victim TLB lookups
>  * [PATCH 0/6] accel/tcg: Support execution from MMIO and small MMU regions
> Based-on: <20180713141636.18665-1-peter.maydell@linaro.org>
> Based-on: <20180710160013.26559-1-peter.maydell@linaro.org>
> 
> It would be possible to fix the 'bad ram pointer' in a more
> minimalist way (by making memory_region_is_unassigned()
> check "!memory_region_is_romd(mr)" rather than "!mr->rom_device")
> but since for 3.0 the only thing this does is turn that assert
> failure into the "can't execute from non-ram" error-exit it
> doesn't seem worth fixing for 3.0. After 3.0 the small-MMU-region
> exec support makes it more interesting. NB that if your
> rom-device is pflash then being able to execute from it when
> it's not in romd mode is not likely to actually help you, since
> pflash status code returns are seldom valid instructions :-)
> 
> Philippe: you might like to try this lot with your MIPS
> code that was getting the bad ram addr error?

Yes! You're my hero =)

Diff:

 Power-On
 ...
 PFLASH: pflash_write: flash reset asked (98 f0)
 pflash_reset reset
 mips_cpu_handle_mmu_fault pc 94760050 ad a8610914 rw 0 mmu_idx 3
 mips_cpu_handle_mmu_fault address=a8610914 physical 0000000008610914 prot 3
 mips_cpu_handle_mmu_fault pc 900034a4 ad 900034a4 rw 2 mmu_idx 3
 mips_cpu_handle_mmu_fault address=900034a4 physical 00000000100034a4 prot 3
-qemu-system-mips: Bad ram pointer 0x4a4
+pflash_read offset:0x34a4 cmd:0x00 width:4 wcycle:0
+pflash_data_read32 data offset:0x34a4 value:0x14400057
 ...
+mips_cpu_handle_mmu_fault pc 90003668 ad 90003668 rw 2 mmu_idx 3
+mips_cpu_handle_mmu_fault address=90003668 physical 0000000010003668 prot 3
 ...
+HW revision is 9.0.0.0
+
+Flash autodetection returned: 0xFFFFFFF6
+VendorID: 0xC2C2       DeviceID: 0xA8A8
+
+Boot Failed. Go to the lab !!

Many thanks for fixing this!

Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  include/exec/exec-all.h |  2 --
>  accel/tcg/cputlb.c      | 30 +++++++++---------------------
>  exec.c                  |  6 ------
>  3 files changed, 9 insertions(+), 29 deletions(-)
> 
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index da73e3bfed2..5f781255826 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -502,8 +502,6 @@ hwaddr memory_region_section_get_iotlb(CPUState *cpu,
>                                         hwaddr paddr, hwaddr xlat,
>                                         int prot,
>                                         target_ulong *address);
> -bool memory_region_is_unassigned(MemoryRegion *mr);
> -
>  #endif
>  
>  /* vl.c */
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index 754795ff253..5e5a2a2616c 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -926,10 +926,6 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
>  {
>      int mmu_idx, index;
>      void *p;
> -    MemoryRegion *mr;
> -    MemoryRegionSection *section;
> -    CPUState *cpu = ENV_GET_CPU(env);
> -    CPUIOTLBEntry *iotlbentry;
>  
>      index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
>      mmu_idx = cpu_mmu_index(env, true);
> @@ -939,29 +935,21 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
>          }
>          assert(tlb_hit(env->tlb_table[mmu_idx][index].addr_code, addr));
>      }
> +    assert(tlb_hit(env->tlb_table[mmu_idx][index].addr_code, addr));
>  
> -    if (unlikely(env->tlb_table[mmu_idx][index].addr_code & TLB_RECHECK)) {
> +    if (unlikely(env->tlb_table[mmu_idx][index].addr_code &
> +                 (TLB_RECHECK | TLB_MMIO))) {
>          /*
> -         * This is a TLB_RECHECK access, where the MMU protection
> -         * covers a smaller range than a target page. Return -1 to
> -         * indicate that we cannot simply execute from RAM here;
> -         * we will perform the necessary repeat of the MMU check
> -         * when the "execute a single insn" code performs the
> -         * load of the guest insn.
> +         * Return -1 if we can't translate and execute from an entire
> +         * page of RAM here, which will cause us to execute by loading
> +         * and translating one insn at a time, without caching:
> +         *  - TLB_RECHECK: means the MMU protection covers a smaller range
> +         *    than a target page, so we must redo the MMU check every insn
> +         *  - TLB_MMIO: region is not backed by RAM
>           */
>          return -1;
>      }
>  
> -    iotlbentry = &env->iotlb[mmu_idx][index];
> -    section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs);
> -    mr = section->mr;
> -    if (memory_region_is_unassigned(mr)) {
> -        /*
> -         * Not guest RAM, so there is no ram_addr_t for it. Return -1,
> -         * and we will execute a single insn from this device.
> -         */
> -        return -1;
> -    }
>      p = (void *)((uintptr_t)addr + env->tlb_table[mmu_idx][index].addend);
>      return qemu_ram_addr_from_host_nofail(p);
>  }
> diff --git a/exec.c b/exec.c
> index 4f5df07b6a2..e7be0761c28 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -402,12 +402,6 @@ static MemoryRegionSection *phys_page_find(AddressSpaceDispatch *d, hwaddr addr)
>      }
>  }
>  
> -bool memory_region_is_unassigned(MemoryRegion *mr)
> -{
> -    return mr != &io_mem_rom && mr != &io_mem_notdirty && !mr->rom_device
> -        && mr != &io_mem_watch;
> -}
> -
>  /* Called from RCU critical section */
>  static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d,
>                                                          hwaddr addr,
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]