| Message ID | 20220209215446.58402-13-f4bug@amsat.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target: Use ArchCPU & CPUArchState as abstract interface to target CPU | expand |
On 2/10/22 08:54, Philippe Mathieu-Daudé wrote: > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > include/hw/m68k/mcf.h | 3 +-- > target/m68k/cpu-qom.h | 2 -- > target/m68k/cpu.h | 4 ++-- > 3 files changed, 3 insertions(+), 6 deletions(-) > > diff --git a/include/hw/m68k/mcf.h b/include/hw/m68k/mcf.h > index 8cbd587bbf..e84fcfb4ca 100644 > --- a/include/hw/m68k/mcf.h > +++ b/include/hw/m68k/mcf.h > @@ -3,7 +3,6 @@ > /* Motorola ColdFire device prototypes. */ > > #include "exec/hwaddr.h" > -#include "target/m68k/cpu-qom.h" > > /* mcf_uart.c */ > uint64_t mcf_uart_read(void *opaque, hwaddr addr, > @@ -16,7 +15,7 @@ void mcf_uart_mm_init(hwaddr base, qemu_irq irq, Chardev *chr); > /* mcf_intc.c */ > qemu_irq *mcf_intc_init(struct MemoryRegion *sysmem, > hwaddr base, > - M68kCPU *cpu); > + ArchCPU *cpu); > > /* mcf5206.c */ > #define TYPE_MCF5206_MBAR "mcf5206-mbar" This part is ok. > diff --git a/target/m68k/cpu-qom.h b/target/m68k/cpu-qom.h > index c2c0736b3b..ec75adad69 100644 > --- a/target/m68k/cpu-qom.h > +++ b/target/m68k/cpu-qom.h > @@ -25,8 +25,6 @@ > > #define TYPE_M68K_CPU "m68k-cpu" > > -typedef struct ArchCPU M68kCPU; > - > OBJECT_DECLARE_TYPE(ArchCPU, M68kCPUClass, > M68K_CPU) > > diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h > index 872e8ce637..90be69e714 100644 > --- a/target/m68k/cpu.h > +++ b/target/m68k/cpu.h > @@ -156,14 +156,14 @@ typedef struct CPUArchState { > * > * A Motorola 68k CPU. > */ > -struct ArchCPU { > +typedef struct ArchCPU { > /*< private >*/ > CPUState parent_obj; > /*< public >*/ > > CPUNegativeOffsetState neg; > CPUM68KState env; > -}; > +} M68kCPU; I don't like these. Rationale? r~
On 9/2/22 23:50, Richard Henderson wrote: > On 2/10/22 08:54, Philippe Mathieu-Daudé wrote: >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> include/hw/m68k/mcf.h | 3 +-- >> target/m68k/cpu-qom.h | 2 -- >> target/m68k/cpu.h | 4 ++-- >> 3 files changed, 3 insertions(+), 6 deletions(-) >> >> diff --git a/include/hw/m68k/mcf.h b/include/hw/m68k/mcf.h >> index 8cbd587bbf..e84fcfb4ca 100644 >> --- a/include/hw/m68k/mcf.h >> +++ b/include/hw/m68k/mcf.h >> @@ -3,7 +3,6 @@ >> /* Motorola ColdFire device prototypes. */ >> #include "exec/hwaddr.h" >> -#include "target/m68k/cpu-qom.h" >> /* mcf_uart.c */ >> uint64_t mcf_uart_read(void *opaque, hwaddr addr, >> @@ -16,7 +15,7 @@ void mcf_uart_mm_init(hwaddr base, qemu_irq irq, >> Chardev *chr); >> /* mcf_intc.c */ >> qemu_irq *mcf_intc_init(struct MemoryRegion *sysmem, >> hwaddr base, >> - M68kCPU *cpu); >> + ArchCPU *cpu); >> /* mcf5206.c */ >> #define TYPE_MCF5206_MBAR "mcf5206-mbar" > > This part is ok. > >> diff --git a/target/m68k/cpu-qom.h b/target/m68k/cpu-qom.h >> index c2c0736b3b..ec75adad69 100644 >> --- a/target/m68k/cpu-qom.h >> +++ b/target/m68k/cpu-qom.h >> @@ -25,8 +25,6 @@ >> #define TYPE_M68K_CPU "m68k-cpu" >> -typedef struct ArchCPU M68kCPU; >> - >> OBJECT_DECLARE_TYPE(ArchCPU, M68kCPUClass, >> M68K_CPU) >> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h >> index 872e8ce637..90be69e714 100644 >> --- a/target/m68k/cpu.h >> +++ b/target/m68k/cpu.h >> @@ -156,14 +156,14 @@ typedef struct CPUArchState { >> * >> * A Motorola 68k CPU. >> */ >> -struct ArchCPU { >> +typedef struct ArchCPU { >> /*< private >*/ >> CPUState parent_obj; >> /*< public >*/ >> CPUNegativeOffsetState neg; >> CPUM68KState env; >> -}; >> +} M68kCPU; > > I don't like these. Rationale? Short-term idea: hw/ models only have access to cpu-qom.h declarations and opaque pointers to generic CPU objects. hw/ should not include cpu.h at all. By restricting FooCPU to target/ code, hw/ files fail to compile if using FooCPU and not ArchCPU. Long-term idea, each target/ is built as a module, exposing an uniform arch-API. I'm still prototyping to see how to disentangle arch-specific hw which access CPU internals (such ARM NVIC or MIPS ITU).
On 2/10/22 10:09, Philippe Mathieu-Daudé wrote: > On 9/2/22 23:50, Richard Henderson wrote: >> On 2/10/22 08:54, Philippe Mathieu-Daudé wrote: >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>> --- >>> include/hw/m68k/mcf.h | 3 +-- >>> target/m68k/cpu-qom.h | 2 -- >>> target/m68k/cpu.h | 4 ++-- >>> 3 files changed, 3 insertions(+), 6 deletions(-) >>> >>> diff --git a/include/hw/m68k/mcf.h b/include/hw/m68k/mcf.h >>> index 8cbd587bbf..e84fcfb4ca 100644 >>> --- a/include/hw/m68k/mcf.h >>> +++ b/include/hw/m68k/mcf.h >>> @@ -3,7 +3,6 @@ >>> /* Motorola ColdFire device prototypes. */ >>> #include "exec/hwaddr.h" >>> -#include "target/m68k/cpu-qom.h" >>> /* mcf_uart.c */ >>> uint64_t mcf_uart_read(void *opaque, hwaddr addr, >>> @@ -16,7 +15,7 @@ void mcf_uart_mm_init(hwaddr base, qemu_irq irq, Chardev *chr); >>> /* mcf_intc.c */ >>> qemu_irq *mcf_intc_init(struct MemoryRegion *sysmem, >>> hwaddr base, >>> - M68kCPU *cpu); >>> + ArchCPU *cpu); >>> /* mcf5206.c */ >>> #define TYPE_MCF5206_MBAR "mcf5206-mbar" >> >> This part is ok. >> >>> diff --git a/target/m68k/cpu-qom.h b/target/m68k/cpu-qom.h >>> index c2c0736b3b..ec75adad69 100644 >>> --- a/target/m68k/cpu-qom.h >>> +++ b/target/m68k/cpu-qom.h >>> @@ -25,8 +25,6 @@ >>> #define TYPE_M68K_CPU "m68k-cpu" >>> -typedef struct ArchCPU M68kCPU; >>> - >>> OBJECT_DECLARE_TYPE(ArchCPU, M68kCPUClass, >>> M68K_CPU) >>> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h >>> index 872e8ce637..90be69e714 100644 >>> --- a/target/m68k/cpu.h >>> +++ b/target/m68k/cpu.h >>> @@ -156,14 +156,14 @@ typedef struct CPUArchState { >>> * >>> * A Motorola 68k CPU. >>> */ >>> -struct ArchCPU { >>> +typedef struct ArchCPU { >>> /*< private >*/ >>> CPUState parent_obj; >>> /*< public >*/ >>> CPUNegativeOffsetState neg; >>> CPUM68KState env; >>> -}; >>> +} M68kCPU; >> >> I don't like these. Rationale? > > Short-term idea: hw/ models only have access to cpu-qom.h declarations > and opaque pointers to generic CPU objects. > > hw/ should not include cpu.h at all. By restricting FooCPU to target/ > code, hw/ files fail to compile if using FooCPU and not ArchCPU. Yes, that would be ideal. If you do want to bring the typedef into cpu.h, please keep it separate; it's easier to read. Especially since one normally expects typedef struct Foo { ... } Foo; and that's not what's happening here. > Long-term idea, each target/ is built as a module, exposing an uniform > arch-API. That would be awesome, yes. > I'm still prototyping to see how to disentangle arch-specific hw which > access CPU internals (such ARM NVIC or MIPS ITU). Complicated, yes. If it comes to it, I would not be opposed to having these tightly coupled devices live in target/, but let's see if you can avoid it. r~
diff --git a/include/hw/m68k/mcf.h b/include/hw/m68k/mcf.h index 8cbd587bbf..e84fcfb4ca 100644 --- a/include/hw/m68k/mcf.h +++ b/include/hw/m68k/mcf.h @@ -3,7 +3,6 @@ /* Motorola ColdFire device prototypes. */ #include "exec/hwaddr.h" -#include "target/m68k/cpu-qom.h" /* mcf_uart.c */ uint64_t mcf_uart_read(void *opaque, hwaddr addr, @@ -16,7 +15,7 @@ void mcf_uart_mm_init(hwaddr base, qemu_irq irq, Chardev *chr); /* mcf_intc.c */ qemu_irq *mcf_intc_init(struct MemoryRegion *sysmem, hwaddr base, - M68kCPU *cpu); + ArchCPU *cpu); /* mcf5206.c */ #define TYPE_MCF5206_MBAR "mcf5206-mbar" diff --git a/target/m68k/cpu-qom.h b/target/m68k/cpu-qom.h index c2c0736b3b..ec75adad69 100644 --- a/target/m68k/cpu-qom.h +++ b/target/m68k/cpu-qom.h @@ -25,8 +25,6 @@ #define TYPE_M68K_CPU "m68k-cpu" -typedef struct ArchCPU M68kCPU; - OBJECT_DECLARE_TYPE(ArchCPU, M68kCPUClass, M68K_CPU) diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h index 872e8ce637..90be69e714 100644 --- a/target/m68k/cpu.h +++ b/target/m68k/cpu.h @@ -156,14 +156,14 @@ typedef struct CPUArchState { * * A Motorola 68k CPU. */ -struct ArchCPU { +typedef struct ArchCPU { /*< private >*/ CPUState parent_obj; /*< public >*/ CPUNegativeOffsetState neg; CPUM68KState env; -}; +} M68kCPU; #ifndef CONFIG_USER_ONLY
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- include/hw/m68k/mcf.h | 3 +-- target/m68k/cpu-qom.h | 2 -- target/m68k/cpu.h | 4 ++-- 3 files changed, 3 insertions(+), 6 deletions(-)