| Message ID | 20201206233949.3783184-19-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/mips: Boring code reordering | expand |
On 12/7/20 12:39 AM, Philippe Mathieu-Daudé wrote: > Restrict the following CPUClass handlers to TCG: > - do_interrupt In this patch it seems do_interrupt is changed to be CONFIG_USER_ONLY, it is not restricted to TCG. Was this desired, should be commented as such? Also, should the whole function then be #ifdefed out in helpers.c? I guess I am vouching for moving the ifndef CONFIG_USER_ONLY one line up in helpers.c. But you wanted this CONFIG_TCG-only? > - do_transaction_failed > - do_unaligned_access > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > Cc: Claudio Fontana <cfontana@suse.de> > > target/mips/cpu.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/target/mips/cpu.c b/target/mips/cpu.c > index 8a4486e3ea1..03bd35b7903 100644 > --- a/target/mips/cpu.c > +++ b/target/mips/cpu.c > @@ -483,7 +483,6 @@ static void mips_cpu_class_init(ObjectClass *c, void *data) > > cc->class_by_name = mips_cpu_class_by_name; > cc->has_work = mips_cpu_has_work; > - cc->do_interrupt = mips_cpu_do_interrupt; > cc->cpu_exec_interrupt = mips_cpu_exec_interrupt; > cc->dump_state = mips_cpu_dump_state; > cc->set_pc = mips_cpu_set_pc; > @@ -491,8 +490,7 @@ static void mips_cpu_class_init(ObjectClass *c, void *data) > cc->gdb_read_register = mips_cpu_gdb_read_register; > cc->gdb_write_register = mips_cpu_gdb_write_register; > #ifndef CONFIG_USER_ONLY > - cc->do_transaction_failed = mips_cpu_do_transaction_failed; > - cc->do_unaligned_access = mips_cpu_do_unaligned_access; > + cc->do_interrupt = mips_cpu_do_interrupt; > cc->get_phys_page_debug = mips_cpu_get_phys_page_debug; > cc->vmsd = &vmstate_mips_cpu; > #endif > @@ -500,6 +498,10 @@ static void mips_cpu_class_init(ObjectClass *c, void *data) > #ifdef CONFIG_TCG > cc->tcg_initialize = mips_tcg_init; > cc->tlb_fill = mips_cpu_tlb_fill; > +#if !defined(CONFIG_USER_ONLY) > + cc->do_unaligned_access = mips_cpu_do_unaligned_access; > + cc->do_transaction_failed = mips_cpu_do_transaction_failed; > +#endif /* CONFIG_TCG && !CONFIG_USER_ONLY */ > #endif > > cc->gdb_num_core_regs = 73; >
On 12/7/20 8:59 AM, Claudio Fontana wrote: > On 12/7/20 12:39 AM, Philippe Mathieu-Daudé wrote: >> Restrict the following CPUClass handlers to TCG: >> - do_interrupt > > In this patch it seems do_interrupt is changed to be CONFIG_USER_ONLY, it is not restricted to TCG. Of course it _is_ as only TCG can do -user- , but would it be better to wrap everything around CONFIG_TCG for what is tcg-only, and then add a subsection in there for CONFIG_USER_ONLY? > Was this desired, should be commented as such? > > Also, should the whole function then be #ifdefed out in helpers.c? > I guess I am vouching for moving the ifndef CONFIG_USER_ONLY one line up in helpers.c. > > But you wanted this CONFIG_TCG-only? > > >> - do_transaction_failed >> - do_unaligned_access >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> Cc: Claudio Fontana <cfontana@suse.de> >> >> target/mips/cpu.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/target/mips/cpu.c b/target/mips/cpu.c >> index 8a4486e3ea1..03bd35b7903 100644 >> --- a/target/mips/cpu.c >> +++ b/target/mips/cpu.c >> @@ -483,7 +483,6 @@ static void mips_cpu_class_init(ObjectClass *c, void *data) >> >> cc->class_by_name = mips_cpu_class_by_name; >> cc->has_work = mips_cpu_has_work; >> - cc->do_interrupt = mips_cpu_do_interrupt; >> cc->cpu_exec_interrupt = mips_cpu_exec_interrupt; >> cc->dump_state = mips_cpu_dump_state; >> cc->set_pc = mips_cpu_set_pc; >> @@ -491,8 +490,7 @@ static void mips_cpu_class_init(ObjectClass *c, void *data) >> cc->gdb_read_register = mips_cpu_gdb_read_register; >> cc->gdb_write_register = mips_cpu_gdb_write_register; >> #ifndef CONFIG_USER_ONLY >> - cc->do_transaction_failed = mips_cpu_do_transaction_failed; >> - cc->do_unaligned_access = mips_cpu_do_unaligned_access; >> + cc->do_interrupt = mips_cpu_do_interrupt; >> cc->get_phys_page_debug = mips_cpu_get_phys_page_debug; >> cc->vmsd = &vmstate_mips_cpu; >> #endif >> @@ -500,6 +498,10 @@ static void mips_cpu_class_init(ObjectClass *c, void *data) >> #ifdef CONFIG_TCG >> cc->tcg_initialize = mips_tcg_init; >> cc->tlb_fill = mips_cpu_tlb_fill; >> +#if !defined(CONFIG_USER_ONLY) >> + cc->do_unaligned_access = mips_cpu_do_unaligned_access; >> + cc->do_transaction_failed = mips_cpu_do_transaction_failed; >> +#endif /* CONFIG_TCG && !CONFIG_USER_ONLY */ >> #endif >> >> cc->gdb_num_core_regs = 73; >> >
On 12/7/20 9:53 AM, Claudio Fontana wrote: > On 12/7/20 8:59 AM, Claudio Fontana wrote: >> On 12/7/20 12:39 AM, Philippe Mathieu-Daudé wrote: >>> Restrict the following CPUClass handlers to TCG: >>> - do_interrupt >> >> In this patch it seems do_interrupt is changed to be CONFIG_USER_ONLY, it is not restricted to TCG. > > Of course it _is_ as only TCG can do -user- , but would it be better to wrap everything around CONFIG_TCG for what is tcg-only, and then add a subsection in there for CONFIG_USER_ONLY? Need coffee, sorry. I think you can see the issue there, sorry for the confusion. Thanks, Claudio > >> Was this desired, should be commented as such? >> >> Also, should the whole function then be #ifdefed out in helpers.c? >> I guess I am vouching for moving the ifndef CONFIG_USER_ONLY one line up in helpers.c. >> >> But you wanted this CONFIG_TCG-only? >> >> >>> - do_transaction_failed >>> - do_unaligned_access >>> >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>> --- >>> Cc: Claudio Fontana <cfontana@suse.de> >>> >>> target/mips/cpu.c | 8 +++++--- >>> 1 file changed, 5 insertions(+), 3 deletions(-) >>> >>> diff --git a/target/mips/cpu.c b/target/mips/cpu.c >>> index 8a4486e3ea1..03bd35b7903 100644 >>> --- a/target/mips/cpu.c >>> +++ b/target/mips/cpu.c >>> @@ -483,7 +483,6 @@ static void mips_cpu_class_init(ObjectClass *c, void *data) >>> >>> cc->class_by_name = mips_cpu_class_by_name; >>> cc->has_work = mips_cpu_has_work; >>> - cc->do_interrupt = mips_cpu_do_interrupt; >>> cc->cpu_exec_interrupt = mips_cpu_exec_interrupt; >>> cc->dump_state = mips_cpu_dump_state; >>> cc->set_pc = mips_cpu_set_pc; >>> @@ -491,8 +490,7 @@ static void mips_cpu_class_init(ObjectClass *c, void *data) >>> cc->gdb_read_register = mips_cpu_gdb_read_register; >>> cc->gdb_write_register = mips_cpu_gdb_write_register; >>> #ifndef CONFIG_USER_ONLY >>> - cc->do_transaction_failed = mips_cpu_do_transaction_failed; >>> - cc->do_unaligned_access = mips_cpu_do_unaligned_access; >>> + cc->do_interrupt = mips_cpu_do_interrupt; >>> cc->get_phys_page_debug = mips_cpu_get_phys_page_debug; >>> cc->vmsd = &vmstate_mips_cpu; >>> #endif >>> @@ -500,6 +498,10 @@ static void mips_cpu_class_init(ObjectClass *c, void *data) >>> #ifdef CONFIG_TCG >>> cc->tcg_initialize = mips_tcg_init; >>> cc->tlb_fill = mips_cpu_tlb_fill; >>> +#if !defined(CONFIG_USER_ONLY) >>> + cc->do_unaligned_access = mips_cpu_do_unaligned_access; >>> + cc->do_transaction_failed = mips_cpu_do_transaction_failed; >>> +#endif /* CONFIG_TCG && !CONFIG_USER_ONLY */ >>> #endif >>> >>> cc->gdb_num_core_regs = 73; >>> >> >
I am adding to my cleanup series the following, so this is done for all targets:
Author: Claudio Fontana <cfontana@suse.de>
Date: Mon Dec 7 11:02:34 2020 +0100
cpu: move do_unaligned_access to tcg_ops
make it consistently SOFTMMU-only.
Signed-off-by: Claudio Fontana <cfontana@suse.de>
commit 1ee8254b568a47453ab481aa206fb9fecc7c16f7
Author: Claudio Fontana <cfontana@suse.de>
Date: Mon Dec 7 10:29:22 2020 +0100
cpu: move cc->transaction_failed to tcg_ops
Signed-off-by: Claudio Fontana <cfontana@suse.de>
commit 1a03124581841b5c473f879f5fd396dccde48667
Author: Claudio Fontana <cfontana@suse.de>
Date: Mon Dec 7 10:02:07 2020 +0100
cpu: move cc->do_interrupt to tcg_ops
Signed-off-by: Claudio Fontana <cfontana@suse.de>
commit 6a35e8f4ee68923006bba404f1f2471038b1039c
Author: Claudio Fontana <cfontana@suse.de>
Date: Mon Dec 7 09:31:14 2020 +0100
target/arm: do not use cc->do_interrupt for KVM directly
cc->do_interrupt is a TCG callback used in accel/tcg only,
call instead directly the arm_cpu_do_interrupt for the
injection of exeptions from KVM, so that
do_interrupt can be exported to TCG-only operations in
the CPUClass.
Signed-off-by: Claudio Fontana <cfontana@suse.de>
On 12/7/20 10:07 AM, Claudio Fontana wrote:
> On 12/7/20 9:53 AM, Claudio Fontana wrote:
>> On 12/7/20 8:59 AM, Claudio Fontana wrote:
>>> On 12/7/20 12:39 AM, Philippe Mathieu-Daudé wrote:
>>>> Restrict the following CPUClass handlers to TCG:
>>>> - do_interrupt
>>>
>>> In this patch it seems do_interrupt is changed to be CONFIG_USER_ONLY, it is not restricted to TCG.
>>
>> Of course it _is_ as only TCG can do -user- , but would it be better to wrap everything around CONFIG_TCG for what is tcg-only, and then add a subsection in there for CONFIG_USER_ONLY?
>
>
> Need coffee, sorry. I think you can see the issue there, sorry for the confusion.
>
> Thanks,
>
> Claudio
>
>
>>
>>> Was this desired, should be commented as such?
>>>
>>> Also, should the whole function then be #ifdefed out in helpers.c?
>>> I guess I am vouching for moving the ifndef CONFIG_USER_ONLY one line up in helpers.c.
>>>
>>> But you wanted this CONFIG_TCG-only?
>>>
>>>
>>>> - do_transaction_failed
>>>> - do_unaligned_access
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> ---
>>>> Cc: Claudio Fontana <cfontana@suse.de>
>>>>
>>>> target/mips/cpu.c | 8 +++++---
>>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
>>>> index 8a4486e3ea1..03bd35b7903 100644
>>>> --- a/target/mips/cpu.c
>>>> +++ b/target/mips/cpu.c
>>>> @@ -483,7 +483,6 @@ static void mips_cpu_class_init(ObjectClass *c, void *data)
>>>>
>>>> cc->class_by_name = mips_cpu_class_by_name;
>>>> cc->has_work = mips_cpu_has_work;
>>>> - cc->do_interrupt = mips_cpu_do_interrupt;
>>>> cc->cpu_exec_interrupt = mips_cpu_exec_interrupt;
>>>> cc->dump_state = mips_cpu_dump_state;
>>>> cc->set_pc = mips_cpu_set_pc;
>>>> @@ -491,8 +490,7 @@ static void mips_cpu_class_init(ObjectClass *c, void *data)
>>>> cc->gdb_read_register = mips_cpu_gdb_read_register;
>>>> cc->gdb_write_register = mips_cpu_gdb_write_register;
>>>> #ifndef CONFIG_USER_ONLY
>>>> - cc->do_transaction_failed = mips_cpu_do_transaction_failed;
>>>> - cc->do_unaligned_access = mips_cpu_do_unaligned_access;
>>>> + cc->do_interrupt = mips_cpu_do_interrupt;
>>>> cc->get_phys_page_debug = mips_cpu_get_phys_page_debug;
>>>> cc->vmsd = &vmstate_mips_cpu;
>>>> #endif
>>>> @@ -500,6 +498,10 @@ static void mips_cpu_class_init(ObjectClass *c, void *data)
>>>> #ifdef CONFIG_TCG
>>>> cc->tcg_initialize = mips_tcg_init;
>>>> cc->tlb_fill = mips_cpu_tlb_fill;
>>>> +#if !defined(CONFIG_USER_ONLY)
>>>> + cc->do_unaligned_access = mips_cpu_do_unaligned_access;
>>>> + cc->do_transaction_failed = mips_cpu_do_transaction_failed;
>>>> +#endif /* CONFIG_TCG && !CONFIG_USER_ONLY */
>>>> #endif
>>>>
>>>> cc->gdb_num_core_regs = 73;
>>>>
>>>
>>
>
On 12/7/20 12:43 PM, Claudio Fontana wrote: > I am adding to my cleanup series the following, so this is done for all targets: Great! thank you Claudio :) > > > Author: Claudio Fontana <cfontana@suse.de> > Date: Mon Dec 7 11:02:34 2020 +0100 > > cpu: move do_unaligned_access to tcg_ops > > make it consistently SOFTMMU-only. > > Signed-off-by: Claudio Fontana <cfontana@suse.de> > > commit 1ee8254b568a47453ab481aa206fb9fecc7c16f7 > Author: Claudio Fontana <cfontana@suse.de> > Date: Mon Dec 7 10:29:22 2020 +0100 > > cpu: move cc->transaction_failed to tcg_ops > > Signed-off-by: Claudio Fontana <cfontana@suse.de> > > commit 1a03124581841b5c473f879f5fd396dccde48667 > Author: Claudio Fontana <cfontana@suse.de> > Date: Mon Dec 7 10:02:07 2020 +0100 > > cpu: move cc->do_interrupt to tcg_ops > > Signed-off-by: Claudio Fontana <cfontana@suse.de> > > commit 6a35e8f4ee68923006bba404f1f2471038b1039c > Author: Claudio Fontana <cfontana@suse.de> > Date: Mon Dec 7 09:31:14 2020 +0100 > > target/arm: do not use cc->do_interrupt for KVM directly > > cc->do_interrupt is a TCG callback used in accel/tcg only, > call instead directly the arm_cpu_do_interrupt for the > injection of exeptions from KVM, so that > > do_interrupt can be exported to TCG-only operations in > the CPUClass. > > Signed-off-by: Claudio Fontana <cfontana@suse.de>
diff --git a/target/mips/cpu.c b/target/mips/cpu.c index 8a4486e3ea1..03bd35b7903 100644 --- a/target/mips/cpu.c +++ b/target/mips/cpu.c @@ -483,7 +483,6 @@ static void mips_cpu_class_init(ObjectClass *c, void *data) cc->class_by_name = mips_cpu_class_by_name; cc->has_work = mips_cpu_has_work; - cc->do_interrupt = mips_cpu_do_interrupt; cc->cpu_exec_interrupt = mips_cpu_exec_interrupt; cc->dump_state = mips_cpu_dump_state; cc->set_pc = mips_cpu_set_pc; @@ -491,8 +490,7 @@ static void mips_cpu_class_init(ObjectClass *c, void *data) cc->gdb_read_register = mips_cpu_gdb_read_register; cc->gdb_write_register = mips_cpu_gdb_write_register; #ifndef CONFIG_USER_ONLY - cc->do_transaction_failed = mips_cpu_do_transaction_failed; - cc->do_unaligned_access = mips_cpu_do_unaligned_access; + cc->do_interrupt = mips_cpu_do_interrupt; cc->get_phys_page_debug = mips_cpu_get_phys_page_debug; cc->vmsd = &vmstate_mips_cpu; #endif @@ -500,6 +498,10 @@ static void mips_cpu_class_init(ObjectClass *c, void *data) #ifdef CONFIG_TCG cc->tcg_initialize = mips_tcg_init; cc->tlb_fill = mips_cpu_tlb_fill; +#if !defined(CONFIG_USER_ONLY) + cc->do_unaligned_access = mips_cpu_do_unaligned_access; + cc->do_transaction_failed = mips_cpu_do_transaction_failed; +#endif /* CONFIG_TCG && !CONFIG_USER_ONLY */ #endif cc->gdb_num_core_regs = 73;
Restrict the following CPUClass handlers to TCG: - do_interrupt - do_transaction_failed - do_unaligned_access Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- Cc: Claudio Fontana <cfontana@suse.de> target/mips/cpu.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)