| Message ID | 20190510185438.29533-4-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | tcg: Add CPUClass::tlb_fill | expand |
On May 10, 2019 8:57 PM, "Richard Henderson" <richard.henderson@linaro.org> wrote: > Please change the title to 'target/mips: Switch to using mips_cpu_tlb_fill()', or something along that line. Also, the reason for changing the field access_type to mips_access type should be explained in the commit message. This commit message is generally poor, as it explains relatively unimportant logging issue, while not explaining the core of the change. Thanks, Aleksandar > Note that env->active_tc.PC is removed from the qemu_log as that value > is garbage. The PC isn't recovered until cpu_restore_state, called from > cpu_loop_exit_restore, called from do_raise_exception_err. > > Cc: Aleksandar Markovic <amarkovic@wavecomp.com> > Cc: Aleksandar Rikalo <arikalo@wavecomp.com> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/mips/internal.h | 5 +++-- > target/mips/cpu.c | 5 ++--- > target/mips/helper.c | 45 ++++++++++++++++++++++------------------- > target/mips/op_helper.c | 15 -------------- > 4 files changed, 29 insertions(+), 41 deletions(-) > > diff --git a/target/mips/internal.h b/target/mips/internal.h > index 286e3888ab..b2b41a51ab 100644 > --- a/target/mips/internal.h > +++ b/target/mips/internal.h > @@ -202,8 +202,9 @@ void cpu_mips_start_count(CPUMIPSState *env); > void cpu_mips_stop_count(CPUMIPSState *env); > > /* helper.c */ > -int mips_cpu_handle_mmu_fault(CPUState *cpu, vaddr address, int size, int rw, > - int mmu_idx); > +bool mips_cpu_tlb_fill(CPUState *cs, vaddr address, int size, > + MMUAccessType access_type, int mmu_idx, > + bool probe, uintptr_t retaddr); > > /* op_helper.c */ > uint32_t float_class_s(uint32_t arg, float_status *fst); > diff --git a/target/mips/cpu.c b/target/mips/cpu.c > index e217fb3e36..a33058609a 100644 > --- a/target/mips/cpu.c > +++ b/target/mips/cpu.c > @@ -197,9 +197,7 @@ static void mips_cpu_class_init(ObjectClass *c, void *data) > cc->synchronize_from_tb = mips_cpu_synchronize_from_tb; > cc->gdb_read_register = mips_cpu_gdb_read_register; > cc->gdb_write_register = mips_cpu_gdb_write_register; > -#ifdef CONFIG_USER_ONLY > - cc->handle_mmu_fault = mips_cpu_handle_mmu_fault; > -#else > +#ifndef CONFIG_USER_ONLY > cc->do_unassigned_access = mips_cpu_unassigned_access; > cc->do_unaligned_access = mips_cpu_do_unaligned_access; > cc->get_phys_page_debug = mips_cpu_get_phys_page_debug; > @@ -208,6 +206,7 @@ static void mips_cpu_class_init(ObjectClass *c, void *data) > cc->disas_set_info = mips_cpu_disas_set_info; > #ifdef CONFIG_TCG > cc->tcg_initialize = mips_tcg_init; > + cc->tlb_fill = mips_cpu_tlb_fill; > #endif > > cc->gdb_num_core_regs = 73; > diff --git a/target/mips/helper.c b/target/mips/helper.c > index 86e622efb8..3a4917ce7b 100644 > --- a/target/mips/helper.c > +++ b/target/mips/helper.c > @@ -874,31 +874,25 @@ refill: > #endif > #endif > > -int mips_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size, int rw, > - int mmu_idx) > +bool mips_cpu_tlb_fill(CPUState *cs, vaddr address, int size, > + MMUAccessType access_type, int mmu_idx, > + bool probe, uintptr_t retaddr) > { > MIPSCPU *cpu = MIPS_CPU(cs); > CPUMIPSState *env = &cpu->env; > #if !defined(CONFIG_USER_ONLY) > hwaddr physical; > int prot; > - int access_type; > + int mips_access_type; > #endif > int ret = TLBRET_BADADDR; > > -#if 0 > - log_cpu_state(cs, 0); > -#endif > - qemu_log_mask(CPU_LOG_MMU, > - "%s pc " TARGET_FMT_lx " ad %" VADDR_PRIx " rw %d mmu_idx %d\n", > - __func__, env->active_tc.PC, address, rw, mmu_idx); > - > /* data access */ > #if !defined(CONFIG_USER_ONLY) > /* XXX: put correct access by using cpu_restore_state() correctly */ > - access_type = ACCESS_INT; > - ret = get_physical_address(env, &physical, &prot, > - address, rw, access_type, mmu_idx); > + mips_access_type = ACCESS_INT; > + ret = get_physical_address(env, &physical, &prot, address, > + access_type, mips_access_type, mmu_idx); > switch (ret) { > case TLBRET_MATCH: > qemu_log_mask(CPU_LOG_MMU, > @@ -915,7 +909,7 @@ int mips_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size, int rw, > tlb_set_page(cs, address & TARGET_PAGE_MASK, > physical & TARGET_PAGE_MASK, prot | PAGE_EXEC, > mmu_idx, TARGET_PAGE_SIZE); > - return 0; > + return true; > } > #if !defined(TARGET_MIPS64) > if ((ret == TLBRET_NOMATCH) && (env->tlb->nb_tlb > 1)) { > @@ -926,27 +920,36 @@ int mips_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size, int rw, > int mode = (env->hflags & MIPS_HFLAG_KSU); > bool ret_walker; > env->hflags &= ~MIPS_HFLAG_KSU; > - ret_walker = page_table_walk_refill(env, address, rw, mmu_idx); > + ret_walker = page_table_walk_refill(env, address, access_type, mmu_idx); > env->hflags |= mode; > if (ret_walker) { > - ret = get_physical_address(env, &physical, &prot, > - address, rw, access_type, mmu_idx); > + ret = get_physical_address(env, &physical, &prot, address, > + access_type, mips_access_type, mmu_idx);¿ > if (ret == TLBRET_MATCH) { > tlb_set_page(cs, address & TARGET_PAGE_MASK, > physical & TARGET_PAGE_MASK, prot | PAGE_EXEC, > mmu_idx, TARGET_PAGE_SIZE); > - return 0; > + return true; > } > } > } > #endif > + if (probe) { > + return false; > + } > #endif > > - raise_mmu_exception(env, address, rw, ret); > - return 1; > + raise_mmu_exception(env, address, access_type, ret); > + do_raise_exception_err(env, cs->exception_index, env->error_code, retaddr); > +} > + > +#ifndef CONFIG_USER_ONLY > +void tlb_fill(CPUState *cs, target_ulong addr, int size, > + MMUAccessType access_type, int mmu_idx, uintptr_t retaddr) > +{ > + mips_cpu_tlb_fill(cs, addr, size, access_type, mmu_idx, false, retaddr); > } > > -#if !defined(CONFIG_USER_ONLY) > hwaddr cpu_mips_translate_address(CPUMIPSState *env, target_ulong address, int rw) > { > hwaddr physical; > diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c > index 0f272a5b93..6d86912958 100644 > --- a/target/mips/op_helper.c > +++ b/target/mips/op_helper.c > @@ -2669,21 +2669,6 @@ void mips_cpu_do_unaligned_access(CPUState *cs, vaddr addr, > do_raise_exception_err(env, excp, error_code, retaddr); > } > > -void tlb_fill(CPUState *cs, target_ulong addr, int size, > - MMUAccessType access_type, int mmu_idx, uintptr_t retaddr) > -{ > - int ret; > - > - ret = mips_cpu_handle_mmu_fault(cs, addr, size, access_type, mmu_idx); > - if (ret) { > - MIPSCPU *cpu = MIPS_CPU(cs); > - CPUMIPSState *env = &cpu->env; > - > - do_raise_exception_err(env, cs->exception_index, > - env->error_code, retaddr); > - } > -} > - > void mips_cpu_unassigned_access(CPUState *cs, hwaddr addr, > bool is_write, bool is_exec, int unused, > unsigned size) > -- > 2.17.1 > >
On Sat, 11 May 2019 at 14:43, Aleksandar Markovic
<aleksandar.m.mail@gmail.com> wrote:
> This commit message is generally poor, as it explains relatively unimportant logging issue, while not explaining the core of the change.
I think the assumption with this sort of "refactor to change
all instances of an API" change is that you will go and
look at the commit message (or cover letter for the patch
series) that introduces the new API; there's no great need
to repeat the same justification for every commit that
applies the refactoring to each of our dozen or more
guest front-ends.
I'm not sure that wordsmithing a commit message really
justifies rerolling this pull request at this point.
thanks
-- PMM
Aleksandar Markovic <aleksandar.m.mail@gmail.com> writes: > On May 10, 2019 8:57 PM, "Richard Henderson" <richard.henderson@linaro.org> > wrote: >> > > Please change the title to 'target/mips: Switch to using > mips_cpu_tlb_fill()', or something along that line. It does seem a little redundant as "target/mips:" already marks it as a mips specific change and viewing the log you can see a series of architectures being converted to a new API. > Also, the reason for changing the field access_type to mips_access type > should be explained in the commit message. ok > This commit message is generally poor, as it explains relatively > unimportant logging issue, while not explaining the core of the > change. Surely the core of the change is explained in the main patches that introduce the new API? I think it would be redundant to repeat that for every individual architecture touched. It's a shame it's hard to explicitly reference a patch in the same series as the commit hashes are not yet permanent. At least when we fix things referring to the short hash of the original commit is fairly easy. Generally for an architecture conversion I want to know what might be different from other architectures converted. If it is a broadly mechanical change it doesn't need to be too detailed. > > Thanks, > Aleksandar > >> Note that env->active_tc.PC is removed from the qemu_log as that value >> is garbage. The PC isn't recovered until cpu_restore_state, called from >> cpu_loop_exit_restore, called from do_raise_exception_err. >> >> Cc: Aleksandar Markovic <amarkovic@wavecomp.com> >> Cc: Aleksandar Rikalo <arikalo@wavecomp.com> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> target/mips/internal.h | 5 +++-- >> target/mips/cpu.c | 5 ++--- >> target/mips/helper.c | 45 ++++++++++++++++++++++------------------- >> target/mips/op_helper.c | 15 -------------- >> 4 files changed, 29 insertions(+), 41 deletions(-) >> >> diff --git a/target/mips/internal.h b/target/mips/internal.h >> index 286e3888ab..b2b41a51ab 100644 >> --- a/target/mips/internal.h >> +++ b/target/mips/internal.h >> @@ -202,8 +202,9 @@ void cpu_mips_start_count(CPUMIPSState *env); >> void cpu_mips_stop_count(CPUMIPSState *env); >> >> /* helper.c */ >> -int mips_cpu_handle_mmu_fault(CPUState *cpu, vaddr address, int size, > int rw, >> - int mmu_idx); >> +bool mips_cpu_tlb_fill(CPUState *cs, vaddr address, int size, >> + MMUAccessType access_type, int mmu_idx, >> + bool probe, uintptr_t retaddr); >> >> /* op_helper.c */ >> uint32_t float_class_s(uint32_t arg, float_status *fst); >> diff --git a/target/mips/cpu.c b/target/mips/cpu.c >> index e217fb3e36..a33058609a 100644 >> --- a/target/mips/cpu.c >> +++ b/target/mips/cpu.c >> @@ -197,9 +197,7 @@ static void mips_cpu_class_init(ObjectClass *c, void > *data) >> cc->synchronize_from_tb = mips_cpu_synchronize_from_tb; >> cc->gdb_read_register = mips_cpu_gdb_read_register; >> cc->gdb_write_register = mips_cpu_gdb_write_register; >> -#ifdef CONFIG_USER_ONLY >> - cc->handle_mmu_fault = mips_cpu_handle_mmu_fault; >> -#else >> +#ifndef CONFIG_USER_ONLY >> cc->do_unassigned_access = mips_cpu_unassigned_access; >> cc->do_unaligned_access = mips_cpu_do_unaligned_access; >> cc->get_phys_page_debug = mips_cpu_get_phys_page_debug; >> @@ -208,6 +206,7 @@ static void mips_cpu_class_init(ObjectClass *c, void > *data) >> cc->disas_set_info = mips_cpu_disas_set_info; >> #ifdef CONFIG_TCG >> cc->tcg_initialize = mips_tcg_init; >> + cc->tlb_fill = mips_cpu_tlb_fill; >> #endif >> >> cc->gdb_num_core_regs = 73; >> diff --git a/target/mips/helper.c b/target/mips/helper.c >> index 86e622efb8..3a4917ce7b 100644 >> --- a/target/mips/helper.c >> +++ b/target/mips/helper.c >> @@ -874,31 +874,25 @@ refill: >> #endif >> #endif >> >> -int mips_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size, int > rw, >> - int mmu_idx) >> +bool mips_cpu_tlb_fill(CPUState *cs, vaddr address, int size, >> + MMUAccessType access_type, int mmu_idx, >> + bool probe, uintptr_t retaddr) >> { >> MIPSCPU *cpu = MIPS_CPU(cs); >> CPUMIPSState *env = &cpu->env; >> #if !defined(CONFIG_USER_ONLY) >> hwaddr physical; >> int prot; >> - int access_type; >> + int mips_access_type; >> #endif >> int ret = TLBRET_BADADDR; >> >> -#if 0 >> - log_cpu_state(cs, 0); >> -#endif >> - qemu_log_mask(CPU_LOG_MMU, >> - "%s pc " TARGET_FMT_lx " ad %" VADDR_PRIx " rw %d mmu_idx > %d\n", >> - __func__, env->active_tc.PC, address, rw, mmu_idx); >> - >> /* data access */ >> #if !defined(CONFIG_USER_ONLY) >> /* XXX: put correct access by using cpu_restore_state() correctly */ >> - access_type = ACCESS_INT; >> - ret = get_physical_address(env, &physical, &prot, >> - address, rw, access_type, mmu_idx); >> + mips_access_type = ACCESS_INT; >> + ret = get_physical_address(env, &physical, &prot, address, >> + access_type, mips_access_type, mmu_idx); >> switch (ret) { >> case TLBRET_MATCH: >> qemu_log_mask(CPU_LOG_MMU, >> @@ -915,7 +909,7 @@ int mips_cpu_handle_mmu_fault(CPUState *cs, vaddr > address, int size, int rw, >> tlb_set_page(cs, address & TARGET_PAGE_MASK, >> physical & TARGET_PAGE_MASK, prot | PAGE_EXEC, >> mmu_idx, TARGET_PAGE_SIZE); >> - return 0; >> + return true; >> } >> #if !defined(TARGET_MIPS64) >> if ((ret == TLBRET_NOMATCH) && (env->tlb->nb_tlb > 1)) { >> @@ -926,27 +920,36 @@ int mips_cpu_handle_mmu_fault(CPUState *cs, vaddr > address, int size, int rw, >> int mode = (env->hflags & MIPS_HFLAG_KSU); >> bool ret_walker; >> env->hflags &= ~MIPS_HFLAG_KSU; >> - ret_walker = page_table_walk_refill(env, address, rw, mmu_idx); >> + ret_walker = page_table_walk_refill(env, address, access_type, > mmu_idx); >> env->hflags |= mode; >> if (ret_walker) { >> - ret = get_physical_address(env, &physical, &prot, >> - address, rw, access_type, > mmu_idx); >> + ret = get_physical_address(env, &physical, &prot, address, >> + access_type, mips_access_type, > mmu_idx);¿ >> if (ret == TLBRET_MATCH) { >> tlb_set_page(cs, address & TARGET_PAGE_MASK, >> physical & TARGET_PAGE_MASK, prot | > PAGE_EXEC, >> mmu_idx, TARGET_PAGE_SIZE); >> - return 0; >> + return true; >> } >> } >> } >> #endif >> + if (probe) { >> + return false; >> + } >> #endif >> >> - raise_mmu_exception(env, address, rw, ret); >> - return 1; >> + raise_mmu_exception(env, address, access_type, ret); >> + do_raise_exception_err(env, cs->exception_index, env->error_code, > retaddr); >> +} >> + >> +#ifndef CONFIG_USER_ONLY >> +void tlb_fill(CPUState *cs, target_ulong addr, int size, >> + MMUAccessType access_type, int mmu_idx, uintptr_t retaddr) >> +{ >> + mips_cpu_tlb_fill(cs, addr, size, access_type, mmu_idx, false, > retaddr); >> } >> >> -#if !defined(CONFIG_USER_ONLY) >> hwaddr cpu_mips_translate_address(CPUMIPSState *env, target_ulong > address, int rw) >> { >> hwaddr physical; >> diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c >> index 0f272a5b93..6d86912958 100644 >> --- a/target/mips/op_helper.c >> +++ b/target/mips/op_helper.c >> @@ -2669,21 +2669,6 @@ void mips_cpu_do_unaligned_access(CPUState *cs, > vaddr addr, >> do_raise_exception_err(env, excp, error_code, retaddr); >> } >> >> -void tlb_fill(CPUState *cs, target_ulong addr, int size, >> - MMUAccessType access_type, int mmu_idx, uintptr_t retaddr) >> -{ >> - int ret; >> - >> - ret = mips_cpu_handle_mmu_fault(cs, addr, size, access_type, > mmu_idx); >> - if (ret) { >> - MIPSCPU *cpu = MIPS_CPU(cs); >> - CPUMIPSState *env = &cpu->env; >> - >> - do_raise_exception_err(env, cs->exception_index, >> - env->error_code, retaddr); >> - } >> -} >> - >> void mips_cpu_unassigned_access(CPUState *cs, hwaddr addr, >> bool is_write, bool is_exec, int unused, >> unsigned size) >> -- >> 2.17.1 >> >> -- Alex Bennée
On May 14, 2019 5:26 PM, "Peter Maydell" <peter.maydell@linaro.org> wrote: > > On Sat, 11 May 2019 at 14:43, Aleksandar Markovic > <aleksandar.m.mail@gmail.com> wrote: > > This commit message is generally poor, as it explains relatively unimportant logging issue, while not explaining the core of the change. > > I think the assumption with this sort of "refactor to change > all instances of an API" change is that you will go and > look at the commit message (or cover letter for the patch > series) that introduces the new API; there's no great need > to repeat the same justification for every commit that > applies the refactoring to each of our dozen or more > guest front-ends. > > I'm not sure that wordsmithing a commit message really > justifies rerolling this pull request at this point. > > thanks > -- PMM Peter, hi. Thanks for your engaging in the discussion! I am not complaining about myself looking at another commit message, but have future maintainers and future developers in mind. Their effort needed for deciphering commit messages like this one is multiple times larger than putting together a clear, full, and right-on-the-money message by the submitter. The commit messages should be made convenient for their readers, not writters, shouldn't they? That being said, I tend to accept your judgement, and I withdraw any request for changing elements of this pull request. Please go ahead with integrating it, and thanks again for involving in this issue, and sorry for taking your time. Sincerely, Aleksandar
On 5/14/19 5:48 PM, Alex Bennée wrote: > > Aleksandar Markovic <aleksandar.m.mail@gmail.com> writes: > >> On May 10, 2019 8:57 PM, "Richard Henderson" <richard.henderson@linaro.org> >> wrote: >>> >> >> Please change the title to 'target/mips: Switch to using >> mips_cpu_tlb_fill()', or something along that line. > > It does seem a little redundant as "target/mips:" already marks it as a > mips specific change and viewing the log you can see a series of > architectures being converted to a new API. > >> Also, the reason for changing the field access_type to mips_access type >> should be explained in the commit message. > > ok > >> This commit message is generally poor, as it explains relatively >> unimportant logging issue, while not explaining the core of the >> change. > > Surely the core of the change is explained in the main patches that > introduce the new API? I think it would be redundant to repeat that for > every individual architecture touched. It's a shame it's hard to > explicitly reference a patch in the same series as the commit hashes are > not yet permanent. At least when we fix things referring to the short > hash of the original commit is fairly easy. Except in the case the maintainer is sending a pull request (like here) where he can manually fix the commits. Still this is a PITA... > > Generally for an architecture conversion I want to know what might be > different from other architectures converted. If it is a broadly > mechanical change it doesn't need to be too detailed.
On Tue, 14 May 2019 at 17:05, Aleksandar Markovic
<aleksandar.m.mail@gmail.com> wrote:
> I am not complaining about myself looking at another commit message, but have future maintainers and future developers in mind. Their effort needed for deciphering commit messages like this one is multiple times larger than putting together a clear, full, and right-on-the-money message by the submitter. The commit messages should be made convenient for their readers, not writters, shouldn't they?
Yeah, good commit messages are important; in the end there
is a judgement call to be made about how much detail is useful.
I think one thing that might have affected our differing
views here is that you were only cc'd on the MIPS-related patch,
so will have been looking at it in isolation. I read the whole
series starting with the initial patches which introduced the
API, so had a fuller context for looking at this patch.
(I think future readers will also be able to acquire that
fuller context easily because they can just look through
the git history for the nearby commit that adds the tlb_fill
method if they want the rationale for the refactoring.)
thanks
-- PMM
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > On 5/14/19 5:48 PM, Alex Bennée wrote: >> >> Aleksandar Markovic <aleksandar.m.mail@gmail.com> writes: >> >>> On May 10, 2019 8:57 PM, "Richard Henderson" <richard.henderson@linaro.org> >>> wrote: >>>> >>> >>> Please change the title to 'target/mips: Switch to using >>> mips_cpu_tlb_fill()', or something along that line. >> >> It does seem a little redundant as "target/mips:" already marks it as a >> mips specific change and viewing the log you can see a series of >> architectures being converted to a new API. >> >>> Also, the reason for changing the field access_type to mips_access type >>> should be explained in the commit message. >> >> ok >> >>> This commit message is generally poor, as it explains relatively >>> unimportant logging issue, while not explaining the core of the >>> change. >> >> Surely the core of the change is explained in the main patches that >> introduce the new API? I think it would be redundant to repeat that for >> every individual architecture touched. It's a shame it's hard to >> explicitly reference a patch in the same series as the commit hashes are >> not yet permanent. At least when we fix things referring to the short >> hash of the original commit is fairly easy. > > Except in the case the maintainer is sending a pull request (like here) > where he can manually fix the commits. Still this is a PITA... If there was tooling that can go from a patch series to a pull request then maybe. But generally a PR is a series of patches that have now passed some standard and can now be sent. I'm not sure I'd want to go over all my commits and re-edit the messages at that point. -- Alex Bennée
diff --git a/target/mips/internal.h b/target/mips/internal.h index 286e3888ab..b2b41a51ab 100644 --- a/target/mips/internal.h +++ b/target/mips/internal.h @@ -202,8 +202,9 @@ void cpu_mips_start_count(CPUMIPSState *env); void cpu_mips_stop_count(CPUMIPSState *env); /* helper.c */ -int mips_cpu_handle_mmu_fault(CPUState *cpu, vaddr address, int size, int rw, - int mmu_idx); +bool mips_cpu_tlb_fill(CPUState *cs, vaddr address, int size, + MMUAccessType access_type, int mmu_idx, + bool probe, uintptr_t retaddr); /* op_helper.c */ uint32_t float_class_s(uint32_t arg, float_status *fst); diff --git a/target/mips/cpu.c b/target/mips/cpu.c index e217fb3e36..a33058609a 100644 --- a/target/mips/cpu.c +++ b/target/mips/cpu.c @@ -197,9 +197,7 @@ static void mips_cpu_class_init(ObjectClass *c, void *data) cc->synchronize_from_tb = mips_cpu_synchronize_from_tb; cc->gdb_read_register = mips_cpu_gdb_read_register; cc->gdb_write_register = mips_cpu_gdb_write_register; -#ifdef CONFIG_USER_ONLY - cc->handle_mmu_fault = mips_cpu_handle_mmu_fault; -#else +#ifndef CONFIG_USER_ONLY cc->do_unassigned_access = mips_cpu_unassigned_access; cc->do_unaligned_access = mips_cpu_do_unaligned_access; cc->get_phys_page_debug = mips_cpu_get_phys_page_debug; @@ -208,6 +206,7 @@ static void mips_cpu_class_init(ObjectClass *c, void *data) cc->disas_set_info = mips_cpu_disas_set_info; #ifdef CONFIG_TCG cc->tcg_initialize = mips_tcg_init; + cc->tlb_fill = mips_cpu_tlb_fill; #endif cc->gdb_num_core_regs = 73; diff --git a/target/mips/helper.c b/target/mips/helper.c index 86e622efb8..3a4917ce7b 100644 --- a/target/mips/helper.c +++ b/target/mips/helper.c @@ -874,31 +874,25 @@ refill: #endif #endif -int mips_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size, int rw, - int mmu_idx) +bool mips_cpu_tlb_fill(CPUState *cs, vaddr address, int size, + MMUAccessType access_type, int mmu_idx, + bool probe, uintptr_t retaddr) { MIPSCPU *cpu = MIPS_CPU(cs); CPUMIPSState *env = &cpu->env; #if !defined(CONFIG_USER_ONLY) hwaddr physical; int prot; - int access_type; + int mips_access_type; #endif int ret = TLBRET_BADADDR; -#if 0 - log_cpu_state(cs, 0); -#endif - qemu_log_mask(CPU_LOG_MMU, - "%s pc " TARGET_FMT_lx " ad %" VADDR_PRIx " rw %d mmu_idx %d\n", - __func__, env->active_tc.PC, address, rw, mmu_idx); - /* data access */ #if !defined(CONFIG_USER_ONLY) /* XXX: put correct access by using cpu_restore_state() correctly */ - access_type = ACCESS_INT; - ret = get_physical_address(env, &physical, &prot, - address, rw, access_type, mmu_idx); + mips_access_type = ACCESS_INT; + ret = get_physical_address(env, &physical, &prot, address, + access_type, mips_access_type, mmu_idx); switch (ret) { case TLBRET_MATCH: qemu_log_mask(CPU_LOG_MMU, @@ -915,7 +909,7 @@ int mips_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size, int rw, tlb_set_page(cs, address & TARGET_PAGE_MASK, physical & TARGET_PAGE_MASK, prot | PAGE_EXEC, mmu_idx, TARGET_PAGE_SIZE); - return 0; + return true; } #if !defined(TARGET_MIPS64) if ((ret == TLBRET_NOMATCH) && (env->tlb->nb_tlb > 1)) { @@ -926,27 +920,36 @@ int mips_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size, int rw, int mode = (env->hflags & MIPS_HFLAG_KSU); bool ret_walker; env->hflags &= ~MIPS_HFLAG_KSU; - ret_walker = page_table_walk_refill(env, address, rw, mmu_idx); + ret_walker = page_table_walk_refill(env, address, access_type, mmu_idx); env->hflags |= mode; if (ret_walker) { - ret = get_physical_address(env, &physical, &prot, - address, rw, access_type, mmu_idx); + ret = get_physical_address(env, &physical, &prot, address, + access_type, mips_access_type, mmu_idx); if (ret == TLBRET_MATCH) { tlb_set_page(cs, address & TARGET_PAGE_MASK, physical & TARGET_PAGE_MASK, prot | PAGE_EXEC, mmu_idx, TARGET_PAGE_SIZE); - return 0; + return true; } } } #endif + if (probe) { + return false; + } #endif - raise_mmu_exception(env, address, rw, ret); - return 1; + raise_mmu_exception(env, address, access_type, ret); + do_raise_exception_err(env, cs->exception_index, env->error_code, retaddr); +} + +#ifndef CONFIG_USER_ONLY +void tlb_fill(CPUState *cs, target_ulong addr, int size, + MMUAccessType access_type, int mmu_idx, uintptr_t retaddr) +{ + mips_cpu_tlb_fill(cs, addr, size, access_type, mmu_idx, false, retaddr); } -#if !defined(CONFIG_USER_ONLY) hwaddr cpu_mips_translate_address(CPUMIPSState *env, target_ulong address, int rw) { hwaddr physical; diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c index 0f272a5b93..6d86912958 100644 --- a/target/mips/op_helper.c +++ b/target/mips/op_helper.c @@ -2669,21 +2669,6 @@ void mips_cpu_do_unaligned_access(CPUState *cs, vaddr addr, do_raise_exception_err(env, excp, error_code, retaddr); } -void tlb_fill(CPUState *cs, target_ulong addr, int size, - MMUAccessType access_type, int mmu_idx, uintptr_t retaddr) -{ - int ret; - - ret = mips_cpu_handle_mmu_fault(cs, addr, size, access_type, mmu_idx); - if (ret) { - MIPSCPU *cpu = MIPS_CPU(cs); - CPUMIPSState *env = &cpu->env; - - do_raise_exception_err(env, cs->exception_index, - env->error_code, retaddr); - } -} - void mips_cpu_unassigned_access(CPUState *cs, hwaddr addr, bool is_write, bool is_exec, int unused, unsigned size)