On Sat, 1 Oct 2022 at 17:38, Richard Henderson <richard.hender...@linaro.org> wrote: > > Add a field to TARGET_PAGE_ENTRY_EXTRA to hold the guarded bit. > In is_guarded_page, use probe_access_full instead of just guessing > that the tlb entry is still present. Also handles the FIXME about > executing from device memory. > > Signed-off-by: Richard Henderson <richard.hender...@linaro.org> > --- > target/arm/cpu-param.h | 8 ++++---- > target/arm/cpu.h | 13 ------------- > target/arm/internals.h | 1 + > target/arm/ptw.c | 7 ++++--- > target/arm/translate-a64.c | 22 ++++++++-------------- > 5 files changed, 17 insertions(+), 34 deletions(-) > > diff --git a/target/arm/cpu-param.h b/target/arm/cpu-param.h > index 118ca0e5c0..689a9645dc 100644 > --- a/target/arm/cpu-param.h > +++ b/target/arm/cpu-param.h > @@ -32,12 +32,12 @@ > # define TARGET_PAGE_BITS_MIN 10 > > /* > - * Cache the attrs and sharability fields from the page table entry. > + * Cache the attrs, sharability, and gp fields from the page table entry. > */ > # define TARGET_PAGE_ENTRY_EXTRA \ > - uint8_t pte_attrs; \ > - uint8_t shareability; > - > + uint8_t pte_attrs; \ > + uint8_t shareability; \ > + bool guarded;
I notice this now brings this very close to just having an ARMCacheAttrs struct in it (in fact it's going to be one byte bigger than the ARMCachettrs). But it's probably better to keep them separate since we care a lot more about keeping the TLB entry small I suppose. > diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c > index 5b67375f4e..22802d1d2f 100644 > --- a/target/arm/translate-a64.c > +++ b/target/arm/translate-a64.c > @@ -14601,22 +14601,16 @@ static bool is_guarded_page(CPUARMState *env, > DisasContext *s) > #ifdef CONFIG_USER_ONLY > return page_get_flags(addr) & PAGE_BTI; > #else > + CPUTLBEntryFull *full; > + void *host; > int mmu_idx = arm_to_core_mmu_idx(s->mmu_idx); > - unsigned int index = tlb_index(env, mmu_idx, addr); > - CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr); > + int flags; > > - /* > - * We test this immediately after reading an insn, which means > - * that any normal page must be in the TLB. The only exception > - * would be for executing from flash or device memory, which > - * does not retain the TLB entry. > - * > - * FIXME: Assume false for those, for now. We could use > - * arm_cpu_get_phys_page_attrs_debug to re-read the page > - * table entry even for that case. > - */ I think we should keep at least some of this comment: the part about the reason we can assert that probe_access_full() doesn't return TLB_INVALID being that we tested immediately after the insn read is still true, right? > - return (tlb_hit(entry->addr_code, addr) && > - arm_tlb_bti_gp(&env_tlb(env)->d[mmu_idx].fulltlb[index].attrs)); > + flags = probe_access_full(env, addr, MMU_INST_FETCH, mmu_idx, > + false, &host, &full, 0); > + assert(!(flags & TLB_INVALID_MASK)); > + > + return full->guarded; > #endif > } Otherwise Reviewed-by: Peter Maydell <peter.mayd...@linaro.org> thanks -- PMM
