| Message ID | 20201211083143.14350-19-cfontana@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i386 cleanup PART 1 | expand |
On 12/11/20 2:31 AM, Claudio Fontana wrote: > From: Eduardo Habkost <ehabkost@redhat.com> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > [claudio: wrapped in CONFIG_TCG] > Signed-off-by: Claudio Fontana <cfontana@suse.de> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > --- > include/hw/core/cpu.h | 8 -------- > include/hw/core/tcg-cpu-ops.h | 12 ++++++++++++ > accel/tcg/cpu-exec.c | 4 ++-- > target/arm/cpu.c | 4 +++- > target/avr/cpu.c | 2 +- > target/hppa/cpu.c | 2 +- > target/i386/tcg/tcg-cpu.c | 2 +- > target/microblaze/cpu.c | 2 +- > target/mips/cpu.c | 4 +++- > target/riscv/cpu.c | 2 +- > target/rx/cpu.c | 2 +- > target/sh4/cpu.c | 2 +- > target/sparc/cpu.c | 2 +- > target/tricore/cpu.c | 2 +- > 14 files changed, 29 insertions(+), 21 deletions(-) > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h > index ea648d52ad..83007d262c 100644 > --- a/include/hw/core/cpu.h > +++ b/include/hw/core/cpu.h > @@ -110,13 +110,6 @@ struct TranslationBlock; > * If the target behaviour here is anything other than "set > * the PC register to the value passed in" then the target must > * also implement the synchronize_from_tb hook. > - * @synchronize_from_tb: Callback for synchronizing state from a TCG > - * #TranslationBlock. This is called when we abandon execution > - * of a TB before starting it, and must set all parts of the CPU > - * state which the previous TB in the chain may not have updated. > - * This always includes at least the program counter; some targets > - * will need to do more. If this hook is not implemented then the > - * default is to call @set_pc(tb->pc). > * @tlb_fill: Callback for handling a softmmu tlb miss or user-only > * address fault. For system mode, if the access is valid, call > * tlb_set_page and return true; if the access is invalid, and > @@ -193,7 +186,6 @@ struct CPUClass { > void (*get_memory_mapping)(CPUState *cpu, MemoryMappingList *list, > Error **errp); > void (*set_pc)(CPUState *cpu, vaddr value); > - void (*synchronize_from_tb)(CPUState *cpu, struct TranslationBlock *tb); > bool (*tlb_fill)(CPUState *cpu, vaddr address, int size, > MMUAccessType access_type, int mmu_idx, > bool probe, uintptr_t retaddr); > diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h > index 4475ef0996..e1d50b3c8b 100644 > --- a/include/hw/core/tcg-cpu-ops.h > +++ b/include/hw/core/tcg-cpu-ops.h > @@ -10,6 +10,8 @@ > #ifndef TCG_CPU_OPS_H > #define TCG_CPU_OPS_H > > +#include "hw/core/cpu.h" This include is circular. Are you sure that splitting out hw/core/tcg-cpu-ops.h from hw/core/cpu.h in patch 15 is even useful? Otherwise the actual code change looks ok. r~
On 12/11/20 6:05 PM, Richard Henderson wrote: > On 12/11/20 2:31 AM, Claudio Fontana wrote: >> From: Eduardo Habkost <ehabkost@redhat.com> >> >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >> [claudio: wrapped in CONFIG_TCG] >> Signed-off-by: Claudio Fontana <cfontana@suse.de> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> >> --- >> include/hw/core/cpu.h | 8 -------- >> include/hw/core/tcg-cpu-ops.h | 12 ++++++++++++ >> accel/tcg/cpu-exec.c | 4 ++-- >> target/arm/cpu.c | 4 +++- >> target/avr/cpu.c | 2 +- >> target/hppa/cpu.c | 2 +- >> target/i386/tcg/tcg-cpu.c | 2 +- >> target/microblaze/cpu.c | 2 +- >> target/mips/cpu.c | 4 +++- >> target/riscv/cpu.c | 2 +- >> target/rx/cpu.c | 2 +- >> target/sh4/cpu.c | 2 +- >> target/sparc/cpu.c | 2 +- >> target/tricore/cpu.c | 2 +- >> 14 files changed, 29 insertions(+), 21 deletions(-) >> >> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h >> index ea648d52ad..83007d262c 100644 >> --- a/include/hw/core/cpu.h >> +++ b/include/hw/core/cpu.h >> @@ -110,13 +110,6 @@ struct TranslationBlock; >> * If the target behaviour here is anything other than "set >> * the PC register to the value passed in" then the target must >> * also implement the synchronize_from_tb hook. >> - * @synchronize_from_tb: Callback for synchronizing state from a TCG >> - * #TranslationBlock. This is called when we abandon execution >> - * of a TB before starting it, and must set all parts of the CPU >> - * state which the previous TB in the chain may not have updated. >> - * This always includes at least the program counter; some targets >> - * will need to do more. If this hook is not implemented then the >> - * default is to call @set_pc(tb->pc). >> * @tlb_fill: Callback for handling a softmmu tlb miss or user-only >> * address fault. For system mode, if the access is valid, call >> * tlb_set_page and return true; if the access is invalid, and >> @@ -193,7 +186,6 @@ struct CPUClass { >> void (*get_memory_mapping)(CPUState *cpu, MemoryMappingList *list, >> Error **errp); >> void (*set_pc)(CPUState *cpu, vaddr value); >> - void (*synchronize_from_tb)(CPUState *cpu, struct TranslationBlock *tb); >> bool (*tlb_fill)(CPUState *cpu, vaddr address, int size, >> MMUAccessType access_type, int mmu_idx, >> bool probe, uintptr_t retaddr); >> diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h >> index 4475ef0996..e1d50b3c8b 100644 >> --- a/include/hw/core/tcg-cpu-ops.h >> +++ b/include/hw/core/tcg-cpu-ops.h >> @@ -10,6 +10,8 @@ >> #ifndef TCG_CPU_OPS_H >> #define TCG_CPU_OPS_H >> >> +#include "hw/core/cpu.h" > > This include is circular. Yes, it's protected though, it was asked that way. > > Are you sure that splitting out hw/core/tcg-cpu-ops.h from hw/core/cpu.h in > patch 15 is even useful? it avoids a huge #ifdef CONFIG_TCG > > Otherwise the actual code change looks ok. > > > r~ >
On 12/11/20 6:10 PM, Claudio Fontana wrote: > On 12/11/20 6:05 PM, Richard Henderson wrote: >> On 12/11/20 2:31 AM, Claudio Fontana wrote: >>> From: Eduardo Habkost <ehabkost@redhat.com> >>> >>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >>> [claudio: wrapped in CONFIG_TCG] >>> Signed-off-by: Claudio Fontana <cfontana@suse.de> >>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> >>> --- >>> include/hw/core/cpu.h | 8 -------- >>> include/hw/core/tcg-cpu-ops.h | 12 ++++++++++++ >>> accel/tcg/cpu-exec.c | 4 ++-- >>> target/arm/cpu.c | 4 +++- >>> target/avr/cpu.c | 2 +- >>> target/hppa/cpu.c | 2 +- >>> target/i386/tcg/tcg-cpu.c | 2 +- >>> target/microblaze/cpu.c | 2 +- >>> target/mips/cpu.c | 4 +++- >>> target/riscv/cpu.c | 2 +- >>> target/rx/cpu.c | 2 +- >>> target/sh4/cpu.c | 2 +- >>> target/sparc/cpu.c | 2 +- >>> target/tricore/cpu.c | 2 +- >>> 14 files changed, 29 insertions(+), 21 deletions(-) >>> >>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h >>> index ea648d52ad..83007d262c 100644 >>> --- a/include/hw/core/cpu.h >>> +++ b/include/hw/core/cpu.h >>> @@ -110,13 +110,6 @@ struct TranslationBlock; >>> * If the target behaviour here is anything other than "set >>> * the PC register to the value passed in" then the target must >>> * also implement the synchronize_from_tb hook. >>> - * @synchronize_from_tb: Callback for synchronizing state from a TCG >>> - * #TranslationBlock. This is called when we abandon execution >>> - * of a TB before starting it, and must set all parts of the CPU >>> - * state which the previous TB in the chain may not have updated. >>> - * This always includes at least the program counter; some targets >>> - * will need to do more. If this hook is not implemented then the >>> - * default is to call @set_pc(tb->pc). >>> * @tlb_fill: Callback for handling a softmmu tlb miss or user-only >>> * address fault. For system mode, if the access is valid, call >>> * tlb_set_page and return true; if the access is invalid, and >>> @@ -193,7 +186,6 @@ struct CPUClass { >>> void (*get_memory_mapping)(CPUState *cpu, MemoryMappingList *list, >>> Error **errp); >>> void (*set_pc)(CPUState *cpu, vaddr value); >>> - void (*synchronize_from_tb)(CPUState *cpu, struct TranslationBlock *tb); >>> bool (*tlb_fill)(CPUState *cpu, vaddr address, int size, >>> MMUAccessType access_type, int mmu_idx, >>> bool probe, uintptr_t retaddr); >>> diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h >>> index 4475ef0996..e1d50b3c8b 100644 >>> --- a/include/hw/core/tcg-cpu-ops.h >>> +++ b/include/hw/core/tcg-cpu-ops.h >>> @@ -10,6 +10,8 @@ >>> #ifndef TCG_CPU_OPS_H >>> #define TCG_CPU_OPS_H >>> >>> +#include "hw/core/cpu.h" >> >> This include is circular. > > Yes, it's protected though, it was asked that way. I mean, during the review. I personally would have preferred to avoid these as only cpu.h is including this for now. > >> >> Are you sure that splitting out hw/core/tcg-cpu-ops.h from hw/core/cpu.h in >> patch 15 is even useful? > > it avoids a huge #ifdef CONFIG_TCG > > >> >> Otherwise the actual code change looks ok. >> >> >> r~ >> >
On 12/11/20 11:10 AM, Claudio Fontana wrote: > On 12/11/20 6:05 PM, Richard Henderson wrote: >> On 12/11/20 2:31 AM, Claudio Fontana wrote: >>> From: Eduardo Habkost <ehabkost@redhat.com> >>> >>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >>> [claudio: wrapped in CONFIG_TCG] >>> Signed-off-by: Claudio Fontana <cfontana@suse.de> >>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> >>> --- >>> include/hw/core/cpu.h | 8 -------- >>> include/hw/core/tcg-cpu-ops.h | 12 ++++++++++++ >>> accel/tcg/cpu-exec.c | 4 ++-- >>> target/arm/cpu.c | 4 +++- >>> target/avr/cpu.c | 2 +- >>> target/hppa/cpu.c | 2 +- >>> target/i386/tcg/tcg-cpu.c | 2 +- >>> target/microblaze/cpu.c | 2 +- >>> target/mips/cpu.c | 4 +++- >>> target/riscv/cpu.c | 2 +- >>> target/rx/cpu.c | 2 +- >>> target/sh4/cpu.c | 2 +- >>> target/sparc/cpu.c | 2 +- >>> target/tricore/cpu.c | 2 +- >>> 14 files changed, 29 insertions(+), 21 deletions(-) >>> >>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h >>> index ea648d52ad..83007d262c 100644 >>> --- a/include/hw/core/cpu.h >>> +++ b/include/hw/core/cpu.h >>> @@ -110,13 +110,6 @@ struct TranslationBlock; >>> * If the target behaviour here is anything other than "set >>> * the PC register to the value passed in" then the target must >>> * also implement the synchronize_from_tb hook. >>> - * @synchronize_from_tb: Callback for synchronizing state from a TCG >>> - * #TranslationBlock. This is called when we abandon execution >>> - * of a TB before starting it, and must set all parts of the CPU >>> - * state which the previous TB in the chain may not have updated. >>> - * This always includes at least the program counter; some targets >>> - * will need to do more. If this hook is not implemented then the >>> - * default is to call @set_pc(tb->pc). >>> * @tlb_fill: Callback for handling a softmmu tlb miss or user-only >>> * address fault. For system mode, if the access is valid, call >>> * tlb_set_page and return true; if the access is invalid, and >>> @@ -193,7 +186,6 @@ struct CPUClass { >>> void (*get_memory_mapping)(CPUState *cpu, MemoryMappingList *list, >>> Error **errp); >>> void (*set_pc)(CPUState *cpu, vaddr value); >>> - void (*synchronize_from_tb)(CPUState *cpu, struct TranslationBlock *tb); >>> bool (*tlb_fill)(CPUState *cpu, vaddr address, int size, >>> MMUAccessType access_type, int mmu_idx, >>> bool probe, uintptr_t retaddr); >>> diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h >>> index 4475ef0996..e1d50b3c8b 100644 >>> --- a/include/hw/core/tcg-cpu-ops.h >>> +++ b/include/hw/core/tcg-cpu-ops.h >>> @@ -10,6 +10,8 @@ >>> #ifndef TCG_CPU_OPS_H >>> #define TCG_CPU_OPS_H >>> >>> +#include "hw/core/cpu.h" >> >> This include is circular. > > Yes, it's protected though, it was asked that way. Well, in my strong opinion, someone asked incorrectly. It's "harmless" because of the protection ifdefs, but it's Wrong because it has the potential to hide bugs. What is it that you thought you needed from core/cpu.h anyway? >> Are you sure that splitting out hw/core/tcg-cpu-ops.h from hw/core/cpu.h in >> patch 15 is even useful? > > it avoids a huge #ifdef CONFIG_TCG So? The question should be: is it useful on its own, and I think the answer to that is clearly not. Thus it should not pretend to be a standalone header file. r~
On 12/11/20 6:28 PM, Richard Henderson wrote: > On 12/11/20 11:10 AM, Claudio Fontana wrote: >> On 12/11/20 6:05 PM, Richard Henderson wrote: >>> On 12/11/20 2:31 AM, Claudio Fontana wrote: >>>> From: Eduardo Habkost <ehabkost@redhat.com> >>>> >>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >>>> [claudio: wrapped in CONFIG_TCG] >>>> Signed-off-by: Claudio Fontana <cfontana@suse.de> >>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> >>>> --- >>>> include/hw/core/cpu.h | 8 -------- >>>> include/hw/core/tcg-cpu-ops.h | 12 ++++++++++++ >>>> accel/tcg/cpu-exec.c | 4 ++-- >>>> target/arm/cpu.c | 4 +++- >>>> target/avr/cpu.c | 2 +- >>>> target/hppa/cpu.c | 2 +- >>>> target/i386/tcg/tcg-cpu.c | 2 +- >>>> target/microblaze/cpu.c | 2 +- >>>> target/mips/cpu.c | 4 +++- >>>> target/riscv/cpu.c | 2 +- >>>> target/rx/cpu.c | 2 +- >>>> target/sh4/cpu.c | 2 +- >>>> target/sparc/cpu.c | 2 +- >>>> target/tricore/cpu.c | 2 +- >>>> 14 files changed, 29 insertions(+), 21 deletions(-) >>>> >>>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h >>>> index ea648d52ad..83007d262c 100644 >>>> --- a/include/hw/core/cpu.h >>>> +++ b/include/hw/core/cpu.h >>>> @@ -110,13 +110,6 @@ struct TranslationBlock; >>>> * If the target behaviour here is anything other than "set >>>> * the PC register to the value passed in" then the target must >>>> * also implement the synchronize_from_tb hook. >>>> - * @synchronize_from_tb: Callback for synchronizing state from a TCG >>>> - * #TranslationBlock. This is called when we abandon execution >>>> - * of a TB before starting it, and must set all parts of the CPU >>>> - * state which the previous TB in the chain may not have updated. >>>> - * This always includes at least the program counter; some targets >>>> - * will need to do more. If this hook is not implemented then the >>>> - * default is to call @set_pc(tb->pc). >>>> * @tlb_fill: Callback for handling a softmmu tlb miss or user-only >>>> * address fault. For system mode, if the access is valid, call >>>> * tlb_set_page and return true; if the access is invalid, and >>>> @@ -193,7 +186,6 @@ struct CPUClass { >>>> void (*get_memory_mapping)(CPUState *cpu, MemoryMappingList *list, >>>> Error **errp); >>>> void (*set_pc)(CPUState *cpu, vaddr value); >>>> - void (*synchronize_from_tb)(CPUState *cpu, struct TranslationBlock *tb); >>>> bool (*tlb_fill)(CPUState *cpu, vaddr address, int size, >>>> MMUAccessType access_type, int mmu_idx, >>>> bool probe, uintptr_t retaddr); >>>> diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h >>>> index 4475ef0996..e1d50b3c8b 100644 >>>> --- a/include/hw/core/tcg-cpu-ops.h >>>> +++ b/include/hw/core/tcg-cpu-ops.h >>>> @@ -10,6 +10,8 @@ >>>> #ifndef TCG_CPU_OPS_H >>>> #define TCG_CPU_OPS_H >>>> >>>> +#include "hw/core/cpu.h" >>> >>> This include is circular. >> >> Yes, it's protected though, it was asked that way. > > Well, in my strong opinion, someone asked incorrectly. It's "harmless" because > of the protection ifdefs, but it's Wrong because it has the potential to hide bugs. > > What is it that you thought you needed from core/cpu.h anyway? > >>> Are you sure that splitting out hw/core/tcg-cpu-ops.h from hw/core/cpu.h in >>> patch 15 is even useful? >> >> it avoids a huge #ifdef CONFIG_TCG > > So? The question should be: is it useful on its own, and I think the answer to > that is clearly not. Thus it should not pretend to be a standalone header file. > > > r~ > The whole point of the exercise is to sort out what is tcg specific and only compile it under CONFIG_TCG. Having everything inside cpu.h wrapped in a 100 line #ifdef is not particularly readable or discoverable, so I think it is actually useful for understanding purposes to have it separate, but that said, I don't feel strongly on this, as I intend to improve this in later series. Thanks, Claudio
On 12/11/20 11:47 AM, Claudio Fontana wrote: >> What is it that you thought you needed from core/cpu.h anyway? ... >>>> Are you sure that splitting out hw/core/tcg-cpu-ops.h from hw/core/cpu.h in >>>> patch 15 is even useful? >>> >>> it avoids a huge #ifdef CONFIG_TCG >> >> So? The question should be: is it useful on its own, and I think the answer to >> that is clearly not. Thus it should not pretend to be a standalone header file. >> > > The whole point of the exercise is to sort out what is tcg specific and > only compile it under CONFIG_TCG. > > Having everything inside cpu.h wrapped in a 100 line #ifdef is not > particularly readable or discoverable, so I think it is actually useful > for understanding purposes to have it separate... Ok, so separate, but perhaps not standalone. My question above remains: what did you need from core/cpu.h? Was it in fact just a typedef for CPUState? In which case "qemu/typedefs.h" is a better choice. r~
On 12/11/20 6:47 PM, Claudio Fontana wrote: > On 12/11/20 6:28 PM, Richard Henderson wrote: >> On 12/11/20 11:10 AM, Claudio Fontana wrote: >>> On 12/11/20 6:05 PM, Richard Henderson wrote: >>>> On 12/11/20 2:31 AM, Claudio Fontana wrote: >>>>> From: Eduardo Habkost <ehabkost@redhat.com> >>>>> >>>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >>>>> [claudio: wrapped in CONFIG_TCG] >>>>> Signed-off-by: Claudio Fontana <cfontana@suse.de> >>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> >>>>> --- >>>>> include/hw/core/cpu.h | 8 -------- >>>>> include/hw/core/tcg-cpu-ops.h | 12 ++++++++++++ >>>>> accel/tcg/cpu-exec.c | 4 ++-- >>>>> target/arm/cpu.c | 4 +++- >>>>> target/avr/cpu.c | 2 +- >>>>> target/hppa/cpu.c | 2 +- >>>>> target/i386/tcg/tcg-cpu.c | 2 +- >>>>> target/microblaze/cpu.c | 2 +- >>>>> target/mips/cpu.c | 4 +++- >>>>> target/riscv/cpu.c | 2 +- >>>>> target/rx/cpu.c | 2 +- >>>>> target/sh4/cpu.c | 2 +- >>>>> target/sparc/cpu.c | 2 +- >>>>> target/tricore/cpu.c | 2 +- >>>>> 14 files changed, 29 insertions(+), 21 deletions(-) >>>>> >>>>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h >>>>> index ea648d52ad..83007d262c 100644 >>>>> --- a/include/hw/core/cpu.h >>>>> +++ b/include/hw/core/cpu.h >>>>> @@ -110,13 +110,6 @@ struct TranslationBlock; >>>>> * If the target behaviour here is anything other than "set >>>>> * the PC register to the value passed in" then the target must >>>>> * also implement the synchronize_from_tb hook. >>>>> - * @synchronize_from_tb: Callback for synchronizing state from a TCG >>>>> - * #TranslationBlock. This is called when we abandon execution >>>>> - * of a TB before starting it, and must set all parts of the CPU >>>>> - * state which the previous TB in the chain may not have updated. >>>>> - * This always includes at least the program counter; some targets >>>>> - * will need to do more. If this hook is not implemented then the >>>>> - * default is to call @set_pc(tb->pc). >>>>> * @tlb_fill: Callback for handling a softmmu tlb miss or user-only >>>>> * address fault. For system mode, if the access is valid, call >>>>> * tlb_set_page and return true; if the access is invalid, and >>>>> @@ -193,7 +186,6 @@ struct CPUClass { >>>>> void (*get_memory_mapping)(CPUState *cpu, MemoryMappingList *list, >>>>> Error **errp); >>>>> void (*set_pc)(CPUState *cpu, vaddr value); >>>>> - void (*synchronize_from_tb)(CPUState *cpu, struct TranslationBlock *tb); >>>>> bool (*tlb_fill)(CPUState *cpu, vaddr address, int size, >>>>> MMUAccessType access_type, int mmu_idx, >>>>> bool probe, uintptr_t retaddr); >>>>> diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h >>>>> index 4475ef0996..e1d50b3c8b 100644 >>>>> --- a/include/hw/core/tcg-cpu-ops.h >>>>> +++ b/include/hw/core/tcg-cpu-ops.h >>>>> @@ -10,6 +10,8 @@ >>>>> #ifndef TCG_CPU_OPS_H >>>>> #define TCG_CPU_OPS_H >>>>> >>>>> +#include "hw/core/cpu.h" >>>> >>>> This include is circular. >>> >>> Yes, it's protected though, it was asked that way. >> >> Well, in my strong opinion, someone asked incorrectly. It's "harmless" because >> of the protection ifdefs, but it's Wrong because it has the potential to hide bugs. >> >> What is it that you thought you needed from core/cpu.h anyway? >> >>>> Are you sure that splitting out hw/core/tcg-cpu-ops.h from hw/core/cpu.h in >>>> patch 15 is even useful? >>> >>> it avoids a huge #ifdef CONFIG_TCG >> >> So? The question should be: is it useful on its own, and I think the answer to >> that is clearly not. Thus it should not pretend to be a standalone header file. >> >> >> r~ >> > > The whole point of the exercise is to sort out what is tcg specific and only compile it under CONFIG_TCG. > > Having everything inside cpu.h wrapped in a 100 line #ifdef is not particularly readable or discoverable, > so I think it is actually useful for understanding purposes to have it separate, > but that said, I don't feel strongly on this, as I intend to improve this in later series. > Should I return this file to the original state (without the extra #includes that pretend it to be a standalone header file, and call it tcg-cpu-ops.h.inc ? Ciao, Claudio
On 12/11/20 12:15 PM, Claudio Fontana wrote: > Should I return this file to the original state (without the extra #includes that pretend it to be a standalone header file, > and call it > > tcg-cpu-ops.h.inc > > ? If this header can work with qemu/typedefs.h, then no, because the circularity has been resolved. Otherwise, yes. Thanks, r~
On 12/11/20 7:22 PM, Richard Henderson wrote: > On 12/11/20 12:15 PM, Claudio Fontana wrote: >> Should I return this file to the original state (without the extra #includes that pretend it to be a standalone header file, >> and call it >> >> tcg-cpu-ops.h.inc >> >> ? > > If this header can work with qemu/typedefs.h, then no, because the circularity > has been resolved. Otherwise, yes. My editor got confused with TranslationBlock, which is why I asked to include its declaration. Easier to forward-declare TranslationBlock in qemu/typedefs.h? Regards, Phil.
On 12/11/20 7:26 PM, Philippe Mathieu-Daudé wrote: > On 12/11/20 7:22 PM, Richard Henderson wrote: >> On 12/11/20 12:15 PM, Claudio Fontana wrote: >>> Should I return this file to the original state (without the extra #includes that pretend it to be a standalone header file, >>> and call it >>> >>> tcg-cpu-ops.h.inc >>> >>> ? >> >> If this header can work with qemu/typedefs.h, then no, because the circularity >> has been resolved. Otherwise, yes. > > My editor got confused with TranslationBlock, which is why I asked > to include its declaration. > > Easier to forward-declare TranslationBlock in qemu/typedefs.h? > > Regards, > > Phil. > Hello Philippe, ok you propose to move the existing fwd declaration of TranslationBlock from cpu.h to qemu/typedefs.h . And what about #include "exec/memattrs.h"? I assume you propose to put struct MemTxAttrs there as a fwd declaration too, and this concludes our experiment here? Thanks, Claudio
On 12/11/20 7:51 PM, Claudio Fontana wrote: > On 12/11/20 7:26 PM, Philippe Mathieu-Daudé wrote: >> On 12/11/20 7:22 PM, Richard Henderson wrote: >>> On 12/11/20 12:15 PM, Claudio Fontana wrote: >>>> Should I return this file to the original state (without the extra #includes that pretend it to be a standalone header file, >>>> and call it >>>> >>>> tcg-cpu-ops.h.inc >>>> >>>> ? >>> >>> If this header can work with qemu/typedefs.h, then no, because the circularity >>> has been resolved. Otherwise, yes. >> >> My editor got confused with TranslationBlock, which is why I asked >> to include its declaration. >> >> Easier to forward-declare TranslationBlock in qemu/typedefs.h? >> >> Regards, >> >> Phil. >> > > Hello Philippe, > > ok you propose to move the existing fwd declaration of TranslationBlock from cpu.h to qemu/typedefs.h . I'll let that answer to Richard =) > > And what about #include "exec/memattrs.h"? > > I assume you propose to put struct MemTxAttrs there as a fwd declaration too, > > and this concludes our experiment here? Well, there is no circular problem here, right? (and Richard already reviewed patch #24 :P ) > > Thanks, > > Claudio >
On Fri, Dec 11, 2020 at 07:51:54PM +0100, Claudio Fontana wrote: > On 12/11/20 7:26 PM, Philippe Mathieu-Daudé wrote: > > On 12/11/20 7:22 PM, Richard Henderson wrote: > >> On 12/11/20 12:15 PM, Claudio Fontana wrote: > >>> Should I return this file to the original state (without the extra #includes that pretend it to be a standalone header file, > >>> and call it > >>> > >>> tcg-cpu-ops.h.inc > >>> > >>> ? > >> > >> If this header can work with qemu/typedefs.h, then no, because the circularity > >> has been resolved. Otherwise, yes. > > > > My editor got confused with TranslationBlock, which is why I asked > > to include its declaration. > > > > Easier to forward-declare TranslationBlock in qemu/typedefs.h? > > > > Regards, > > > > Phil. > > > > Hello Philippe, > > ok you propose to move the existing fwd declaration of TranslationBlock from cpu.h to qemu/typedefs.h . It seems simpler to just add a typedef struct TranslationBlock TranslationBlock; line to tcg-cpu-ops.h. Or, an even simpler solution: just use `struct TranslationBlock` instead of `TranslationBlock` in the declarations being moved to tcg-cpu-ops.h. We don't need to move declarations to typedefs.h anymore, because now the compilers we support don't warn about typedef redefinitions: https://lore.kernel.org/qemu-devel/20200914134636.GZ1618070@habkost.net/ > > And what about #include "exec/memattrs.h"? > > I assume you propose to put struct MemTxAttrs there as a fwd declaration too, This can't be done, because MemTxAttrs can't be an incomplete type in the code you are moving (the methods get a MemTxAttrs value, not a pointer). > > and this concludes our experiment here? > > Thanks, > > Claudio >
On 12/11/20 9:02 PM, Eduardo Habkost wrote: > On Fri, Dec 11, 2020 at 07:51:54PM +0100, Claudio Fontana wrote: >> On 12/11/20 7:26 PM, Philippe Mathieu-Daudé wrote: >>> On 12/11/20 7:22 PM, Richard Henderson wrote: >>>> On 12/11/20 12:15 PM, Claudio Fontana wrote: >>>>> Should I return this file to the original state (without the extra #includes that pretend it to be a standalone header file, >>>>> and call it >>>>> >>>>> tcg-cpu-ops.h.inc >>>>> >>>>> ? >>>> >>>> If this header can work with qemu/typedefs.h, then no, because the circularity >>>> has been resolved. Otherwise, yes. >>> >>> My editor got confused with TranslationBlock, which is why I asked >>> to include its declaration. >>> >>> Easier to forward-declare TranslationBlock in qemu/typedefs.h? >>> >>> Regards, >>> >>> Phil. >>> >> >> Hello Philippe, >> >> ok you propose to move the existing fwd declaration of TranslationBlock from cpu.h to qemu/typedefs.h . > > It seems simpler to just add a > > typedef struct TranslationBlock TranslationBlock; > > line to tcg-cpu-ops.h. > > Or, an even simpler solution: just use `struct TranslationBlock` > instead of `TranslationBlock` in the declarations being moved to > tcg-cpu-ops.h. > > We don't need to move declarations to typedefs.h anymore, because > now the compilers we support don't warn about typedef > redefinitions: > https://lore.kernel.org/qemu-devel/20200914134636.GZ1618070@habkost.net/ > > >> >> And what about #include "exec/memattrs.h"? >> >> I assume you propose to put struct MemTxAttrs there as a fwd declaration too, > > This can't be done, because MemTxAttrs can't be an incomplete > type in the code you are moving (the methods get a MemTxAttrs > value, not a pointer). I'm confused now on what we are trying to do: if we want the file to be a "proper header" or just a TCG-ops-only convenience split of cpu.h. I thought that we were only solving a highlighting issue in some editor (Philippe), and I wonder if these changes in qemu/typedef.h help with that? I tried adding both to qemu/typedef.h, and since cpu.h is the only user of the file, and it already includes memattrs.h, everything is fine. But here maybe you are proposing to make it a regular header, and include this instead of just hw/core/cpu.h in the targets? I am thinking whether it is the case to scrap this whole mess, make TCGCPUOps a pointer in CPUClass, and in the targets say for example: #include "tcg-cpu-ops.h" ... +static struct TCGCPUOps cris_tcg_ops = { + .initialize = cris_initialize_tcg, +}; + static void cris_cpu_class_init(ObjectClass *oc, void *data) { DeviceClass *dc = DEVICE_CLASS(oc); @@ -284,7 +292,7 @@ static void cris_cpu_class_init(ObjectClass *oc, void *data) cc->gdb_stop_before_watchpoint = true; cc->disas_set_info = cris_disas_set_info; - cc->tcg_ops.initialize = cris_initialize_tcg; + cc->tcg_ops = &cris_tcg_ops; } What do you all think of this? Thanks, Claudio > >> >> and this concludes our experiment here? >> >> Thanks, >> >> Claudio >> >
On 12/12/20 11:00 AM, Claudio Fontana wrote: > On 12/11/20 9:02 PM, Eduardo Habkost wrote: >> On Fri, Dec 11, 2020 at 07:51:54PM +0100, Claudio Fontana wrote: >>> On 12/11/20 7:26 PM, Philippe Mathieu-Daudé wrote: >>>> On 12/11/20 7:22 PM, Richard Henderson wrote: >>>>> On 12/11/20 12:15 PM, Claudio Fontana wrote: >>>>>> Should I return this file to the original state (without the extra #includes that pretend it to be a standalone header file, >>>>>> and call it >>>>>> >>>>>> tcg-cpu-ops.h.inc >>>>>> >>>>>> ? >>>>> >>>>> If this header can work with qemu/typedefs.h, then no, because the circularity >>>>> has been resolved. Otherwise, yes. >>>> >>>> My editor got confused with TranslationBlock, which is why I asked >>>> to include its declaration. >>>> >>>> Easier to forward-declare TranslationBlock in qemu/typedefs.h? >>>> >>>> Regards, >>>> >>>> Phil. >>>> >>> >>> Hello Philippe, >>> >>> ok you propose to move the existing fwd declaration of TranslationBlock from cpu.h to qemu/typedefs.h . >> >> It seems simpler to just add a >> >> typedef struct TranslationBlock TranslationBlock; >> >> line to tcg-cpu-ops.h. >> >> Or, an even simpler solution: just use `struct TranslationBlock` >> instead of `TranslationBlock` in the declarations being moved to >> tcg-cpu-ops.h. >> >> We don't need to move declarations to typedefs.h anymore, because >> now the compilers we support don't warn about typedef >> redefinitions: >> https://lore.kernel.org/qemu-devel/20200914134636.GZ1618070@habkost.net/ >> >> >>> >>> And what about #include "exec/memattrs.h"? >>> >>> I assume you propose to put struct MemTxAttrs there as a fwd declaration too, >> >> This can't be done, because MemTxAttrs can't be an incomplete >> type in the code you are moving (the methods get a MemTxAttrs >> value, not a pointer). > > > > I'm confused now on what we are trying to do: if we want the file to be a "proper header" or just a TCG-ops-only convenience split of cpu.h. > > I thought that we were only solving a highlighting issue in some editor (Philippe), > and I wonder if these changes in qemu/typedef.h help with that? > > I tried adding both to qemu/typedef.h, and since cpu.h is the only user of the file, and it already includes memattrs.h, everything is fine. > > But here maybe you are proposing to make it a regular header, and include this instead of just hw/core/cpu.h in the targets? > > I am thinking whether it is the case to scrap this whole mess, make TCGCPUOps a pointer in CPUClass, and in the targets say for example: > > #include "tcg-cpu-ops.h" > > ... > > +static struct TCGCPUOps cris_tcg_ops = { > + .initialize = cris_initialize_tcg, > +}; > + > static void cris_cpu_class_init(ObjectClass *oc, void *data) > { > DeviceClass *dc = DEVICE_CLASS(oc); > @@ -284,7 +292,7 @@ static void cris_cpu_class_init(ObjectClass *oc, void *data) > cc->gdb_stop_before_watchpoint = true; > > cc->disas_set_info = cris_disas_set_info; > - cc->tcg_ops.initialize = cris_initialize_tcg; > + cc->tcg_ops = &cris_tcg_ops; > } > > > What do you all think of this? > > Thanks, > > Claudio Not sure it solves all problems: the MMUAccessType is still a cpu.h enum, so we are back to the circular dependency. Will try the .inc in the next spin, and I hope that the discussion can go on from there, with Eduardo, Philippe and Richard laying out more clearly what your requirements are. Thanks, Claudio
On Sat, Dec 12, 2020 at 11:00:03AM +0100, Claudio Fontana wrote: > On 12/11/20 9:02 PM, Eduardo Habkost wrote: > > On Fri, Dec 11, 2020 at 07:51:54PM +0100, Claudio Fontana wrote: > >> On 12/11/20 7:26 PM, Philippe Mathieu-Daudé wrote: > >>> On 12/11/20 7:22 PM, Richard Henderson wrote: > >>>> On 12/11/20 12:15 PM, Claudio Fontana wrote: > >>>>> Should I return this file to the original state (without the extra #includes that pretend it to be a standalone header file, > >>>>> and call it > >>>>> > >>>>> tcg-cpu-ops.h.inc > >>>>> > >>>>> ? > >>>> > >>>> If this header can work with qemu/typedefs.h, then no, because the circularity > >>>> has been resolved. Otherwise, yes. > >>> > >>> My editor got confused with TranslationBlock, which is why I asked > >>> to include its declaration. > >>> > >>> Easier to forward-declare TranslationBlock in qemu/typedefs.h? > >>> > >>> Regards, > >>> > >>> Phil. > >>> > >> > >> Hello Philippe, > >> > >> ok you propose to move the existing fwd declaration of TranslationBlock from cpu.h to qemu/typedefs.h . > > > > It seems simpler to just add a > > > > typedef struct TranslationBlock TranslationBlock; > > > > line to tcg-cpu-ops.h. > > > > Or, an even simpler solution: just use `struct TranslationBlock` > > instead of `TranslationBlock` in the declarations being moved to > > tcg-cpu-ops.h. > > > > We don't need to move declarations to typedefs.h anymore, because > > now the compilers we support don't warn about typedef > > redefinitions: > > https://lore.kernel.org/qemu-devel/20200914134636.GZ1618070@habkost.net/ > > > > > >> > >> And what about #include "exec/memattrs.h"? > >> > >> I assume you propose to put struct MemTxAttrs there as a fwd declaration too, > > > > This can't be done, because MemTxAttrs can't be an incomplete > > type in the code you are moving (the methods get a MemTxAttrs > > value, not a pointer). > > > > I'm confused now on what we are trying to do: if we want the > file to be a "proper header" or just a TCG-ops-only convenience > split of cpu.h. Personally, I don't see the point of creating a new header if it's not a proper header. > > I thought that we were only solving a highlighting issue in some editor (Philippe), > and I wonder if these changes in qemu/typedef.h help with that? > > I tried adding both to qemu/typedef.h, and since cpu.h is the only user of the file, and it already includes memattrs.h, everything is fine. > > But here maybe you are proposing to make it a regular header, and include this instead of just hw/core/cpu.h in the targets? > > I am thinking whether it is the case to scrap this whole mess, make TCGCPUOps a pointer in CPUClass, and in the targets say for example: > > #include "tcg-cpu-ops.h" > > ... > > +static struct TCGCPUOps cris_tcg_ops = { > + .initialize = cris_initialize_tcg, > +}; > + > static void cris_cpu_class_init(ObjectClass *oc, void *data) > { > DeviceClass *dc = DEVICE_CLASS(oc); > @@ -284,7 +292,7 @@ static void cris_cpu_class_init(ObjectClass *oc, void *data) > cc->gdb_stop_before_watchpoint = true; > > cc->disas_set_info = cris_disas_set_info; > - cc->tcg_ops.initialize = cris_initialize_tcg; > + cc->tcg_ops = &cris_tcg_ops; > } > > > What do you all think of this? Making tcg_ops a pointer may make a lot of sense, but (as mentioned in my reply to v12) I'm worried by the scope of this series growing too much. I suggest doing this refactor in smaller steps, to let us focus in a single issue at a time. Instead of splitting the struct and creating a new header file in a single patch, you can first create the new struct in the same header, and worry about moving it to a separate header later (in the same series, or in another series).
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h index ea648d52ad..83007d262c 100644 --- a/include/hw/core/cpu.h +++ b/include/hw/core/cpu.h @@ -110,13 +110,6 @@ struct TranslationBlock; * If the target behaviour here is anything other than "set * the PC register to the value passed in" then the target must * also implement the synchronize_from_tb hook. - * @synchronize_from_tb: Callback for synchronizing state from a TCG - * #TranslationBlock. This is called when we abandon execution - * of a TB before starting it, and must set all parts of the CPU - * state which the previous TB in the chain may not have updated. - * This always includes at least the program counter; some targets - * will need to do more. If this hook is not implemented then the - * default is to call @set_pc(tb->pc). * @tlb_fill: Callback for handling a softmmu tlb miss or user-only * address fault. For system mode, if the access is valid, call * tlb_set_page and return true; if the access is invalid, and @@ -193,7 +186,6 @@ struct CPUClass { void (*get_memory_mapping)(CPUState *cpu, MemoryMappingList *list, Error **errp); void (*set_pc)(CPUState *cpu, vaddr value); - void (*synchronize_from_tb)(CPUState *cpu, struct TranslationBlock *tb); bool (*tlb_fill)(CPUState *cpu, vaddr address, int size, MMUAccessType access_type, int mmu_idx, bool probe, uintptr_t retaddr); diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h index 4475ef0996..e1d50b3c8b 100644 --- a/include/hw/core/tcg-cpu-ops.h +++ b/include/hw/core/tcg-cpu-ops.h @@ -10,6 +10,8 @@ #ifndef TCG_CPU_OPS_H #define TCG_CPU_OPS_H +#include "hw/core/cpu.h" + /** * struct TcgCpuOperations: TCG operations specific to a CPU class */ @@ -20,6 +22,16 @@ typedef struct TcgCpuOperations { * Called when the first CPU is realized. */ void (*initialize)(void); + /** + * @synchronize_from_tb: Synchronize state from a TCG #TranslationBlock + * + * This is called when we abandon execution of a TB before + * starting it, and must set all parts of the CPU state which + * the previous TB in the chain may not have updated. This + * will need to do more. If this hook is not implemented then + * the default is to call @set_pc(tb->pc). + */ + void (*synchronize_from_tb)(CPUState *cpu, struct TranslationBlock *tb); } TcgCpuOperations; #endif /* TCG_CPU_OPS_H */ diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c index 50eb92d217..05dba7f2cc 100644 --- a/accel/tcg/cpu-exec.c +++ b/accel/tcg/cpu-exec.c @@ -192,8 +192,8 @@ static inline tcg_target_ulong cpu_tb_exec(CPUState *cpu, TranslationBlock *itb) TARGET_FMT_lx "] %s\n", last_tb->tc.ptr, last_tb->pc, lookup_symbol(last_tb->pc)); - if (cc->synchronize_from_tb) { - cc->synchronize_from_tb(cpu, last_tb); + if (cc->tcg_ops.synchronize_from_tb) { + cc->tcg_ops.synchronize_from_tb(cpu, last_tb); } else { assert(cc->set_pc); cc->set_pc(cpu, last_tb->pc); diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 1fa9382a7c..ac1757d49f 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -54,6 +54,7 @@ static void arm_cpu_set_pc(CPUState *cs, vaddr value) } } +#ifdef CONFIG_TCG static void arm_cpu_synchronize_from_tb(CPUState *cs, TranslationBlock *tb) { ARMCPU *cpu = ARM_CPU(cs); @@ -69,6 +70,7 @@ static void arm_cpu_synchronize_from_tb(CPUState *cs, TranslationBlock *tb) env->regs[15] = tb->pc; } } +#endif /* CONFIG_TCG */ static bool arm_cpu_has_work(CPUState *cs) { @@ -2242,7 +2244,6 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data) cc->cpu_exec_interrupt = arm_cpu_exec_interrupt; cc->dump_state = arm_cpu_dump_state; cc->set_pc = arm_cpu_set_pc; - cc->synchronize_from_tb = arm_cpu_synchronize_from_tb; cc->gdb_read_register = arm_cpu_gdb_read_register; cc->gdb_write_register = arm_cpu_gdb_write_register; #ifndef CONFIG_USER_ONLY @@ -2262,6 +2263,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data) cc->disas_set_info = arm_disas_set_info; #ifdef CONFIG_TCG cc->tcg_ops.initialize = arm_translate_init; + cc->tcg_ops.synchronize_from_tb = arm_cpu_synchronize_from_tb; cc->tlb_fill = arm_cpu_tlb_fill; cc->debug_excp_handler = arm_debug_excp_handler; cc->debug_check_watchpoint = arm_debug_check_watchpoint; diff --git a/target/avr/cpu.c b/target/avr/cpu.c index 94306a2aa0..f753c15768 100644 --- a/target/avr/cpu.c +++ b/target/avr/cpu.c @@ -207,7 +207,7 @@ static void avr_cpu_class_init(ObjectClass *oc, void *data) cc->vmsd = &vms_avr_cpu; cc->disas_set_info = avr_cpu_disas_set_info; cc->tcg_ops.initialize = avr_cpu_tcg_init; - cc->synchronize_from_tb = avr_cpu_synchronize_from_tb; + cc->tcg_ops.synchronize_from_tb = avr_cpu_synchronize_from_tb; cc->gdb_read_register = avr_cpu_gdb_read_register; cc->gdb_write_register = avr_cpu_gdb_write_register; cc->gdb_num_core_regs = 35; diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c index 4c778966c2..12a09e93ae 100644 --- a/target/hppa/cpu.c +++ b/target/hppa/cpu.c @@ -143,7 +143,7 @@ static void hppa_cpu_class_init(ObjectClass *oc, void *data) cc->cpu_exec_interrupt = hppa_cpu_exec_interrupt; cc->dump_state = hppa_cpu_dump_state; cc->set_pc = hppa_cpu_set_pc; - cc->synchronize_from_tb = hppa_cpu_synchronize_from_tb; + cc->tcg_ops.synchronize_from_tb = hppa_cpu_synchronize_from_tb; cc->gdb_read_register = hppa_cpu_gdb_read_register; cc->gdb_write_register = hppa_cpu_gdb_write_register; cc->tlb_fill = hppa_cpu_tlb_fill; diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c index 1f2a3e881a..d1414e2970 100644 --- a/target/i386/tcg/tcg-cpu.c +++ b/target/i386/tcg/tcg-cpu.c @@ -60,7 +60,7 @@ void tcg_cpu_common_class_init(CPUClass *cc) { cc->do_interrupt = x86_cpu_do_interrupt; cc->cpu_exec_interrupt = x86_cpu_exec_interrupt; - cc->synchronize_from_tb = x86_cpu_synchronize_from_tb; + cc->tcg_ops.synchronize_from_tb = x86_cpu_synchronize_from_tb; cc->cpu_exec_enter = x86_cpu_exec_enter; cc->cpu_exec_exit = x86_cpu_exec_exit; cc->tcg_ops.initialize = tcg_x86_init; diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c index bc10518fa3..97d94d9c27 100644 --- a/target/microblaze/cpu.c +++ b/target/microblaze/cpu.c @@ -322,7 +322,7 @@ static void mb_cpu_class_init(ObjectClass *oc, void *data) cc->cpu_exec_interrupt = mb_cpu_exec_interrupt; cc->dump_state = mb_cpu_dump_state; cc->set_pc = mb_cpu_set_pc; - cc->synchronize_from_tb = mb_cpu_synchronize_from_tb; + cc->tcg_ops.synchronize_from_tb = mb_cpu_synchronize_from_tb; cc->gdb_read_register = mb_cpu_gdb_read_register; cc->gdb_write_register = mb_cpu_gdb_write_register; cc->tlb_fill = mb_cpu_tlb_fill; diff --git a/target/mips/cpu.c b/target/mips/cpu.c index bc48573763..4a539349a9 100644 --- a/target/mips/cpu.c +++ b/target/mips/cpu.c @@ -44,6 +44,7 @@ static void mips_cpu_set_pc(CPUState *cs, vaddr value) } } +#ifdef CONFIG_TCG static void mips_cpu_synchronize_from_tb(CPUState *cs, TranslationBlock *tb) { MIPSCPU *cpu = MIPS_CPU(cs); @@ -53,6 +54,7 @@ static void mips_cpu_synchronize_from_tb(CPUState *cs, TranslationBlock *tb) env->hflags &= ~MIPS_HFLAG_BMASK; env->hflags |= tb->flags & MIPS_HFLAG_BMASK; } +#endif /* CONFIG_TCG */ static bool mips_cpu_has_work(CPUState *cs) { @@ -238,7 +240,6 @@ static void mips_cpu_class_init(ObjectClass *c, void *data) cc->cpu_exec_interrupt = mips_cpu_exec_interrupt; cc->dump_state = mips_cpu_dump_state; cc->set_pc = mips_cpu_set_pc; - 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; #ifndef CONFIG_USER_ONLY @@ -250,6 +251,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_ops.initialize = mips_tcg_init; + cc->tcg_ops.synchronize_from_tb = mips_cpu_synchronize_from_tb; cc->tlb_fill = mips_cpu_tlb_fill; #endif diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index 27dd1645c9..a9c30879d3 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -543,7 +543,7 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data) cc->cpu_exec_interrupt = riscv_cpu_exec_interrupt; cc->dump_state = riscv_cpu_dump_state; cc->set_pc = riscv_cpu_set_pc; - cc->synchronize_from_tb = riscv_cpu_synchronize_from_tb; + cc->tcg_ops.synchronize_from_tb = riscv_cpu_synchronize_from_tb; cc->gdb_read_register = riscv_cpu_gdb_read_register; cc->gdb_write_register = riscv_cpu_gdb_write_register; cc->gdb_num_core_regs = 33; diff --git a/target/rx/cpu.c b/target/rx/cpu.c index a701a09b11..d03c4e0b05 100644 --- a/target/rx/cpu.c +++ b/target/rx/cpu.c @@ -189,7 +189,7 @@ static void rx_cpu_class_init(ObjectClass *klass, void *data) cc->cpu_exec_interrupt = rx_cpu_exec_interrupt; cc->dump_state = rx_cpu_dump_state; cc->set_pc = rx_cpu_set_pc; - cc->synchronize_from_tb = rx_cpu_synchronize_from_tb; + cc->tcg_ops.synchronize_from_tb = rx_cpu_synchronize_from_tb; cc->gdb_read_register = rx_cpu_gdb_read_register; cc->gdb_write_register = rx_cpu_gdb_write_register; cc->get_phys_page_debug = rx_cpu_get_phys_page_debug; diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c index bdc5c9d90b..a33025b5c8 100644 --- a/target/sh4/cpu.c +++ b/target/sh4/cpu.c @@ -222,7 +222,7 @@ static void superh_cpu_class_init(ObjectClass *oc, void *data) cc->cpu_exec_interrupt = superh_cpu_exec_interrupt; cc->dump_state = superh_cpu_dump_state; cc->set_pc = superh_cpu_set_pc; - cc->synchronize_from_tb = superh_cpu_synchronize_from_tb; + cc->tcg_ops.synchronize_from_tb = superh_cpu_synchronize_from_tb; cc->gdb_read_register = superh_cpu_gdb_read_register; cc->gdb_write_register = superh_cpu_gdb_write_register; cc->tlb_fill = superh_cpu_tlb_fill; diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c index 07e48b86d1..baf6c5b587 100644 --- a/target/sparc/cpu.c +++ b/target/sparc/cpu.c @@ -868,7 +868,7 @@ static void sparc_cpu_class_init(ObjectClass *oc, void *data) cc->memory_rw_debug = sparc_cpu_memory_rw_debug; #endif cc->set_pc = sparc_cpu_set_pc; - cc->synchronize_from_tb = sparc_cpu_synchronize_from_tb; + cc->tcg_ops.synchronize_from_tb = sparc_cpu_synchronize_from_tb; cc->gdb_read_register = sparc_cpu_gdb_read_register; cc->gdb_write_register = sparc_cpu_gdb_write_register; cc->tlb_fill = sparc_cpu_tlb_fill; diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c index 78b2925955..5edf96c600 100644 --- a/target/tricore/cpu.c +++ b/target/tricore/cpu.c @@ -162,7 +162,7 @@ static void tricore_cpu_class_init(ObjectClass *c, void *data) cc->dump_state = tricore_cpu_dump_state; cc->set_pc = tricore_cpu_set_pc; - cc->synchronize_from_tb = tricore_cpu_synchronize_from_tb; + cc->tcg_ops.synchronize_from_tb = tricore_cpu_synchronize_from_tb; cc->get_phys_page_debug = tricore_cpu_get_phys_page_debug; cc->tcg_ops.initialize = tricore_tcg_init; cc->tlb_fill = tricore_cpu_tlb_fill;