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~