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 > --- > 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é > --- > 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, >