| Message ID | 20190403034358.21999-24-richard.henderson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tcg: Add CPUClass::tlb_fill | expand |
On Wed, 3 Apr 2019 at 05:00, Richard Henderson <richard.henderson@linaro.org> wrote: > > Cc: Max Filippov <jcmvbkbc@gmail.com> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/xtensa/cpu.h | 5 +-- > target/xtensa/cpu.c | 5 ++- > target/xtensa/helper.c | 74 +++++++++++++++++++++--------------------- > 3 files changed, 42 insertions(+), 42 deletions(-) > -#ifdef CONFIG_USER_ONLY > - > -int xtensa_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size, int rw, > - int mmu_idx) > +bool xtensa_cpu_tlb_fill(CPUState *cs, vaddr address, int size, > + MMUAccessType access_type, int mmu_idx, > + bool probe, uintptr_t retaddr) > { > XtensaCPU *cpu = XTENSA_CPU(cs); > CPUXtensaState *env = &cpu->env; > + target_ulong vaddr = address; > + int ret; > > - qemu_log_mask(CPU_LOG_INT, > - "%s: rw = %d, address = 0x%08" VADDR_PRIx ", size = %d\n", > - __func__, rw, address, size); > - env->sregs[EXCVADDR] = address; > - env->sregs[EXCCAUSE] = rw ? STORE_PROHIBITED_CAUSE : LOAD_PROHIBITED_CAUSE; > - cs->exception_index = EXC_USER; > - return 1; Previously we set exception_index to EXC_USER... > +#ifdef CONFIG_USER_ONLY > + ret = (access_type == MMU_DATA_STORE ? > + STORE_PROHIBITED_CAUSE : LOAD_PROHIBITED_CAUSE); > +#else > + uint32_t paddr; > + uint32_t page_size; > + unsigned access; > + > + ret = xtensa_get_physical_addr(env, true, vaddr, access_type, mmu_idx, > + &paddr, &page_size, &access); > + > + qemu_log_mask(CPU_LOG_MMU, "%s(%08x, %d, %d) -> %08x, ret = %d\n", > + __func__, vaddr, access_type, mmu_idx, paddr, ret); > + > + if (ret == 0) { > + tlb_set_page(cs, vaddr & TARGET_PAGE_MASK, paddr & TARGET_PAGE_MASK, > + access, mmu_idx, page_size); > + return true; > + } > + if (probe) { > + return false; > + } > +#endif > + > + cpu_restore_state(cs, retaddr, true); > + HELPER(exception_cause_vaddr)(env, env->pc, ret, vaddr); ...but now we'll set it to whatever exception_cause_vaddr does, which is something more complicated based on the state of env->sregs[PS]. We'll also end up setting env->sregs[PS] bits and env->pc, which the old code did not. (In particular since we set the PS_EXCM bit, the second time we take an exception won't we then end up setting exception_index to EXC_DOUBLE, not EXC_USER ?) thanks -- PMM
On Tue, Apr 30, 2019 at 3:11 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Wed, 3 Apr 2019 at 05:00, Richard Henderson > <richard.henderson@linaro.org> wrote: > > > > Cc: Max Filippov <jcmvbkbc@gmail.com> > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > > --- > > target/xtensa/cpu.h | 5 +-- > > target/xtensa/cpu.c | 5 ++- > > target/xtensa/helper.c | 74 +++++++++++++++++++++--------------------- > > 3 files changed, 42 insertions(+), 42 deletions(-) > > > -#ifdef CONFIG_USER_ONLY > > - > > -int xtensa_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size, int rw, > > - int mmu_idx) > > +bool xtensa_cpu_tlb_fill(CPUState *cs, vaddr address, int size, > > + MMUAccessType access_type, int mmu_idx, > > + bool probe, uintptr_t retaddr) > > { > > XtensaCPU *cpu = XTENSA_CPU(cs); > > CPUXtensaState *env = &cpu->env; > > + target_ulong vaddr = address; > > + int ret; > > > > - qemu_log_mask(CPU_LOG_INT, > > - "%s: rw = %d, address = 0x%08" VADDR_PRIx ", size = %d\n", > > - __func__, rw, address, size); > > - env->sregs[EXCVADDR] = address; > > - env->sregs[EXCCAUSE] = rw ? STORE_PROHIBITED_CAUSE : LOAD_PROHIBITED_CAUSE; > > - cs->exception_index = EXC_USER; > > - return 1; > > Previously we set exception_index to EXC_USER... > > > +#ifdef CONFIG_USER_ONLY > > + ret = (access_type == MMU_DATA_STORE ? > > + STORE_PROHIBITED_CAUSE : LOAD_PROHIBITED_CAUSE); > > +#else > > + uint32_t paddr; > > + uint32_t page_size; > > + unsigned access; > > + > > + ret = xtensa_get_physical_addr(env, true, vaddr, access_type, mmu_idx, > > + &paddr, &page_size, &access); > > + > > + qemu_log_mask(CPU_LOG_MMU, "%s(%08x, %d, %d) -> %08x, ret = %d\n", > > + __func__, vaddr, access_type, mmu_idx, paddr, ret); > > + > > + if (ret == 0) { > > + tlb_set_page(cs, vaddr & TARGET_PAGE_MASK, paddr & TARGET_PAGE_MASK, > > + access, mmu_idx, page_size); > > + return true; > > + } > > + if (probe) { > > + return false; > > + } > > +#endif > > + > > + cpu_restore_state(cs, retaddr, true); > > + HELPER(exception_cause_vaddr)(env, env->pc, ret, vaddr); > > ...but now we'll set it to whatever exception_cause_vaddr does, > which is something more complicated based on the state of > env->sregs[PS]. > > We'll also end up setting env->sregs[PS] bits and env->pc, which > the old code did not. (In particular since we set the PS_EXCM bit, > the second time we take an exception won't we then end up > setting exception_index to EXC_DOUBLE, not EXC_USER ?) I guess it doesn't matter, because linux-user userspace never handles exceptions. PS, PC and similar must be fixed up by the linux-user exception handler. But I haven't tested it. Richard, is there a branch with this series available somewhere?
On 4/30/19 10:32 AM, Max Filippov wrote: > On Tue, Apr 30, 2019 at 3:11 AM Peter Maydell <peter.maydell@linaro.org> wrote: >> >> On Wed, 3 Apr 2019 at 05:00, Richard Henderson >> <richard.henderson@linaro.org> wrote: >>> >>> Cc: Max Filippov <jcmvbkbc@gmail.com> >>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >>> --- >>> target/xtensa/cpu.h | 5 +-- >>> target/xtensa/cpu.c | 5 ++- >>> target/xtensa/helper.c | 74 +++++++++++++++++++++--------------------- >>> 3 files changed, 42 insertions(+), 42 deletions(-) >> >>> -#ifdef CONFIG_USER_ONLY >>> - >>> -int xtensa_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size, int rw, >>> - int mmu_idx) >>> +bool xtensa_cpu_tlb_fill(CPUState *cs, vaddr address, int size, >>> + MMUAccessType access_type, int mmu_idx, >>> + bool probe, uintptr_t retaddr) >>> { >>> XtensaCPU *cpu = XTENSA_CPU(cs); >>> CPUXtensaState *env = &cpu->env; >>> + target_ulong vaddr = address; >>> + int ret; >>> >>> - qemu_log_mask(CPU_LOG_INT, >>> - "%s: rw = %d, address = 0x%08" VADDR_PRIx ", size = %d\n", >>> - __func__, rw, address, size); >>> - env->sregs[EXCVADDR] = address; >>> - env->sregs[EXCCAUSE] = rw ? STORE_PROHIBITED_CAUSE : LOAD_PROHIBITED_CAUSE; >>> - cs->exception_index = EXC_USER; >>> - return 1; >> >> Previously we set exception_index to EXC_USER... >> >>> +#ifdef CONFIG_USER_ONLY >>> + ret = (access_type == MMU_DATA_STORE ? >>> + STORE_PROHIBITED_CAUSE : LOAD_PROHIBITED_CAUSE); >>> +#else >>> + uint32_t paddr; >>> + uint32_t page_size; >>> + unsigned access; >>> + >>> + ret = xtensa_get_physical_addr(env, true, vaddr, access_type, mmu_idx, >>> + &paddr, &page_size, &access); >>> + >>> + qemu_log_mask(CPU_LOG_MMU, "%s(%08x, %d, %d) -> %08x, ret = %d\n", >>> + __func__, vaddr, access_type, mmu_idx, paddr, ret); >>> + >>> + if (ret == 0) { >>> + tlb_set_page(cs, vaddr & TARGET_PAGE_MASK, paddr & TARGET_PAGE_MASK, >>> + access, mmu_idx, page_size); >>> + return true; >>> + } >>> + if (probe) { >>> + return false; >>> + } >>> +#endif >>> + >>> + cpu_restore_state(cs, retaddr, true); >>> + HELPER(exception_cause_vaddr)(env, env->pc, ret, vaddr); >> >> ...but now we'll set it to whatever exception_cause_vaddr does, >> which is something more complicated based on the state of >> env->sregs[PS]. >> >> We'll also end up setting env->sregs[PS] bits and env->pc, which >> the old code did not. (In particular since we set the PS_EXCM bit, >> the second time we take an exception won't we then end up >> setting exception_index to EXC_DOUBLE, not EXC_USER ?) > > I guess it doesn't matter, because linux-user userspace never handles > exceptions. PS, PC and similar must be fixed up by the linux-user > exception handler. But I haven't tested it. It does handle exceptions, in linux-user/xtensa/cpu_loop.c. And Peter's right that I should have kept EXC_USER. > Richard, is there a branch with this series available somewhere? https://github.com/rth7680/qemu/tree/tcg-tlb-fill r~
On Tue, Apr 30, 2019 at 10:44 AM Richard Henderson <richard.henderson@linaro.org> wrote: > On 4/30/19 10:32 AM, Max Filippov wrote: > > On Tue, Apr 30, 2019 at 3:11 AM Peter Maydell <peter.maydell@linaro.org> wrote: > >> ...but now we'll set it to whatever exception_cause_vaddr does, > >> which is something more complicated based on the state of > >> env->sregs[PS]. > >> > >> We'll also end up setting env->sregs[PS] bits and env->pc, which > >> the old code did not. (In particular since we set the PS_EXCM bit, > >> the second time we take an exception won't we then end up > >> setting exception_index to EXC_DOUBLE, not EXC_USER ?) > > > > I guess it doesn't matter, because linux-user userspace never handles > > exceptions. PS, PC and similar must be fixed up by the linux-user > > exception handler. But I haven't tested it. > > It does handle exceptions, in linux-user/xtensa/cpu_loop.c. > And Peter's right that I should have kept EXC_USER. PC must also either be preserved or restored from the EPC1 in the cpu_loop for the SYSCALL_CAUSE. > > Richard, is there a branch with this series available somewhere? > https://github.com/rth7680/qemu/tree/tcg-tlb-fill Thanks, I'll try it.
On Tue, Apr 30, 2019 at 11:14 AM Max Filippov <jcmvbkbc@gmail.com> wrote: > On Tue, Apr 30, 2019 at 10:44 AM Richard Henderson > > And Peter's right that I should have kept EXC_USER. It appears to work as is: the EXC_USER is set up by the exception_cause helper because there's always PS_U in the PS, PS_EXCM is cleared in the cpu_loop and the current PC is preserved by the xtensa_cpu_tlb_fill. I'll play with it some more...
On Tue, Apr 30, 2019 at 2:07 PM Max Filippov <jcmvbkbc@gmail.com> wrote: > On Tue, Apr 30, 2019 at 11:14 AM Max Filippov <jcmvbkbc@gmail.com> wrote: > > On Tue, Apr 30, 2019 at 10:44 AM Richard Henderson > > > And Peter's right that I should have kept EXC_USER. > > It appears to work as is: the EXC_USER is set up by the > exception_cause helper because there's always PS_U in > the PS, PS_EXCM is cleared in the cpu_loop and the > current PC is preserved by the xtensa_cpu_tlb_fill. > I'll play with it some more... I've run gcc/uclibc testsuites for xtensa-linux with this series as is, got no new regressions.
diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h index 4d8152682f..8ac6f8eeca 100644 --- a/target/xtensa/cpu.h +++ b/target/xtensa/cpu.h @@ -552,8 +552,9 @@ static inline XtensaCPU *xtensa_env_get_cpu(const CPUXtensaState *env) #define ENV_OFFSET offsetof(XtensaCPU, env) -int xtensa_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int rw, int size, - int mmu_idx); +bool xtensa_cpu_tlb_fill(CPUState *cs, vaddr address, int size, + MMUAccessType access_type, int mmu_idx, + bool probe, uintptr_t retaddr); void xtensa_cpu_do_interrupt(CPUState *cpu); bool xtensa_cpu_exec_interrupt(CPUState *cpu, int interrupt_request); void xtensa_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr, vaddr addr, diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c index a54dbe4260..da1236377e 100644 --- a/target/xtensa/cpu.c +++ b/target/xtensa/cpu.c @@ -181,9 +181,8 @@ static void xtensa_cpu_class_init(ObjectClass *oc, void *data) cc->gdb_read_register = xtensa_cpu_gdb_read_register; cc->gdb_write_register = xtensa_cpu_gdb_write_register; cc->gdb_stop_before_watchpoint = true; -#ifdef CONFIG_USER_ONLY - cc->handle_mmu_fault = xtensa_cpu_handle_mmu_fault; -#else + cc->tlb_fill = xtensa_cpu_tlb_fill; +#ifndef CONFIG_USER_ONLY cc->do_unaligned_access = xtensa_cpu_do_unaligned_access; cc->get_phys_page_debug = xtensa_cpu_get_phys_page_debug; cc->do_transaction_failed = xtensa_cpu_do_transaction_failed; diff --git a/target/xtensa/helper.c b/target/xtensa/helper.c index f4867a9b56..3dcab54fbf 100644 --- a/target/xtensa/helper.c +++ b/target/xtensa/helper.c @@ -237,24 +237,49 @@ void xtensa_cpu_list(FILE *f, fprintf_function cpu_fprintf) } } -#ifdef CONFIG_USER_ONLY - -int xtensa_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size, int rw, - int mmu_idx) +bool xtensa_cpu_tlb_fill(CPUState *cs, vaddr address, int size, + MMUAccessType access_type, int mmu_idx, + bool probe, uintptr_t retaddr) { XtensaCPU *cpu = XTENSA_CPU(cs); CPUXtensaState *env = &cpu->env; + target_ulong vaddr = address; + int ret; - qemu_log_mask(CPU_LOG_INT, - "%s: rw = %d, address = 0x%08" VADDR_PRIx ", size = %d\n", - __func__, rw, address, size); - env->sregs[EXCVADDR] = address; - env->sregs[EXCCAUSE] = rw ? STORE_PROHIBITED_CAUSE : LOAD_PROHIBITED_CAUSE; - cs->exception_index = EXC_USER; - return 1; +#ifdef CONFIG_USER_ONLY + ret = (access_type == MMU_DATA_STORE ? + STORE_PROHIBITED_CAUSE : LOAD_PROHIBITED_CAUSE); +#else + uint32_t paddr; + uint32_t page_size; + unsigned access; + + ret = xtensa_get_physical_addr(env, true, vaddr, access_type, mmu_idx, + &paddr, &page_size, &access); + + qemu_log_mask(CPU_LOG_MMU, "%s(%08x, %d, %d) -> %08x, ret = %d\n", + __func__, vaddr, access_type, mmu_idx, paddr, ret); + + if (ret == 0) { + tlb_set_page(cs, vaddr & TARGET_PAGE_MASK, paddr & TARGET_PAGE_MASK, + access, mmu_idx, page_size); + return true; + } + if (probe) { + return false; + } +#endif + + cpu_restore_state(cs, retaddr, true); + HELPER(exception_cause_vaddr)(env, env->pc, ret, vaddr); } -#else +#ifndef CONFIG_USER_ONLY +void tlb_fill(CPUState *cs, target_ulong vaddr, int size, + MMUAccessType access_type, int mmu_idx, uintptr_t retaddr) +{ + xtensa_cpu_tlb_fill(cs, vaddr, size, access_type, mmu_idx, false, retaddr); +} void xtensa_cpu_do_unaligned_access(CPUState *cs, vaddr addr, MMUAccessType access_type, @@ -272,31 +297,6 @@ void xtensa_cpu_do_unaligned_access(CPUState *cs, } } -void tlb_fill(CPUState *cs, target_ulong vaddr, int size, - MMUAccessType access_type, int mmu_idx, uintptr_t retaddr) -{ - XtensaCPU *cpu = XTENSA_CPU(cs); - CPUXtensaState *env = &cpu->env; - uint32_t paddr; - uint32_t page_size; - unsigned access; - int ret = xtensa_get_physical_addr(env, true, vaddr, access_type, mmu_idx, - &paddr, &page_size, &access); - - qemu_log_mask(CPU_LOG_MMU, "%s(%08x, %d, %d) -> %08x, ret = %d\n", - __func__, vaddr, access_type, mmu_idx, paddr, ret); - - if (ret == 0) { - tlb_set_page(cs, - vaddr & TARGET_PAGE_MASK, - paddr & TARGET_PAGE_MASK, - access, mmu_idx, page_size); - } else { - cpu_restore_state(cs, retaddr, true); - HELPER(exception_cause_vaddr)(env, env->pc, ret, vaddr); - } -} - void xtensa_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr, vaddr addr, unsigned size, MMUAccessType access_type, int mmu_idx, MemTxAttrs attrs,
Cc: Max Filippov <jcmvbkbc@gmail.com> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/xtensa/cpu.h | 5 +-- target/xtensa/cpu.c | 5 ++- target/xtensa/helper.c | 74 +++++++++++++++++++++--------------------- 3 files changed, 42 insertions(+), 42 deletions(-)