[PATCH 0/4] include: Header cleanups around "cpu.h"

[PATCH 0/4] include: Header cleanups around "cpu.h"

[Philippe Mathieu-Daudé]

Few more cleanups related to "cpu.h" in "exec/cpu-all.h".

(I plan to merge that myself)

Philippe Mathieu-Daudé (4):
  target/ppc: Include missing headers in mmu-hash[32,64].h
  tcg/tci: Include missing 'disas/dis-asm.h' header
  exec/ram_addr: Include missing 'exec/hwaddr.h' and 'exec/cpu-common.h'
  exec/cpu-all: Include 'cpu.h' earlier so MMU_USER_IDX is always
    defined

 include/exec/cpu-all.h  | 5 ++++-
 include/exec/ram_addr.h | 3 +++
 target/ppc/mmu-hash32.h | 3 +++
 target/ppc/mmu-hash64.h | 2 ++
 tcg/tci.c               | 1 +
 5 files changed, 13 insertions(+), 1 deletion(-)

-- 
2.45.2

[PATCH 1/4] target/ppc: Include missing headers in mmu-hash[32,64].h

[Philippe Mathieu-Daudé]

CPUState* is dereferenced, so we need the structure definition
from "cpu.h". PowerPCCPU is declared in "cpu-qom.h". Include
them in order to avoid when refactoring:

  In file included from ../../target/ppc/cpu_init.c:27:
  target/ppc/mmu-hash32.h:6:23: error: unknown type name 'PowerPCCPU'
      6 | bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
        |                       ^
  target/ppc/mmu-hash32.h:66:15: error: incomplete definition of type 'struct ArchCPU'
     66 |     return cpu->env.spr[SPR_SDR1] & SDR_32_HTABORG;
        |            ~~~^
  target/ppc/mmu-hash64.h:173:36: error: unknown type name ‘PowerPCCPU’; did you mean ‘PowerPCCPUAlias’?
    173 | static inline void ppc_hash64_init(PowerPCCPU *cpu)
        |                                    ^~~~~~~~~~
        |                                    PowerPCCPUAlias

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/ppc/mmu-hash32.h | 3 +++
 target/ppc/mmu-hash64.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/target/ppc/mmu-hash32.h b/target/ppc/mmu-hash32.h
index 2838de031c7..abbff206d4e 100644
--- a/target/ppc/mmu-hash32.h
+++ b/target/ppc/mmu-hash32.h
@@ -3,6 +3,9 @@
 
 #ifndef CONFIG_USER_ONLY
 
+#include "target/ppc/cpu-qom.h"
+#include "cpu.h"
+
 bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
                       hwaddr *raddrp, int *psizep, int *protp, int mmu_idx,
                       bool guest_visible);
diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
index ae8d4b37aed..b32e17c2c58 100644
--- a/target/ppc/mmu-hash64.h
+++ b/target/ppc/mmu-hash64.h
@@ -3,6 +3,8 @@
 
 #ifndef CONFIG_USER_ONLY
 
+#include "target/ppc/cpu-qom.h"
+
 #ifdef TARGET_PPC64
 void dump_slb(PowerPCCPU *cpu);
 int ppc_store_slb(PowerPCCPU *cpu, target_ulong slot,
-- 
2.45.2

[PATCH 2/4] tcg/tci: Include missing 'disas/dis-asm.h' header

[Philippe Mathieu-Daudé]

"disas/dis-asm.h" defines bfd_vma and disassemble_info,
include it in order to avoid (when refactoring other
headers):

  tcg/tci.c:1066:20: error: unknown type name 'bfd_vma'
  int print_insn_tci(bfd_vma addr, disassemble_info *info)
                     ^
  tcg/tci.c:1066:34: error: unknown type name 'disassemble_info'
  int print_insn_tci(bfd_vma addr, disassemble_info *info)
                                   ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tcg/tci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tcg/tci.c b/tcg/tci.c
index 3afb2235285..3eb95e20b65 100644
--- a/tcg/tci.c
+++ b/tcg/tci.c
@@ -21,6 +21,7 @@
 #include "tcg/tcg.h"
 #include "tcg/helper-info.h"
 #include "tcg/tcg-ldst.h"
+#include "disas/dis-asm.h"
 #include <ffi.h>
 
 
-- 
2.45.2

[PATCH 3/4] exec/ram_addr: Include missing 'exec/hwaddr.h' and 'exec/cpu-common.h'

[Philippe Mathieu-Daudé]

'hwaddr' is defined in "exec/hwaddr.h", 'ram_addr_t' in
"exec/cpu-common.h". Include these headers in order to
avoid when refactoring unrelated headers:

  In file included from ../../hw/s390x/s390-virtio-ccw.c:17:
  include/sysemu/physmem-target.h:37:24: error: unknown type name 'hwaddr'
     37 |     (MemoryRegion *mr, hwaddr offset, hwaddr length, unsigned client);
        |                        ^
  In file included from ../../hw/s390x/s390-virtio-ccw.c:16:
  include/exec/ram_addr.h:52:36: error: unknown type name 'ram_addr_t'
     52 | RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
        |                                    ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/exec/ram_addr.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 53785cdb87c..ff157c1f42a 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -28,6 +28,9 @@
 #include "exec/exec-all.h"
 #include "qemu/rcu.h"
 
+#include "exec/hwaddr.h"
+#include "exec/cpu-common.h"
+
 extern uint64_t total_dirty_pages;
 
 /**
-- 
2.45.2

[PATCH 4/4] exec/cpu-all: Include 'cpu.h' earlier so MMU_USER_IDX is always defined

[Philippe Mathieu-Daudé]

Include "cpu.h" earlier to get the MMU_USER_IDX definition soon
enough and avoid when refactoring unrelated headers:

  In file included from include/exec/translator.h:271,
                   from ../../accel/tcg/translator.c:13:
  include/exec/cpu-all.h: In function ‘cpu_mmu_index’:
  include/exec/cpu-all.h:274:12: error: ‘MMU_USER_IDX’ undeclared (first use in this function)
    274 |     return MMU_USER_IDX;
        |            ^~~~~~~~~~~~
  include/exec/cpu-all.h:274:12: note: each undeclared identifier is reported only once for each function it appears in
  ninja: build stopped: subcommand failed.

We need to forward-declare cpu_mmu_index() to avoid on user emulation:

  In file included from include/exec/cpu-all.h:263,
                   from include/exec/translator.h:271,
                   from ../../accel/tcg/translator.c:13:
  ../../target/sparc/cpu.h: In function ‘cpu_get_tb_cpu_state’:
  ../../target/sparc/cpu.h:757:13: error: implicit declaration of function ‘cpu_mmu_index’ [-Werror=implicit-function-declaration]
    757 |     flags = cpu_mmu_index(env_cpu(env), false);
        |             ^~~~~~~~~~~~~

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/exec/cpu-all.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index f7eea33b101..09f537d06fa 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -180,8 +180,12 @@ CPUArchState *cpu_copy(CPUArchState *env);
      | CPU_INTERRUPT_TGT_EXT_3   \
      | CPU_INTERRUPT_TGT_EXT_4)
 
+#include "cpu.h"
+
 #ifdef CONFIG_USER_ONLY
 
+static inline int cpu_mmu_index(CPUState *cs, bool ifetch);
+
 /*
  * Allow some level of source compatibility with softmmu.  We do not
  * support any of the more exotic features, so only invalid pages may
@@ -271,7 +275,6 @@ static inline bool tlb_hit(uint64_t tlb_addr, vaddr addr)
 #endif /* !CONFIG_USER_ONLY */
 
 /* Validate correct placement of CPUArchState. */
-#include "cpu.h"
 QEMU_BUILD_BUG_ON(offsetof(ArchCPU, parent_obj) != 0);
 QEMU_BUILD_BUG_ON(offsetof(ArchCPU, env) != sizeof(CPUState));
 
-- 
2.45.2

Re: [PATCH 1/4] target/ppc: Include missing headers in mmu-hash[32,64].h

[Richard Henderson]

On 12/18/24 09:51, Philippe Mathieu-Daudé wrote:
> CPUState* is dereferenced, so we need the structure definition
> from "cpu.h". PowerPCCPU is declared in "cpu-qom.h". Include
> them in order to avoid when refactoring:
> 
>    In file included from ../../target/ppc/cpu_init.c:27:
>    target/ppc/mmu-hash32.h:6:23: error: unknown type name 'PowerPCCPU'
>        6 | bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
>          |                       ^
>    target/ppc/mmu-hash32.h:66:15: error: incomplete definition of type 'struct ArchCPU'
>       66 |     return cpu->env.spr[SPR_SDR1] & SDR_32_HTABORG;
>          |            ~~~^
>    target/ppc/mmu-hash64.h:173:36: error: unknown type name ‘PowerPCCPU’; did you mean ‘PowerPCCPUAlias’?
>      173 | static inline void ppc_hash64_init(PowerPCCPU *cpu)
>          |                                    ^~~~~~~~~~
>          |                                    PowerPCCPUAlias
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/ppc/mmu-hash32.h | 3 +++
>   target/ppc/mmu-hash64.h | 2 ++
>   2 files changed, 5 insertions(+)
> 
> diff --git a/target/ppc/mmu-hash32.h b/target/ppc/mmu-hash32.h
> index 2838de031c7..abbff206d4e 100644
> --- a/target/ppc/mmu-hash32.h
> +++ b/target/ppc/mmu-hash32.h
> @@ -3,6 +3,9 @@
>   
>   #ifndef CONFIG_USER_ONLY
>   
> +#include "target/ppc/cpu-qom.h"
> +#include "cpu.h"

cpu.h handles cpu-qom.h.
Do we really need both?


r~

> +
>   bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
>                         hwaddr *raddrp, int *psizep, int *protp, int mmu_idx,
>                         bool guest_visible);
> diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
> index ae8d4b37aed..b32e17c2c58 100644
> --- a/target/ppc/mmu-hash64.h
> +++ b/target/ppc/mmu-hash64.h
> @@ -3,6 +3,8 @@
>   
>   #ifndef CONFIG_USER_ONLY
>   
> +#include "target/ppc/cpu-qom.h"
> +
>   #ifdef TARGET_PPC64
>   void dump_slb(PowerPCCPU *cpu);
>   int ppc_store_slb(PowerPCCPU *cpu, target_ulong slot,

Re: [PATCH 0/4] include: Header cleanups around "cpu.h"

[Richard Henderson]

On 12/18/24 09:51, Philippe Mathieu-Daudé wrote:
> Philippe Mathieu-Daudé (4):
>    target/ppc: Include missing headers in mmu-hash[32,64].h
>    tcg/tci: Include missing 'disas/dis-asm.h' header
>    exec/ram_addr: Include missing 'exec/hwaddr.h' and 'exec/cpu-common.h'
>    exec/cpu-all: Include 'cpu.h' earlier so MMU_USER_IDX is always
>      defined

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Though, frankly these are hard to review in isolation.


r~

Re: [PATCH 1/4] target/ppc: Include missing headers in mmu-hash[32,64].h

[Philippe Mathieu-Daudé]

On 18/12/24 17:41, Richard Henderson wrote:
> On 12/18/24 09:51, Philippe Mathieu-Daudé wrote:
>> CPUState* is dereferenced, so we need the structure definition
>> from "cpu.h". PowerPCCPU is declared in "cpu-qom.h". Include
>> them in order to avoid when refactoring:
>>
>>    In file included from ../../target/ppc/cpu_init.c:27:
>>    target/ppc/mmu-hash32.h:6:23: error: unknown type name 'PowerPCCPU'
>>        6 | bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, 
>> MMUAccessType access_type,
>>          |                       ^
>>    target/ppc/mmu-hash32.h:66:15: error: incomplete definition of type 
>> 'struct ArchCPU'
>>       66 |     return cpu->env.spr[SPR_SDR1] & SDR_32_HTABORG;
>>          |            ~~~^
>>    target/ppc/mmu-hash64.h:173:36: error: unknown type name 
>> ‘PowerPCCPU’; did you mean ‘PowerPCCPUAlias’?
>>      173 | static inline void ppc_hash64_init(PowerPCCPU *cpu)
>>          |                                    ^~~~~~~~~~
>>          |                                    PowerPCCPUAlias
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   target/ppc/mmu-hash32.h | 3 +++
>>   target/ppc/mmu-hash64.h | 2 ++
>>   2 files changed, 5 insertions(+)
>>
>> diff --git a/target/ppc/mmu-hash32.h b/target/ppc/mmu-hash32.h
>> index 2838de031c7..abbff206d4e 100644
>> --- a/target/ppc/mmu-hash32.h
>> +++ b/target/ppc/mmu-hash32.h
>> @@ -3,6 +3,9 @@
>>   #ifndef CONFIG_USER_ONLY
>> +#include "target/ppc/cpu-qom.h"
>> +#include "cpu.h"
> 
> cpu.h handles cpu-qom.h.
> Do we really need both?

"cpu-qom.h" is the one I want. Currently we need "cpu.h" for all the
inlined functions dereferencing ArchCPU, like ppc_hash32_hpt_base(),
but long term it should be avoided in .h because sources become too
specific. Here we want to link the different MMU 32/64 in the same
PPC binary, having 2 different ArchCPU implementations. This patch
is quite old now so I don't remember well, I'll revisit and see if
it is still necessary.

>> +
>>   bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType 
>> access_type,
>>                         hwaddr *raddrp, int *psizep, int *protp, int 
>> mmu_idx,
>>                         bool guest_visible);
>> diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
>> index ae8d4b37aed..b32e17c2c58 100644
>> --- a/target/ppc/mmu-hash64.h
>> +++ b/target/ppc/mmu-hash64.h
>> @@ -3,6 +3,8 @@
>>   #ifndef CONFIG_USER_ONLY
>> +#include "target/ppc/cpu-qom.h"
>> +
>>   #ifdef TARGET_PPC64
>>   void dump_slb(PowerPCCPU *cpu);
>>   int ppc_store_slb(PowerPCCPU *cpu, target_ulong slot,
> 

Re: [PATCH 0/4] include: Header cleanups around "cpu.h"

[Philippe Mathieu-Daudé]

On 18/12/24 17:42, Richard Henderson wrote:
> On 12/18/24 09:51, Philippe Mathieu-Daudé wrote:
>> Philippe Mathieu-Daudé (4):
>>    target/ppc: Include missing headers in mmu-hash[32,64].h
>>    tcg/tci: Include missing 'disas/dis-asm.h' header
>>    exec/ram_addr: Include missing 'exec/hwaddr.h' and 'exec/cpu-common.h'
>>    exec/cpu-all: Include 'cpu.h' earlier so MMU_USER_IDX is always
>>      defined
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> Though, frankly these are hard to review in isolation.

Sorry for that. I'll send a PR with what I've already queued.
Next patches should be closer to the main tree.

Queuing patches 2-4.