Re: [RFC PATCH 12/15] hw/m68k: Restrict M68kCPU type to target/ code

[Richard Henderson <richard.henderson@linaro.org>]

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~