| Message ID | 20221004141051.110653-9-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tcg: CPUTLBEntryFull and TARGET_TB_PCREL | expand |
On Wed, Oct 5, 2022 at 1:11 AM Richard Henderson <richard.henderson@linaro.org> wrote: > > Now that we have collected all of the page data into > CPUTLBEntryFull, provide an interface to record that > all in one go, instead of using 4 arguments. This interface > allows CPUTLBEntryFull to be extended without having to > change the number of arguments. > > Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > include/exec/cpu-defs.h | 14 +++++++++++ > include/exec/exec-all.h | 22 ++++++++++++++++++ > accel/tcg/cputlb.c | 51 ++++++++++++++++++++++++++--------------- > 3 files changed, 69 insertions(+), 18 deletions(-) > > diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h > index f70f54d850..5e12cc1854 100644 > --- a/include/exec/cpu-defs.h > +++ b/include/exec/cpu-defs.h > @@ -148,7 +148,21 @@ typedef struct CPUTLBEntryFull { > * + the offset within the target MemoryRegion (otherwise) > */ > hwaddr xlat_section; > + > + /* > + * @phys_addr contains the physical address in the address space > + * given by cpu_asidx_from_attrs(cpu, @attrs). > + */ > + hwaddr phys_addr; > + > + /* @attrs contains the memory transaction attributes for the page. */ > MemTxAttrs attrs; > + > + /* @prot contains the complete protections for the page. */ > + uint8_t prot; > + > + /* @lg_page_size contains the log2 of the page size. */ > + uint8_t lg_page_size; > } CPUTLBEntryFull; > > /* > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index d255d69bc1..b1b920a713 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -257,6 +257,28 @@ void tlb_flush_range_by_mmuidx_all_cpus_synced(CPUState *cpu, > uint16_t idxmap, > unsigned bits); > > +/** > + * tlb_set_page_full: > + * @cpu: CPU context > + * @mmu_idx: mmu index of the tlb to modify > + * @vaddr: virtual address of the entry to add > + * @full: the details of the tlb entry > + * > + * Add an entry to @cpu tlb index @mmu_idx. All of the fields of > + * @full must be filled, except for xlat_section, and constitute > + * the complete description of the translated page. > + * > + * This is generally called by the target tlb_fill function after > + * having performed a successful page table walk to find the physical > + * address and attributes for the translation. > + * > + * At most one entry for a given virtual address is permitted. Only a > + * single TARGET_PAGE_SIZE region is mapped; @full->lg_page_size is only > + * used by tlb_flush_page. > + */ > +void tlb_set_page_full(CPUState *cpu, int mmu_idx, target_ulong vaddr, > + CPUTLBEntryFull *full); > + > /** > * tlb_set_page_with_attrs: > * @cpu: CPU to add this TLB entry for > diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c > index e3ee4260bd..361078471b 100644 > --- a/accel/tcg/cputlb.c > +++ b/accel/tcg/cputlb.c > @@ -1095,16 +1095,16 @@ static void tlb_add_large_page(CPUArchState *env, int mmu_idx, > env_tlb(env)->d[mmu_idx].large_page_mask = lp_mask; > } > > -/* Add a new TLB entry. At most one entry for a given virtual address > +/* > + * Add a new TLB entry. At most one entry for a given virtual address > * is permitted. Only a single TARGET_PAGE_SIZE region is mapped, the > * supplied size is only used by tlb_flush_page. > * > * Called from TCG-generated code, which is under an RCU read-side > * critical section. > */ > -void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, > - hwaddr paddr, MemTxAttrs attrs, int prot, > - int mmu_idx, target_ulong size) > +void tlb_set_page_full(CPUState *cpu, int mmu_idx, > + target_ulong vaddr, CPUTLBEntryFull *full) > { > CPUArchState *env = cpu->env_ptr; > CPUTLB *tlb = env_tlb(env); > @@ -1117,35 +1117,36 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, > CPUTLBEntry *te, tn; > hwaddr iotlb, xlat, sz, paddr_page; > target_ulong vaddr_page; > - int asidx = cpu_asidx_from_attrs(cpu, attrs); > - int wp_flags; > + int asidx, wp_flags, prot; > bool is_ram, is_romd; > > assert_cpu_is_self(cpu); > > - if (size <= TARGET_PAGE_SIZE) { > + if (full->lg_page_size <= TARGET_PAGE_BITS) { > sz = TARGET_PAGE_SIZE; > } else { > - tlb_add_large_page(env, mmu_idx, vaddr, size); > - sz = size; > + sz = (hwaddr)1 << full->lg_page_size; > + tlb_add_large_page(env, mmu_idx, vaddr, sz); > } > vaddr_page = vaddr & TARGET_PAGE_MASK; > - paddr_page = paddr & TARGET_PAGE_MASK; > + paddr_page = full->phys_addr & TARGET_PAGE_MASK; > > + prot = full->prot; > + asidx = cpu_asidx_from_attrs(cpu, full->attrs); > section = address_space_translate_for_iotlb(cpu, asidx, paddr_page, > - &xlat, &sz, attrs, &prot); > + &xlat, &sz, full->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); > + vaddr, full->phys_addr, prot, mmu_idx); > > address = vaddr_page; > - if (size < TARGET_PAGE_SIZE) { > + if (full->lg_page_size < TARGET_PAGE_BITS) { > /* Repeat the MMU check and TLB fill on every access. */ > address |= TLB_INVALID_MASK; > } > - if (attrs.byte_swap) { > + if (full->attrs.byte_swap) { > address |= TLB_BSWAP; > } > > @@ -1236,8 +1237,10 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, > * 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(). > */ > + desc->fulltlb[index] = *full; > desc->fulltlb[index].xlat_section = iotlb - vaddr_page; > - desc->fulltlb[index].attrs = attrs; > + desc->fulltlb[index].phys_addr = paddr_page; > + desc->fulltlb[index].prot = prot; > > /* Now calculate the new entry */ > tn.addend = addend - vaddr_page; > @@ -1272,9 +1275,21 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, > qemu_spin_unlock(&tlb->c.lock); > } > > -/* Add a new TLB entry, but without specifying the memory > - * transaction attributes to be used. > - */ > +void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, > + hwaddr paddr, MemTxAttrs attrs, int prot, > + int mmu_idx, target_ulong size) > +{ > + CPUTLBEntryFull full = { > + .phys_addr = paddr, > + .attrs = attrs, > + .prot = prot, > + .lg_page_size = ctz64(size) > + }; > + > + assert(is_power_of_2(size)); I'm hitting this assert with embedded RISC-V machines using PMP What's happening is that I'm going through riscv_cpu_tlb_fill() for a single stage machine mode (no MMU) lookup. This calls get_physical_address_pmp(), which then calls pmp_is_range_in_tlb() to determine the tlb size. That ends up in pmp_get_tlb_size() with pmp_sa: 0x10004abc pmp_ea: 0x1000ffff tlb_sa: 0x10004000 tlb_ea: 0x10004fff So we take the: if (pmp_sa >= tlb_sa && pmp_sa <= tlb_ea && pmp_ea >= tlb_ea) { return tlb_ea - pmp_sa + 1; } path and return 1348 (0x544) for the tlb size, which isn't a multiple of 2. Any ideas of what to do here? Alistair
On 10/10/22 22:01, Alistair Francis wrote: > On Wed, Oct 5, 2022 at 1:11 AM Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> Now that we have collected all of the page data into >> CPUTLBEntryFull, provide an interface to record that >> all in one go, instead of using 4 arguments. This interface >> allows CPUTLBEntryFull to be extended without having to >> change the number of arguments. >> >> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> >> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> >> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> include/exec/cpu-defs.h | 14 +++++++++++ >> include/exec/exec-all.h | 22 ++++++++++++++++++ >> accel/tcg/cputlb.c | 51 ++++++++++++++++++++++++++--------------- >> 3 files changed, 69 insertions(+), 18 deletions(-) >> >> diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h >> index f70f54d850..5e12cc1854 100644 >> --- a/include/exec/cpu-defs.h >> +++ b/include/exec/cpu-defs.h >> @@ -148,7 +148,21 @@ typedef struct CPUTLBEntryFull { >> * + the offset within the target MemoryRegion (otherwise) >> */ >> hwaddr xlat_section; >> + >> + /* >> + * @phys_addr contains the physical address in the address space >> + * given by cpu_asidx_from_attrs(cpu, @attrs). >> + */ >> + hwaddr phys_addr; >> + >> + /* @attrs contains the memory transaction attributes for the page. */ >> MemTxAttrs attrs; >> + >> + /* @prot contains the complete protections for the page. */ >> + uint8_t prot; >> + >> + /* @lg_page_size contains the log2 of the page size. */ >> + uint8_t lg_page_size; >> } CPUTLBEntryFull; >> >> /* >> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h >> index d255d69bc1..b1b920a713 100644 >> --- a/include/exec/exec-all.h >> +++ b/include/exec/exec-all.h >> @@ -257,6 +257,28 @@ void tlb_flush_range_by_mmuidx_all_cpus_synced(CPUState *cpu, >> uint16_t idxmap, >> unsigned bits); >> >> +/** >> + * tlb_set_page_full: >> + * @cpu: CPU context >> + * @mmu_idx: mmu index of the tlb to modify >> + * @vaddr: virtual address of the entry to add >> + * @full: the details of the tlb entry >> + * >> + * Add an entry to @cpu tlb index @mmu_idx. All of the fields of >> + * @full must be filled, except for xlat_section, and constitute >> + * the complete description of the translated page. >> + * >> + * This is generally called by the target tlb_fill function after >> + * having performed a successful page table walk to find the physical >> + * address and attributes for the translation. >> + * >> + * At most one entry for a given virtual address is permitted. Only a >> + * single TARGET_PAGE_SIZE region is mapped; @full->lg_page_size is only >> + * used by tlb_flush_page. >> + */ >> +void tlb_set_page_full(CPUState *cpu, int mmu_idx, target_ulong vaddr, >> + CPUTLBEntryFull *full); >> + >> /** >> * tlb_set_page_with_attrs: >> * @cpu: CPU to add this TLB entry for >> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c >> index e3ee4260bd..361078471b 100644 >> --- a/accel/tcg/cputlb.c >> +++ b/accel/tcg/cputlb.c >> @@ -1095,16 +1095,16 @@ static void tlb_add_large_page(CPUArchState *env, int mmu_idx, >> env_tlb(env)->d[mmu_idx].large_page_mask = lp_mask; >> } >> >> -/* Add a new TLB entry. At most one entry for a given virtual address >> +/* >> + * Add a new TLB entry. At most one entry for a given virtual address >> * is permitted. Only a single TARGET_PAGE_SIZE region is mapped, the >> * supplied size is only used by tlb_flush_page. >> * >> * Called from TCG-generated code, which is under an RCU read-side >> * critical section. >> */ >> -void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, >> - hwaddr paddr, MemTxAttrs attrs, int prot, >> - int mmu_idx, target_ulong size) >> +void tlb_set_page_full(CPUState *cpu, int mmu_idx, >> + target_ulong vaddr, CPUTLBEntryFull *full) >> { >> CPUArchState *env = cpu->env_ptr; >> CPUTLB *tlb = env_tlb(env); >> @@ -1117,35 +1117,36 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, >> CPUTLBEntry *te, tn; >> hwaddr iotlb, xlat, sz, paddr_page; >> target_ulong vaddr_page; >> - int asidx = cpu_asidx_from_attrs(cpu, attrs); >> - int wp_flags; >> + int asidx, wp_flags, prot; >> bool is_ram, is_romd; >> >> assert_cpu_is_self(cpu); >> >> - if (size <= TARGET_PAGE_SIZE) { >> + if (full->lg_page_size <= TARGET_PAGE_BITS) { >> sz = TARGET_PAGE_SIZE; >> } else { >> - tlb_add_large_page(env, mmu_idx, vaddr, size); >> - sz = size; >> + sz = (hwaddr)1 << full->lg_page_size; >> + tlb_add_large_page(env, mmu_idx, vaddr, sz); >> } >> vaddr_page = vaddr & TARGET_PAGE_MASK; >> - paddr_page = paddr & TARGET_PAGE_MASK; >> + paddr_page = full->phys_addr & TARGET_PAGE_MASK; >> >> + prot = full->prot; >> + asidx = cpu_asidx_from_attrs(cpu, full->attrs); >> section = address_space_translate_for_iotlb(cpu, asidx, paddr_page, >> - &xlat, &sz, attrs, &prot); >> + &xlat, &sz, full->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); >> + vaddr, full->phys_addr, prot, mmu_idx); >> >> address = vaddr_page; >> - if (size < TARGET_PAGE_SIZE) { >> + if (full->lg_page_size < TARGET_PAGE_BITS) { >> /* Repeat the MMU check and TLB fill on every access. */ >> address |= TLB_INVALID_MASK; >> } >> - if (attrs.byte_swap) { >> + if (full->attrs.byte_swap) { >> address |= TLB_BSWAP; >> } >> >> @@ -1236,8 +1237,10 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, >> * 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(). >> */ >> + desc->fulltlb[index] = *full; >> desc->fulltlb[index].xlat_section = iotlb - vaddr_page; >> - desc->fulltlb[index].attrs = attrs; >> + desc->fulltlb[index].phys_addr = paddr_page; >> + desc->fulltlb[index].prot = prot; >> >> /* Now calculate the new entry */ >> tn.addend = addend - vaddr_page; >> @@ -1272,9 +1275,21 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, >> qemu_spin_unlock(&tlb->c.lock); >> } >> >> -/* Add a new TLB entry, but without specifying the memory >> - * transaction attributes to be used. >> - */ >> +void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, >> + hwaddr paddr, MemTxAttrs attrs, int prot, >> + int mmu_idx, target_ulong size) >> +{ >> + CPUTLBEntryFull full = { >> + .phys_addr = paddr, >> + .attrs = attrs, >> + .prot = prot, >> + .lg_page_size = ctz64(size) >> + }; >> + >> + assert(is_power_of_2(size)); > > I'm hitting this assert with embedded RISC-V machines using PMP > > What's happening is that I'm going through riscv_cpu_tlb_fill() for a > single stage machine mode (no MMU) lookup. This calls > get_physical_address_pmp(), which then calls pmp_is_range_in_tlb() to > determine the tlb size. > > That ends up in pmp_get_tlb_size() with > > pmp_sa: 0x10004abc > pmp_ea: 0x1000ffff > tlb_sa: 0x10004000 > tlb_ea: 0x10004fff > > So we take the: > > if (pmp_sa >= tlb_sa && pmp_sa <= tlb_ea && pmp_ea >= tlb_ea) { > return tlb_ea - pmp_sa + 1; > } > > path and return 1348 (0x544) for the tlb size, which isn't a multiple of 2. > > Any ideas of what to do here? Ah. Well, this is less than TARGET_PAGE_SIZE, and any value less than that is not cached in the tlb (but is used for the one translation), so you can "round down" to 1. Which is what Arm does for all of its sub-page-size translations. For pmp, I would expect any value above TARGET_PAGE_SIZE, would just clamp to TARGET_PAGE_SIZE, since anything above T_P_S starts triggering large-page overflushing logic in the softmmu, which you don't need, since I expect there is no per-page tlb flush instruction that affects PMP, only changes to the system registers. r~
diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h index f70f54d850..5e12cc1854 100644 --- a/include/exec/cpu-defs.h +++ b/include/exec/cpu-defs.h @@ -148,7 +148,21 @@ typedef struct CPUTLBEntryFull { * + the offset within the target MemoryRegion (otherwise) */ hwaddr xlat_section; + + /* + * @phys_addr contains the physical address in the address space + * given by cpu_asidx_from_attrs(cpu, @attrs). + */ + hwaddr phys_addr; + + /* @attrs contains the memory transaction attributes for the page. */ MemTxAttrs attrs; + + /* @prot contains the complete protections for the page. */ + uint8_t prot; + + /* @lg_page_size contains the log2 of the page size. */ + uint8_t lg_page_size; } CPUTLBEntryFull; /* diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h index d255d69bc1..b1b920a713 100644 --- a/include/exec/exec-all.h +++ b/include/exec/exec-all.h @@ -257,6 +257,28 @@ void tlb_flush_range_by_mmuidx_all_cpus_synced(CPUState *cpu, uint16_t idxmap, unsigned bits); +/** + * tlb_set_page_full: + * @cpu: CPU context + * @mmu_idx: mmu index of the tlb to modify + * @vaddr: virtual address of the entry to add + * @full: the details of the tlb entry + * + * Add an entry to @cpu tlb index @mmu_idx. All of the fields of + * @full must be filled, except for xlat_section, and constitute + * the complete description of the translated page. + * + * This is generally called by the target tlb_fill function after + * having performed a successful page table walk to find the physical + * address and attributes for the translation. + * + * At most one entry for a given virtual address is permitted. Only a + * single TARGET_PAGE_SIZE region is mapped; @full->lg_page_size is only + * used by tlb_flush_page. + */ +void tlb_set_page_full(CPUState *cpu, int mmu_idx, target_ulong vaddr, + CPUTLBEntryFull *full); + /** * tlb_set_page_with_attrs: * @cpu: CPU to add this TLB entry for diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c index e3ee4260bd..361078471b 100644 --- a/accel/tcg/cputlb.c +++ b/accel/tcg/cputlb.c @@ -1095,16 +1095,16 @@ static void tlb_add_large_page(CPUArchState *env, int mmu_idx, env_tlb(env)->d[mmu_idx].large_page_mask = lp_mask; } -/* Add a new TLB entry. At most one entry for a given virtual address +/* + * Add a new TLB entry. At most one entry for a given virtual address * is permitted. Only a single TARGET_PAGE_SIZE region is mapped, the * supplied size is only used by tlb_flush_page. * * Called from TCG-generated code, which is under an RCU read-side * critical section. */ -void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, - hwaddr paddr, MemTxAttrs attrs, int prot, - int mmu_idx, target_ulong size) +void tlb_set_page_full(CPUState *cpu, int mmu_idx, + target_ulong vaddr, CPUTLBEntryFull *full) { CPUArchState *env = cpu->env_ptr; CPUTLB *tlb = env_tlb(env); @@ -1117,35 +1117,36 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, CPUTLBEntry *te, tn; hwaddr iotlb, xlat, sz, paddr_page; target_ulong vaddr_page; - int asidx = cpu_asidx_from_attrs(cpu, attrs); - int wp_flags; + int asidx, wp_flags, prot; bool is_ram, is_romd; assert_cpu_is_self(cpu); - if (size <= TARGET_PAGE_SIZE) { + if (full->lg_page_size <= TARGET_PAGE_BITS) { sz = TARGET_PAGE_SIZE; } else { - tlb_add_large_page(env, mmu_idx, vaddr, size); - sz = size; + sz = (hwaddr)1 << full->lg_page_size; + tlb_add_large_page(env, mmu_idx, vaddr, sz); } vaddr_page = vaddr & TARGET_PAGE_MASK; - paddr_page = paddr & TARGET_PAGE_MASK; + paddr_page = full->phys_addr & TARGET_PAGE_MASK; + prot = full->prot; + asidx = cpu_asidx_from_attrs(cpu, full->attrs); section = address_space_translate_for_iotlb(cpu, asidx, paddr_page, - &xlat, &sz, attrs, &prot); + &xlat, &sz, full->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); + vaddr, full->phys_addr, prot, mmu_idx); address = vaddr_page; - if (size < TARGET_PAGE_SIZE) { + if (full->lg_page_size < TARGET_PAGE_BITS) { /* Repeat the MMU check and TLB fill on every access. */ address |= TLB_INVALID_MASK; } - if (attrs.byte_swap) { + if (full->attrs.byte_swap) { address |= TLB_BSWAP; } @@ -1236,8 +1237,10 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, * 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(). */ + desc->fulltlb[index] = *full; desc->fulltlb[index].xlat_section = iotlb - vaddr_page; - desc->fulltlb[index].attrs = attrs; + desc->fulltlb[index].phys_addr = paddr_page; + desc->fulltlb[index].prot = prot; /* Now calculate the new entry */ tn.addend = addend - vaddr_page; @@ -1272,9 +1275,21 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, qemu_spin_unlock(&tlb->c.lock); } -/* Add a new TLB entry, but without specifying the memory - * transaction attributes to be used. - */ +void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, + hwaddr paddr, MemTxAttrs attrs, int prot, + int mmu_idx, target_ulong size) +{ + CPUTLBEntryFull full = { + .phys_addr = paddr, + .attrs = attrs, + .prot = prot, + .lg_page_size = ctz64(size) + }; + + assert(is_power_of_2(size)); + tlb_set_page_full(cpu, mmu_idx, vaddr, &full); +} + void tlb_set_page(CPUState *cpu, target_ulong vaddr, hwaddr paddr, int prot, int mmu_idx, target_ulong size)