[PATCH 0/5] target/i386: Header cleanups around "cpu.h"

[PATCH 0/5] target/i386: Header cleanups around "cpu.h"

[Philippe Mathieu-Daudé]

These patches are part of a big refactor cleanup
around "cpu.h". Most changes should be trivial IMHO.

Philippe Mathieu-Daudé (5):
  target/i386: Remove NEED_CPU_H guard from target-specific headers
  target/i386/cpu: Remove dead helper_lock() declaration
  target/i386/ops_sse: Include missing "cpu.h" header
  target/i386: Remove x86_cpu_dump_local_apic_state() dead stub
  hw/i386/x86: Reduce init_topo_info() scope

 hw/i386/x86.c                | 2 +-
 include/hw/i386/x86.h        | 3 ---
 target/i386/cpu-dump.c       | 5 +----
 target/i386/cpu.h            | 7 ++++---
 target/i386/hax/hax-i386.h   | 2 --
 target/i386/hvf/hvf-i386.h   | 4 ----
 target/i386/ops_sse.h        | 1 +
 target/i386/ops_sse_header.h | 3 +++
 8 files changed, 10 insertions(+), 17 deletions(-)

-- 
2.38.1

[PATCH 1/5] target/i386: Remove NEED_CPU_H guard from target-specific headers

[Philippe Mathieu-Daudé]

NEED_CPU_H is always defined for these target-specific headers.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/i386/hax/hax-i386.h | 2 --
 target/i386/hvf/hvf-i386.h | 4 ----
 2 files changed, 6 deletions(-)

diff --git a/target/i386/hax/hax-i386.h b/target/i386/hax/hax-i386.h
index efbb346238..409ebdb4af 100644
--- a/target/i386/hax/hax-i386.h
+++ b/target/i386/hax/hax-i386.h
@@ -49,7 +49,6 @@ struct hax_vm {
     struct hax_vcpu_state **vcpus;
 };
 
-#ifdef NEED_CPU_H
 /* Functions exported to host specific mode */
 hax_fd hax_vcpu_get_fd(CPUArchState *env);
 int valid_hax_tunnel_size(uint16_t size);
@@ -66,7 +65,6 @@ int hax_sync_vcpu_state(CPUArchState *env, struct vcpu_state_t *state,
                         int set);
 int hax_sync_msr(CPUArchState *env, struct hax_msr_data *msrs, int set);
 int hax_sync_fpu(CPUArchState *env, struct fx_layout *fl, int set);
-#endif
 
 int hax_vm_destroy(struct hax_vm *vm);
 int hax_capability(struct hax_state *hax, struct hax_capabilityinfo *cap);
diff --git a/target/i386/hvf/hvf-i386.h b/target/i386/hvf/hvf-i386.h
index 76e9235524..95b47c1c2e 100644
--- a/target/i386/hvf/hvf-i386.h
+++ b/target/i386/hvf/hvf-i386.h
@@ -24,11 +24,7 @@
 
 void hvf_handle_io(CPUArchState *, uint16_t, void *, int, int, int);
 
-#ifdef NEED_CPU_H
-/* Functions exported to host specific mode */
-
 /* Host specific functions */
 int hvf_inject_interrupt(CPUArchState *env, int vector);
-#endif
 
 #endif
-- 
2.38.1

Re: [PATCH 1/5] target/i386: Remove NEED_CPU_H guard from target-specific headers

[Richard Henderson]

On 12/16/22 14:01, Philippe Mathieu-Daudé wrote:
> NEED_CPU_H is always defined for these target-specific headers.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   target/i386/hax/hax-i386.h | 2 --
>   target/i386/hvf/hvf-i386.h | 4 ----
>   2 files changed, 6 deletions(-)

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

r~

[PATCH 2/5] target/i386/cpu: Remove dead helper_lock() declaration

[Philippe Mathieu-Daudé]

Missed in commit 37b995f6e7 ("target-i386: remove helper_lock()").

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/i386/cpu.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index f729e0f09c..9824b7f8f2 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2313,9 +2313,6 @@ static inline void cpu_set_fpuc(CPUX86State *env, uint16_t fpuc)
      }
 }
 
-/* mem_helper.c */
-void helper_lock_init(void);
-
 /* svm_helper.c */
 #ifdef CONFIG_USER_ONLY
 static inline void
-- 
2.38.1

Re: [PATCH 2/5] target/i386/cpu: Remove dead helper_lock() declaration

[Richard Henderson]

On 12/16/22 14:01, Philippe Mathieu-Daudé wrote:
> Missed in commit 37b995f6e7 ("target-i386: remove helper_lock()").
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   target/i386/cpu.h | 3 ---
>   1 file changed, 3 deletions(-)

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

r~

[PATCH 3/5] target/i386/ops_sse: Include missing "cpu.h" header

[Philippe Mathieu-Daudé]

'Reg' is defined as 'MMXReg', itself declared in "cpu.h".

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/i386/ops_sse.h        | 1 +
 target/i386/ops_sse_header.h | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/target/i386/ops_sse.h b/target/i386/ops_sse.h
index 3cbc36a59d..68c88c9ee3 100644
--- a/target/i386/ops_sse.h
+++ b/target/i386/ops_sse.h
@@ -19,6 +19,7 @@
  */
 
 #include "crypto/aes.h"
+#include "cpu.h"
 
 #if SHIFT == 0
 #define Reg MMXReg
diff --git a/target/i386/ops_sse_header.h b/target/i386/ops_sse_header.h
index 8a7b2f4e2f..5631a6d1e1 100644
--- a/target/i386/ops_sse_header.h
+++ b/target/i386/ops_sse_header.h
@@ -16,6 +16,9 @@
  * You should have received a copy of the GNU Lesser General Public
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
+
+#include "cpu.h"
+
 #if SHIFT == 0
 #define Reg MMXReg
 #define SUFFIX _mmx
-- 
2.38.1

Re: [PATCH 3/5] target/i386/ops_sse: Include missing "cpu.h" header

[Richard Henderson]

On 12/16/22 14:01, Philippe Mathieu-Daudé wrote:
> 'Reg' is defined as 'MMXReg', itself declared in "cpu.h".
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/i386/ops_sse.h        | 1 +
>   target/i386/ops_sse_header.h | 3 +++
>   2 files changed, 4 insertions(+)

No, both of these are multiply included.
If you like, rename them "*.inc.h".


r~

[PATCH 4/5] target/i386: Remove x86_cpu_dump_local_apic_state() dead stub

[Philippe Mathieu-Daudé]

x86_cpu_dump_local_apic_state() is called from monitor.c which
is only compiled for system emulation since commit bf95728400
("monitor: remove target-specific code from monitor.c").

Interestingly this stub was added few weeks later in commit
1f871d49e3 ("hmp: added local apic dump state") and was not
necessary by that time.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/i386/cpu-dump.c | 5 +----
 target/i386/cpu.h      | 4 ++++
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/target/i386/cpu-dump.c b/target/i386/cpu-dump.c
index 08ac957e99..40697064d9 100644
--- a/target/i386/cpu-dump.c
+++ b/target/i386/cpu-dump.c
@@ -335,10 +335,7 @@ void x86_cpu_dump_local_apic_state(CPUState *cs, int flags)
     }
     qemu_printf(" PPR 0x%02x\n", apic_get_ppr(s));
 }
-#else
-void x86_cpu_dump_local_apic_state(CPUState *cs, int flags)
-{
-}
+
 #endif /* !CONFIG_USER_ONLY */
 
 #define DUMP_CODE_BYTES_TOTAL    50
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 9824b7f8f2..32d048f326 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2355,12 +2355,16 @@ typedef int X86CPUVersion;
  */
 void x86_cpu_set_default_version(X86CPUVersion version);
 
+#ifndef CONFIG_USER_ONLY
+
 #define APIC_DEFAULT_ADDRESS 0xfee00000
 #define APIC_SPACE_SIZE      0x100000
 
 /* cpu-dump.c */
 void x86_cpu_dump_local_apic_state(CPUState *cs, int flags);
 
+#endif
+
 /* cpu.c */
 bool cpu_is_bsp(X86CPU *cpu);
 
-- 
2.38.1

Re: [PATCH 4/5] target/i386: Remove x86_cpu_dump_local_apic_state() dead stub

[Richard Henderson]

On 12/16/22 14:01, Philippe Mathieu-Daudé wrote:
> x86_cpu_dump_local_apic_state() is called from monitor.c which
> is only compiled for system emulation since commit bf95728400
> ("monitor: remove target-specific code from monitor.c").
> 
> Interestingly this stub was added few weeks later in commit
> 1f871d49e3 ("hmp: added local apic dump state") and was not
> necessary by that time.

Crossed in-flight, presumably.

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


r~

[PATCH 5/5] hw/i386/x86: Reduce init_topo_info() scope

[Philippe Mathieu-Daudé]

This function is not used anywhere outside this file, so
we can delete the prototype from include/hw/i386/x86.h and
make the function "static void".

This fixes when building with -Wall and using Clang
("Apple clang version 14.0.0 (clang-1400.0.29.202)"):

  ../hw/i386/x86.c:70:24: error: static function 'MACHINE' is used in an inline function with external linkage [-Werror,-Wstatic-in-inline]
      MachineState *ms = MACHINE(x86ms);
                         ^
  include/hw/i386/x86.h:101:1: note: use 'static' to give inline function 'init_topo_info' internal linkage
  void init_topo_info(X86CPUTopoInfo *topo_info, const X86MachineState *x86ms);
  ^
  static
  include/hw/boards.h:24:49: note: 'MACHINE' declared here
  OBJECT_DECLARE_TYPE(MachineState, MachineClass, MACHINE)
                                                  ^

Reported-by: Stefan Weil <sw@weilnetz.de>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/x86.c         | 2 +-
 include/hw/i386/x86.h | 3 ---
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 78cc131926..b6ba0b2d00 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -64,7 +64,7 @@
 /* Physical Address of PVH entry point read from kernel ELF NOTE */
 static size_t pvh_start_addr;
 
-inline void init_topo_info(X86CPUTopoInfo *topo_info,
+static void init_topo_info(X86CPUTopoInfo *topo_info,
                            const X86MachineState *x86ms)
 {
     MachineState *ms = MACHINE(x86ms);
diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
index 62fa5774f8..5d3047a1d1 100644
--- a/include/hw/i386/x86.h
+++ b/include/hw/i386/x86.h
@@ -20,7 +20,6 @@
 #include "exec/hwaddr.h"
 #include "qemu/notify.h"
 
-#include "hw/i386/topology.h"
 #include "hw/boards.h"
 #include "hw/nmi.h"
 #include "hw/isa/isa.h"
@@ -98,8 +97,6 @@ struct X86MachineState {
 #define TYPE_X86_MACHINE   MACHINE_TYPE_NAME("x86")
 OBJECT_DECLARE_TYPE(X86MachineState, X86MachineClass, X86_MACHINE)
 
-void init_topo_info(X86CPUTopoInfo *topo_info, const X86MachineState *x86ms);
-
 uint32_t x86_cpu_apic_id_from_index(X86MachineState *pcms,
                                     unsigned int cpu_index);
 
-- 
2.38.1

Re: [PATCH 5/5] hw/i386/x86: Reduce init_topo_info() scope

[Richard Henderson]

On 12/16/22 14:01, Philippe Mathieu-Daudé wrote:
> This function is not used anywhere outside this file, so
> we can delete the prototype from include/hw/i386/x86.h and
> make the function "static void".
> 
> This fixes when building with -Wall and using Clang
> ("Apple clang version 14.0.0 (clang-1400.0.29.202)"):
> 
>    ../hw/i386/x86.c:70:24: error: static function 'MACHINE' is used in an inline function with external linkage [-Werror,-Wstatic-in-inline]
>        MachineState *ms = MACHINE(x86ms);
>                           ^
>    include/hw/i386/x86.h:101:1: note: use 'static' to give inline function 'init_topo_info' internal linkage
>    void init_topo_info(X86CPUTopoInfo *topo_info, const X86MachineState *x86ms);
>    ^
>    static
>    include/hw/boards.h:24:49: note: 'MACHINE' declared here
>    OBJECT_DECLARE_TYPE(MachineState, MachineClass, MACHINE)
>                                                    ^
> 
> Reported-by: Stefan Weil<sw@weilnetz.de>
> Suggested-by: Peter Maydell<peter.maydell@linaro.org>
> Reviewed-by: Peter Maydell<peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/i386/x86.c         | 2 +-
>   include/hw/i386/x86.h | 3 ---
>   2 files changed, 1 insertion(+), 4 deletions(-)

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

r~