| Message ID | 20180620130619.11362-2-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support M-profile MPU regions smaller than 1K | expand |
On 20/06/18 14:06, Peter Maydell wrote: > Add support for MMU protection regions that are smaller than > TARGET_PAGE_SIZE. We do this by marking the TLB entry for those > pages with a flag TLB_RECHECK. This flag causes us to always > take the slow-path for accesses. In the slow path we can then > special case them to always call tlb_fill() again, so we have > the correct information for the exact address being accessed. > > This change allows us to handle reading and writing from small > regions; we cannot deal with execution from the small region. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > accel/tcg/softmmu_template.h | 24 ++++--- > include/exec/cpu-all.h | 5 +- > accel/tcg/cputlb.c | 131 +++++++++++++++++++++++++++++------ > 3 files changed, 130 insertions(+), 30 deletions(-) > > diff --git a/accel/tcg/softmmu_template.h b/accel/tcg/softmmu_template.h > index 239ea6692b4..c47591c9709 100644 > --- a/accel/tcg/softmmu_template.h > +++ b/accel/tcg/softmmu_template.h > @@ -98,10 +98,12 @@ > static inline DATA_TYPE glue(io_read, SUFFIX)(CPUArchState *env, > size_t mmu_idx, size_t index, > target_ulong addr, > - uintptr_t retaddr) > + uintptr_t retaddr, > + bool recheck) > { > CPUIOTLBEntry *iotlbentry = &env->iotlb[mmu_idx][index]; > - return io_readx(env, iotlbentry, mmu_idx, addr, retaddr, DATA_SIZE); > + return io_readx(env, iotlbentry, mmu_idx, addr, retaddr, recheck, > + DATA_SIZE); > } > #endif > > @@ -138,7 +140,8 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env, target_ulong addr, > > /* ??? Note that the io helpers always read data in the target > byte ordering. We should push the LE/BE request down into io. */ > - res = glue(io_read, SUFFIX)(env, mmu_idx, index, addr, retaddr); > + res = glue(io_read, SUFFIX)(env, mmu_idx, index, addr, retaddr, > + tlb_addr & TLB_RECHECK); > res = TGT_LE(res); > return res; > } > @@ -205,7 +208,8 @@ WORD_TYPE helper_be_ld_name(CPUArchState *env, target_ulong addr, > > /* ??? Note that the io helpers always read data in the target > byte ordering. We should push the LE/BE request down into io. */ > - res = glue(io_read, SUFFIX)(env, mmu_idx, index, addr, retaddr); > + res = glue(io_read, SUFFIX)(env, mmu_idx, index, addr, retaddr, > + tlb_addr & TLB_RECHECK); > res = TGT_BE(res); > return res; > } > @@ -259,10 +263,12 @@ static inline void glue(io_write, SUFFIX)(CPUArchState *env, > size_t mmu_idx, size_t index, > DATA_TYPE val, > target_ulong addr, > - uintptr_t retaddr) > + uintptr_t retaddr, > + bool recheck) > { > CPUIOTLBEntry *iotlbentry = &env->iotlb[mmu_idx][index]; > - return io_writex(env, iotlbentry, mmu_idx, val, addr, retaddr, DATA_SIZE); > + return io_writex(env, iotlbentry, mmu_idx, val, addr, retaddr, > + recheck, DATA_SIZE); > } > > void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val, > @@ -298,7 +304,8 @@ void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val, > /* ??? Note that the io helpers always read data in the target > byte ordering. We should push the LE/BE request down into io. */ > val = TGT_LE(val); > - glue(io_write, SUFFIX)(env, mmu_idx, index, val, addr, retaddr); > + glue(io_write, SUFFIX)(env, mmu_idx, index, val, addr, > + retaddr, tlb_addr & TLB_RECHECK); > return; > } > > @@ -375,7 +382,8 @@ void helper_be_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val, > /* ??? Note that the io helpers always read data in the target > byte ordering. We should push the LE/BE request down into io. */ > val = TGT_BE(val); > - glue(io_write, SUFFIX)(env, mmu_idx, index, val, addr, retaddr); > + glue(io_write, SUFFIX)(env, mmu_idx, index, val, addr, retaddr, > + tlb_addr & TLB_RECHECK); > return; > } > > diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h > index 7fa726b8e36..7338f57062f 100644 > --- a/include/exec/cpu-all.h > +++ b/include/exec/cpu-all.h > @@ -330,11 +330,14 @@ CPUArchState *cpu_copy(CPUArchState *env); > #define TLB_NOTDIRTY (1 << (TARGET_PAGE_BITS - 2)) > /* Set if TLB entry is an IO callback. */ > #define TLB_MMIO (1 << (TARGET_PAGE_BITS - 3)) > +/* Set if TLB entry must have MMU lookup repeated for every access */ > +#define TLB_RECHECK (1 << (TARGET_PAGE_BITS - 4)) > > /* Use this mask to check interception with an alignment mask > * in a TCG backend. > */ > -#define TLB_FLAGS_MASK (TLB_INVALID_MASK | TLB_NOTDIRTY | TLB_MMIO) > +#define TLB_FLAGS_MASK (TLB_INVALID_MASK | TLB_NOTDIRTY | TLB_MMIO \ > + | TLB_RECHECK) > > void dump_exec_info(FILE *f, fprintf_function cpu_fprintf); > void dump_opcount_info(FILE *f, fprintf_function cpu_fprintf); > diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c > index 0a721bb9c40..d893452295f 100644 > --- a/accel/tcg/cputlb.c > +++ b/accel/tcg/cputlb.c > @@ -621,27 +621,42 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, > target_ulong code_address; > uintptr_t addend; > CPUTLBEntry *te, *tv, tn; > - hwaddr iotlb, xlat, sz; > + hwaddr iotlb, xlat, sz, paddr_page; > + target_ulong vaddr_page; > unsigned vidx = env->vtlb_index++ % CPU_VTLB_SIZE; > int asidx = cpu_asidx_from_attrs(cpu, attrs); > > assert_cpu_is_self(cpu); > - assert(size >= TARGET_PAGE_SIZE); > - if (size != TARGET_PAGE_SIZE) { > - tlb_add_large_page(env, vaddr, size); > - } > > - sz = size; > - section = address_space_translate_for_iotlb(cpu, asidx, paddr, &xlat, &sz, > - attrs, &prot); > + if (size < TARGET_PAGE_SIZE) { > + sz = TARGET_PAGE_SIZE; > + } else { > + if (size > TARGET_PAGE_SIZE) { > + tlb_add_large_page(env, vaddr, size); > + } > + sz = size; > + } > + vaddr_page = vaddr & TARGET_PAGE_MASK; > + paddr_page = paddr & TARGET_PAGE_MASK; > + > + section = address_space_translate_for_iotlb(cpu, asidx, paddr_page, > + &xlat, &sz, attrs, &prot); > assert(sz >= TARGET_PAGE_SIZE); > > tlb_debug("vaddr=" TARGET_FMT_lx " paddr=0x" TARGET_FMT_plx > " prot=%x idx=%d\n", > vaddr, paddr, prot, mmu_idx); > > - address = vaddr; > - if (!memory_region_is_ram(section->mr) && !memory_region_is_romd(section->mr)) { > + address = vaddr_page; > + if (size < TARGET_PAGE_SIZE) { > + /* > + * Slow-path the TLB entries; we will repeat the MMU check and TLB > + * fill on every access. > + */ > + address |= TLB_RECHECK; > + } > + if (!memory_region_is_ram(section->mr) && > + !memory_region_is_romd(section->mr)) { > /* IO memory case */ > address |= TLB_MMIO; > addend = 0; > @@ -651,10 +666,10 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, > } > > code_address = address; > - iotlb = memory_region_section_get_iotlb(cpu, section, vaddr, paddr, xlat, > - prot, &address); > + iotlb = memory_region_section_get_iotlb(cpu, section, vaddr_page, > + paddr_page, xlat, prot, &address); > > - index = (vaddr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1); > + index = (vaddr_page >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1); > te = &env->tlb_table[mmu_idx][index]; > /* do not discard the translation in te, evict it into a victim tlb */ > tv = &env->tlb_v_table[mmu_idx][vidx]; > @@ -670,18 +685,18 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, > * TARGET_PAGE_BITS, and either > * + the ram_addr_t of the page base of the target RAM (if NOTDIRTY or ROM) > * + the offset within section->mr of the page base (otherwise) > - * We subtract the vaddr (which is page aligned and thus won't > + * We subtract the vaddr_page (which is page aligned and thus won't > * disturb the low bits) to give an offset which can be added to the > * (non-page-aligned) vaddr of the eventual memory access to get > * the MemoryRegion offset for the access. Note that the vaddr we > * subtract here is that of the page base, and not the same as the > * vaddr we add back in io_readx()/io_writex()/get_page_addr_code(). > */ > - env->iotlb[mmu_idx][index].addr = iotlb - vaddr; > + env->iotlb[mmu_idx][index].addr = iotlb - vaddr_page; > env->iotlb[mmu_idx][index].attrs = attrs; > > /* Now calculate the new entry */ > - tn.addend = addend - vaddr; > + tn.addend = addend - vaddr_page; > if (prot & PAGE_READ) { > tn.addr_read = address; > } else { > @@ -702,7 +717,7 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, > tn.addr_write = address | TLB_MMIO; > } else if (memory_region_is_ram(section->mr) > && cpu_physical_memory_is_clean( > - memory_region_get_ram_addr(section->mr) + xlat)) { > + memory_region_get_ram_addr(section->mr) + xlat)) { > tn.addr_write = address | TLB_NOTDIRTY; > } else { > tn.addr_write = address; > @@ -775,7 +790,8 @@ static inline ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr) > > static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry, > int mmu_idx, > - target_ulong addr, uintptr_t retaddr, int size) > + target_ulong addr, uintptr_t retaddr, > + bool recheck, int size) > { > CPUState *cpu = ENV_GET_CPU(env); > hwaddr mr_offset; > @@ -785,6 +801,29 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry, > bool locked = false; > MemTxResult r; > > + if (recheck) { > + /* > + * This is a TLB_RECHECK access, where the MMU protection > + * covers a smaller range than a target page, and we must > + * repeat the MMU check here. This tlb_fill() call might > + * longjump out if this access should cause a guest exception. > + */ > + int index; > + target_ulong tlb_addr; > + > + tlb_fill(cpu, addr, size, MMU_DATA_LOAD, mmu_idx, retaddr); > + > + index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1); > + tlb_addr = env->tlb_table[mmu_idx][index].addr_read; > + if (!(tlb_addr & ~(TARGET_PAGE_MASK | TLB_RECHECK))) { > + /* RAM access */ > + uintptr_t haddr = addr + env->tlb_table[mmu_idx][index].addend; > + > + return ldn_p((void *)haddr, size); > + } > + /* Fall through for handling IO accesses */ > + } > + > section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs); > mr = section->mr; > mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr; > @@ -819,7 +858,7 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry, > static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry, > int mmu_idx, > uint64_t val, target_ulong addr, > - uintptr_t retaddr, int size) > + uintptr_t retaddr, bool recheck, int size) > { > CPUState *cpu = ENV_GET_CPU(env); > hwaddr mr_offset; > @@ -828,6 +867,30 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry, > bool locked = false; > MemTxResult r; > > + if (recheck) { > + /* > + * This is a TLB_RECHECK access, where the MMU protection > + * covers a smaller range than a target page, and we must > + * repeat the MMU check here. This tlb_fill() call might > + * longjump out if this access should cause a guest exception. > + */ > + int index; > + target_ulong tlb_addr; > + > + tlb_fill(cpu, addr, size, MMU_DATA_STORE, mmu_idx, retaddr); > + > + index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1); > + tlb_addr = env->tlb_table[mmu_idx][index].addr_write; > + if (!(tlb_addr & ~(TARGET_PAGE_MASK | TLB_RECHECK))) { > + /* RAM access */ > + uintptr_t haddr = addr + env->tlb_table[mmu_idx][index].addend; > + > + stn_p((void *)haddr, size, val); > + return; > + } > + /* Fall through for handling IO accesses */ > + } > + > section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs); > mr = section->mr; > mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr; > @@ -911,6 +974,32 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr) > tlb_fill(ENV_GET_CPU(env), addr, 0, MMU_INST_FETCH, mmu_idx, 0); > } > } > + > + if (unlikely(env->tlb_table[mmu_idx][index].addr_code & TLB_RECHECK)) { > + /* > + * This is a TLB_RECHECK access, where the MMU protection > + * covers a smaller range than a target page, and we must > + * repeat the MMU check here. This tlb_fill() call might > + * longjump out if this access should cause a guest exception. > + */ > + int index; > + target_ulong tlb_addr; > + > + tlb_fill(cpu, addr, 0, MMU_INST_FETCH, mmu_idx, 0); > + > + index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1); > + tlb_addr = env->tlb_table[mmu_idx][index].addr_code; > + if (!(tlb_addr & ~(TARGET_PAGE_MASK | TLB_RECHECK))) { > + /* RAM access. We can't handle this, so for now just stop */ > + cpu_abort(cpu, "Unable to handle guest executing from RAM within " > + "a small MPU region at 0x" TARGET_FMT_lx, addr); > + } > + /* > + * Fall through to handle IO accesses (which will almost certainly > + * also result in failure) > + */ > + } > + > iotlbentry = &env->iotlb[mmu_idx][index]; > section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs); > mr = section->mr; > @@ -1019,8 +1108,8 @@ static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr, > tlb_addr = tlbe->addr_write & ~TLB_INVALID_MASK; > } > > - /* Notice an IO access */ > - if (unlikely(tlb_addr & TLB_MMIO)) { > + /* Notice an IO access or a needs-MMU-lookup access */ > + if (unlikely(tlb_addr & (TLB_MMIO | TLB_RECHECK))) { > /* There's really nothing that can be done to > support this apart from stop-the-world. */ > goto stop_the_world; This patch is very interesting as forcing the slow path is something required to implement the sun4u MMU IE (invert endian) bit - for some background see Richard's email at https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg02835.html. Presumably there is nothing here that would prevent the slow path being used outside of TLB_RECHECK? ATB, Mark.
On 20 June 2018 at 16:46, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > On 20/06/18 14:06, Peter Maydell wrote: > >> Add support for MMU protection regions that are smaller than >> TARGET_PAGE_SIZE. We do this by marking the TLB entry for those >> pages with a flag TLB_RECHECK. This flag causes us to always >> take the slow-path for accesses. In the slow path we can then >> special case them to always call tlb_fill() again, so we have >> the correct information for the exact address being accessed. >> >> This change allows us to handle reading and writing from small >> regions; we cannot deal with execution from the small region. >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > This patch is very interesting as forcing the slow path is something > required to implement the sun4u MMU IE (invert endian) bit - for some > background see Richard's email at > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg02835.html. > > Presumably there is nothing here that would prevent the slow path being used > outside of TLB_RECHECK? Nope; we already have various things that force a slowpath. Essentially all you need to do is ensure that some lowbit in the tlb addr fields is set, and we're only moderately constrained in how many of those we have. You might or might not be able to use TLB_RECHECK, I don't know. thanks -- PMM
Hi Peter, On Wed, Jun 20, 2018 at 6:06 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > Add support for MMU protection regions that are smaller than > TARGET_PAGE_SIZE. We do this by marking the TLB entry for those > pages with a flag TLB_RECHECK. This flag causes us to always > take the slow-path for accesses. In the slow path we can then > special case them to always call tlb_fill() again, so we have > the correct information for the exact address being accessed. > > This change allows us to handle reading and writing from small > regions; we cannot deal with execution from the small region. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > accel/tcg/softmmu_template.h | 24 ++++--- > include/exec/cpu-all.h | 5 +- > accel/tcg/cputlb.c | 131 +++++++++++++++++++++++++++++------ > 3 files changed, 130 insertions(+), 30 deletions(-) I'm observing the following failure with xtensa tests: (qemu) qemu: fatal: Unable to handle guest executing from RAM within a small MPU region at 0xd0000804 Bisection points to this patch. Any idea what happened? -- Thanks. -- Max
On Sat, Jun 30, 2018 at 12:20 PM, Max Filippov <jcmvbkbc@gmail.com> wrote: > Hi Peter, > > On Wed, Jun 20, 2018 at 6:06 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> Add support for MMU protection regions that are smaller than >> TARGET_PAGE_SIZE. We do this by marking the TLB entry for those >> pages with a flag TLB_RECHECK. This flag causes us to always >> take the slow-path for accesses. In the slow path we can then >> special case them to always call tlb_fill() again, so we have >> the correct information for the exact address being accessed. >> >> This change allows us to handle reading and writing from small >> regions; we cannot deal with execution from the small region. >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> --- >> accel/tcg/softmmu_template.h | 24 ++++--- >> include/exec/cpu-all.h | 5 +- >> accel/tcg/cputlb.c | 131 +++++++++++++++++++++++++++++------ >> 3 files changed, 130 insertions(+), 30 deletions(-) > > I'm observing the following failure with xtensa tests: > > (qemu) qemu: fatal: Unable to handle guest executing from RAM within a > small MPU region at 0xd0000804 > > Bisection points to this patch. Any idea what happened? Ok, I think I've found the issue: the following check in the get_page_addr_code does not work correctly when -1 is in the addr_code in the QEMU TLB: if (unlikely(env->tlb_table[mmu_idx][index].addr_code & TLB_RECHECK)) tlb_set_page_with_attrs sets addr_code to -1 in the TLB entry when the translation is not executable. -- Thanks. -- Max
On Sat, Jun 30, 2018 at 12:42 PM, Max Filippov <jcmvbkbc@gmail.com> wrote: > On Sat, Jun 30, 2018 at 12:20 PM, Max Filippov <jcmvbkbc@gmail.com> wrote: >> Hi Peter, >> >> On Wed, Jun 20, 2018 at 6:06 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> Add support for MMU protection regions that are smaller than >>> TARGET_PAGE_SIZE. We do this by marking the TLB entry for those >>> pages with a flag TLB_RECHECK. This flag causes us to always >>> take the slow-path for accesses. In the slow path we can then >>> special case them to always call tlb_fill() again, so we have >>> the correct information for the exact address being accessed. >>> >>> This change allows us to handle reading and writing from small >>> regions; we cannot deal with execution from the small region. >>> >>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >>> --- >>> accel/tcg/softmmu_template.h | 24 ++++--- >>> include/exec/cpu-all.h | 5 +- >>> accel/tcg/cputlb.c | 131 +++++++++++++++++++++++++++++------ >>> 3 files changed, 130 insertions(+), 30 deletions(-) >> >> I'm observing the following failure with xtensa tests: >> >> (qemu) qemu: fatal: Unable to handle guest executing from RAM within a >> small MPU region at 0xd0000804 >> >> Bisection points to this patch. Any idea what happened? > > Ok, I think I've found the issue: the following check in the > get_page_addr_code does not work correctly when -1 is in the > addr_code in the QEMU TLB: > > if (unlikely(env->tlb_table[mmu_idx][index].addr_code & TLB_RECHECK)) > > tlb_set_page_with_attrs sets addr_code to -1 in the TLB entry > when the translation is not executable. Looks like it can be fixed with the following: diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c index eebe97dabb75..633cffe9ed74 100644 --- a/accel/tcg/cputlb.c +++ b/accel/tcg/cputlb.c @@ -692,16 +692,16 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, if (prot & PAGE_READ) { tn.addr_read = address; } else { - tn.addr_read = -1; + tn.addr_read = TLB_INVALID_MASK; } if (prot & PAGE_EXEC) { tn.addr_code = code_address; } else { - tn.addr_code = -1; + tn.addr_code = TLB_INVALID_MASK; } - tn.addr_write = -1; + tn.addr_write = TLB_INVALID_MASK; if (prot & PAGE_WRITE) { if ((memory_region_is_ram(section->mr) && section->readonly) || memory_region_is_romd(section->mr)) { -- Thanks. -- Max
On 30 June 2018 at 20:42, Max Filippov <jcmvbkbc@gmail.com> wrote: > On Sat, Jun 30, 2018 at 12:20 PM, Max Filippov <jcmvbkbc@gmail.com> wrote: >> I'm observing the following failure with xtensa tests: >> >> (qemu) qemu: fatal: Unable to handle guest executing from RAM within a >> small MPU region at 0xd0000804 >> >> Bisection points to this patch. Any idea what happened? > > Ok, I think I've found the issue: the following check in the > get_page_addr_code does not work correctly when -1 is in the > addr_code in the QEMU TLB: > > if (unlikely(env->tlb_table[mmu_idx][index].addr_code & TLB_RECHECK)) Yes, Laurent ran into that and I sent a fix out on Friday: http://patchwork.ozlabs.org/project/qemu-devel/list/?series=52914 -- could you give that patchset a try? thanks -- PMM
On 30 June 2018 at 21:08, Peter Maydell <peter.maydell@linaro.org> wrote: > On 30 June 2018 at 20:42, Max Filippov <jcmvbkbc@gmail.com> wrote: >> On Sat, Jun 30, 2018 at 12:20 PM, Max Filippov <jcmvbkbc@gmail.com> wrote: >>> I'm observing the following failure with xtensa tests: >>> >>> (qemu) qemu: fatal: Unable to handle guest executing from RAM within a >>> small MPU region at 0xd0000804 >>> >>> Bisection points to this patch. Any idea what happened? >> >> Ok, I think I've found the issue: the following check in the >> get_page_addr_code does not work correctly when -1 is in the >> addr_code in the QEMU TLB: >> >> if (unlikely(env->tlb_table[mmu_idx][index].addr_code & TLB_RECHECK)) > > Yes, Laurent ran into that and I sent a fix out on Friday: > http://patchwork.ozlabs.org/project/qemu-devel/list/?series=52914 ...oh, no, wait, you've hit the other bug I sent a fix for: http://patchwork.ozlabs.org/patch/937029/ thanks -- PMM
On Sat, Jun 30, 2018 at 1:10 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 30 June 2018 at 21:08, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 30 June 2018 at 20:42, Max Filippov <jcmvbkbc@gmail.com> wrote: >>> On Sat, Jun 30, 2018 at 12:20 PM, Max Filippov <jcmvbkbc@gmail.com> wrote: >>>> I'm observing the following failure with xtensa tests: >>>> >>>> (qemu) qemu: fatal: Unable to handle guest executing from RAM within a >>>> small MPU region at 0xd0000804 >>>> >>>> Bisection points to this patch. Any idea what happened? >>> >>> Ok, I think I've found the issue: the following check in the >>> get_page_addr_code does not work correctly when -1 is in the >>> addr_code in the QEMU TLB: >>> >>> if (unlikely(env->tlb_table[mmu_idx][index].addr_code & TLB_RECHECK)) >> >> Yes, Laurent ran into that and I sent a fix out on Friday: >> http://patchwork.ozlabs.org/project/qemu-devel/list/?series=52914 > > ...oh, no, wait, you've hit the other bug I sent a fix for: > http://patchwork.ozlabs.org/patch/937029/ Thanks, that last one fixes it for me. -- Thanks. -- Max
diff --git a/accel/tcg/softmmu_template.h b/accel/tcg/softmmu_template.h index 239ea6692b4..c47591c9709 100644 --- a/accel/tcg/softmmu_template.h +++ b/accel/tcg/softmmu_template.h @@ -98,10 +98,12 @@ static inline DATA_TYPE glue(io_read, SUFFIX)(CPUArchState *env, size_t mmu_idx, size_t index, target_ulong addr, - uintptr_t retaddr) + uintptr_t retaddr, + bool recheck) { CPUIOTLBEntry *iotlbentry = &env->iotlb[mmu_idx][index]; - return io_readx(env, iotlbentry, mmu_idx, addr, retaddr, DATA_SIZE); + return io_readx(env, iotlbentry, mmu_idx, addr, retaddr, recheck, + DATA_SIZE); } #endif @@ -138,7 +140,8 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env, target_ulong addr, /* ??? Note that the io helpers always read data in the target byte ordering. We should push the LE/BE request down into io. */ - res = glue(io_read, SUFFIX)(env, mmu_idx, index, addr, retaddr); + res = glue(io_read, SUFFIX)(env, mmu_idx, index, addr, retaddr, + tlb_addr & TLB_RECHECK); res = TGT_LE(res); return res; } @@ -205,7 +208,8 @@ WORD_TYPE helper_be_ld_name(CPUArchState *env, target_ulong addr, /* ??? Note that the io helpers always read data in the target byte ordering. We should push the LE/BE request down into io. */ - res = glue(io_read, SUFFIX)(env, mmu_idx, index, addr, retaddr); + res = glue(io_read, SUFFIX)(env, mmu_idx, index, addr, retaddr, + tlb_addr & TLB_RECHECK); res = TGT_BE(res); return res; } @@ -259,10 +263,12 @@ static inline void glue(io_write, SUFFIX)(CPUArchState *env, size_t mmu_idx, size_t index, DATA_TYPE val, target_ulong addr, - uintptr_t retaddr) + uintptr_t retaddr, + bool recheck) { CPUIOTLBEntry *iotlbentry = &env->iotlb[mmu_idx][index]; - return io_writex(env, iotlbentry, mmu_idx, val, addr, retaddr, DATA_SIZE); + return io_writex(env, iotlbentry, mmu_idx, val, addr, retaddr, + recheck, DATA_SIZE); } void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val, @@ -298,7 +304,8 @@ void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val, /* ??? Note that the io helpers always read data in the target byte ordering. We should push the LE/BE request down into io. */ val = TGT_LE(val); - glue(io_write, SUFFIX)(env, mmu_idx, index, val, addr, retaddr); + glue(io_write, SUFFIX)(env, mmu_idx, index, val, addr, + retaddr, tlb_addr & TLB_RECHECK); return; } @@ -375,7 +382,8 @@ void helper_be_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val, /* ??? Note that the io helpers always read data in the target byte ordering. We should push the LE/BE request down into io. */ val = TGT_BE(val); - glue(io_write, SUFFIX)(env, mmu_idx, index, val, addr, retaddr); + glue(io_write, SUFFIX)(env, mmu_idx, index, val, addr, retaddr, + tlb_addr & TLB_RECHECK); return; } diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h index 7fa726b8e36..7338f57062f 100644 --- a/include/exec/cpu-all.h +++ b/include/exec/cpu-all.h @@ -330,11 +330,14 @@ CPUArchState *cpu_copy(CPUArchState *env); #define TLB_NOTDIRTY (1 << (TARGET_PAGE_BITS - 2)) /* Set if TLB entry is an IO callback. */ #define TLB_MMIO (1 << (TARGET_PAGE_BITS - 3)) +/* Set if TLB entry must have MMU lookup repeated for every access */ +#define TLB_RECHECK (1 << (TARGET_PAGE_BITS - 4)) /* Use this mask to check interception with an alignment mask * in a TCG backend. */ -#define TLB_FLAGS_MASK (TLB_INVALID_MASK | TLB_NOTDIRTY | TLB_MMIO) +#define TLB_FLAGS_MASK (TLB_INVALID_MASK | TLB_NOTDIRTY | TLB_MMIO \ + | TLB_RECHECK) void dump_exec_info(FILE *f, fprintf_function cpu_fprintf); void dump_opcount_info(FILE *f, fprintf_function cpu_fprintf); diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c index 0a721bb9c40..d893452295f 100644 --- a/accel/tcg/cputlb.c +++ b/accel/tcg/cputlb.c @@ -621,27 +621,42 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, target_ulong code_address; uintptr_t addend; CPUTLBEntry *te, *tv, tn; - hwaddr iotlb, xlat, sz; + hwaddr iotlb, xlat, sz, paddr_page; + target_ulong vaddr_page; unsigned vidx = env->vtlb_index++ % CPU_VTLB_SIZE; int asidx = cpu_asidx_from_attrs(cpu, attrs); assert_cpu_is_self(cpu); - assert(size >= TARGET_PAGE_SIZE); - if (size != TARGET_PAGE_SIZE) { - tlb_add_large_page(env, vaddr, size); - } - sz = size; - section = address_space_translate_for_iotlb(cpu, asidx, paddr, &xlat, &sz, - attrs, &prot); + if (size < TARGET_PAGE_SIZE) { + sz = TARGET_PAGE_SIZE; + } else { + if (size > TARGET_PAGE_SIZE) { + tlb_add_large_page(env, vaddr, size); + } + sz = size; + } + vaddr_page = vaddr & TARGET_PAGE_MASK; + paddr_page = paddr & TARGET_PAGE_MASK; + + section = address_space_translate_for_iotlb(cpu, asidx, paddr_page, + &xlat, &sz, attrs, &prot); assert(sz >= TARGET_PAGE_SIZE); tlb_debug("vaddr=" TARGET_FMT_lx " paddr=0x" TARGET_FMT_plx " prot=%x idx=%d\n", vaddr, paddr, prot, mmu_idx); - address = vaddr; - if (!memory_region_is_ram(section->mr) && !memory_region_is_romd(section->mr)) { + address = vaddr_page; + if (size < TARGET_PAGE_SIZE) { + /* + * Slow-path the TLB entries; we will repeat the MMU check and TLB + * fill on every access. + */ + address |= TLB_RECHECK; + } + if (!memory_region_is_ram(section->mr) && + !memory_region_is_romd(section->mr)) { /* IO memory case */ address |= TLB_MMIO; addend = 0; @@ -651,10 +666,10 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, } code_address = address; - iotlb = memory_region_section_get_iotlb(cpu, section, vaddr, paddr, xlat, - prot, &address); + iotlb = memory_region_section_get_iotlb(cpu, section, vaddr_page, + paddr_page, xlat, prot, &address); - index = (vaddr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1); + index = (vaddr_page >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1); te = &env->tlb_table[mmu_idx][index]; /* do not discard the translation in te, evict it into a victim tlb */ tv = &env->tlb_v_table[mmu_idx][vidx]; @@ -670,18 +685,18 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, * TARGET_PAGE_BITS, and either * + the ram_addr_t of the page base of the target RAM (if NOTDIRTY or ROM) * + the offset within section->mr of the page base (otherwise) - * We subtract the vaddr (which is page aligned and thus won't + * We subtract the vaddr_page (which is page aligned and thus won't * disturb the low bits) to give an offset which can be added to the * (non-page-aligned) vaddr of the eventual memory access to get * the MemoryRegion offset for the access. Note that the vaddr we * subtract here is that of the page base, and not the same as the * vaddr we add back in io_readx()/io_writex()/get_page_addr_code(). */ - env->iotlb[mmu_idx][index].addr = iotlb - vaddr; + env->iotlb[mmu_idx][index].addr = iotlb - vaddr_page; env->iotlb[mmu_idx][index].attrs = attrs; /* Now calculate the new entry */ - tn.addend = addend - vaddr; + tn.addend = addend - vaddr_page; if (prot & PAGE_READ) { tn.addr_read = address; } else { @@ -702,7 +717,7 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, tn.addr_write = address | TLB_MMIO; } else if (memory_region_is_ram(section->mr) && cpu_physical_memory_is_clean( - memory_region_get_ram_addr(section->mr) + xlat)) { + memory_region_get_ram_addr(section->mr) + xlat)) { tn.addr_write = address | TLB_NOTDIRTY; } else { tn.addr_write = address; @@ -775,7 +790,8 @@ static inline ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr) static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry, int mmu_idx, - target_ulong addr, uintptr_t retaddr, int size) + target_ulong addr, uintptr_t retaddr, + bool recheck, int size) { CPUState *cpu = ENV_GET_CPU(env); hwaddr mr_offset; @@ -785,6 +801,29 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry, bool locked = false; MemTxResult r; + if (recheck) { + /* + * This is a TLB_RECHECK access, where the MMU protection + * covers a smaller range than a target page, and we must + * repeat the MMU check here. This tlb_fill() call might + * longjump out if this access should cause a guest exception. + */ + int index; + target_ulong tlb_addr; + + tlb_fill(cpu, addr, size, MMU_DATA_LOAD, mmu_idx, retaddr); + + index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1); + tlb_addr = env->tlb_table[mmu_idx][index].addr_read; + if (!(tlb_addr & ~(TARGET_PAGE_MASK | TLB_RECHECK))) { + /* RAM access */ + uintptr_t haddr = addr + env->tlb_table[mmu_idx][index].addend; + + return ldn_p((void *)haddr, size); + } + /* Fall through for handling IO accesses */ + } + section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs); mr = section->mr; mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr; @@ -819,7 +858,7 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry, static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry, int mmu_idx, uint64_t val, target_ulong addr, - uintptr_t retaddr, int size) + uintptr_t retaddr, bool recheck, int size) { CPUState *cpu = ENV_GET_CPU(env); hwaddr mr_offset; @@ -828,6 +867,30 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry, bool locked = false; MemTxResult r; + if (recheck) { + /* + * This is a TLB_RECHECK access, where the MMU protection + * covers a smaller range than a target page, and we must + * repeat the MMU check here. This tlb_fill() call might + * longjump out if this access should cause a guest exception. + */ + int index; + target_ulong tlb_addr; + + tlb_fill(cpu, addr, size, MMU_DATA_STORE, mmu_idx, retaddr); + + index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1); + tlb_addr = env->tlb_table[mmu_idx][index].addr_write; + if (!(tlb_addr & ~(TARGET_PAGE_MASK | TLB_RECHECK))) { + /* RAM access */ + uintptr_t haddr = addr + env->tlb_table[mmu_idx][index].addend; + + stn_p((void *)haddr, size, val); + return; + } + /* Fall through for handling IO accesses */ + } + section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs); mr = section->mr; mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr; @@ -911,6 +974,32 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr) tlb_fill(ENV_GET_CPU(env), addr, 0, MMU_INST_FETCH, mmu_idx, 0); } } + + if (unlikely(env->tlb_table[mmu_idx][index].addr_code & TLB_RECHECK)) { + /* + * This is a TLB_RECHECK access, where the MMU protection + * covers a smaller range than a target page, and we must + * repeat the MMU check here. This tlb_fill() call might + * longjump out if this access should cause a guest exception. + */ + int index; + target_ulong tlb_addr; + + tlb_fill(cpu, addr, 0, MMU_INST_FETCH, mmu_idx, 0); + + index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1); + tlb_addr = env->tlb_table[mmu_idx][index].addr_code; + if (!(tlb_addr & ~(TARGET_PAGE_MASK | TLB_RECHECK))) { + /* RAM access. We can't handle this, so for now just stop */ + cpu_abort(cpu, "Unable to handle guest executing from RAM within " + "a small MPU region at 0x" TARGET_FMT_lx, addr); + } + /* + * Fall through to handle IO accesses (which will almost certainly + * also result in failure) + */ + } + iotlbentry = &env->iotlb[mmu_idx][index]; section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs); mr = section->mr; @@ -1019,8 +1108,8 @@ static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr, tlb_addr = tlbe->addr_write & ~TLB_INVALID_MASK; } - /* Notice an IO access */ - if (unlikely(tlb_addr & TLB_MMIO)) { + /* Notice an IO access or a needs-MMU-lookup access */ + if (unlikely(tlb_addr & (TLB_MMIO | TLB_RECHECK))) { /* There's really nothing that can be done to support this apart from stop-the-world. */ goto stop_the_world;
Add support for MMU protection regions that are smaller than TARGET_PAGE_SIZE. We do this by marking the TLB entry for those pages with a flag TLB_RECHECK. This flag causes us to always take the slow-path for accesses. In the slow path we can then special case them to always call tlb_fill() again, so we have the correct information for the exact address being accessed. This change allows us to handle reading and writing from small regions; we cannot deal with execution from the small region. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- accel/tcg/softmmu_template.h | 24 ++++--- include/exec/cpu-all.h | 5 +- accel/tcg/cputlb.c | 131 +++++++++++++++++++++++++++++------ 3 files changed, 130 insertions(+), 30 deletions(-) -- 2.17.1