Re: [Qemu-devel] [PATCH 23/28] numa: Don't include hw/boards.h into sysemu/numa.h

[Eduardo Habkost]

On Fri, Jul 26, 2019 at 02:05:37PM +0200, Markus Armbruster wrote:
> sysemu/numa.h includes hw/boards.h just for the CPUArchId typedef, at
> the cost of pulling in more than two dozen extra headers indirectly.
> 
> I could move the typedef from hw/boards.h to qemu/typedefs.h.  But
> it's used in just two headers: boards.h and numa.h.
> 
> I could move it to another header both its users include.
> exec/cpu-common.h seems to be the least bad fit.
> 
> But I'm keeping this simple & stupid: declare the struct tag in
> numa.h.
> 
> Cc: Eduardo Habkost <address@hidden>
> Cc: Marcel Apfelbaum <address@hidden>
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  exec.c                     | 1 +
>  hw/core/machine-qmp-cmds.c | 1 +
>  hw/core/machine.c          | 1 +
>  hw/mem/pc-dimm.c           | 1 +
>  include/hw/boards.h        | 2 +-
>  include/sysemu/numa.h      | 9 +++++++--
>  6 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 6d60fdfb1f..4d9e146c79 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -41,6 +41,7 @@
>  #if defined(CONFIG_USER_ONLY)
>  #include "qemu.h"
>  #else /* !CONFIG_USER_ONLY */
> +#include "hw/boards.h"
>  #include "exec/memory.h"
>  #include "exec/ioport.h"
>  #include "sysemu/dma.h"
> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> index c83e8954e1..d8284671f0 100644
> --- a/hw/core/machine-qmp-cmds.c
> +++ b/hw/core/machine-qmp-cmds.c
> @@ -9,6 +9,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "cpu.h"
> +#include "hw/boards.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-commands-machine.h"
>  #include "qapi/qmp/qerror.h"
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 2c9c93983a..901f3fa905 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -23,6 +23,7 @@
>  #include "sysemu/numa.h"
>  #include "qemu/error-report.h"
>  #include "sysemu/qtest.h"
> +#include "hw/boards.h"
>  #include "hw/pci/pci.h"
>  #include "hw/mem/nvdimm.h"
>  
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 0c83dcd61e..fa90d4fc6c 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -19,6 +19,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "hw/boards.h"
>  #include "hw/mem/pc-dimm.h"
>  #include "hw/qdev-properties.h"
>  #include "migration/vmstate.h"
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 67e551636a..739d109fe1 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -86,7 +86,7 @@ void machine_class_allow_dynamic_sysbus_dev(MachineClass 
> *mc, const char *type);
>   * @props - CPU object properties, initialized by board
>   * #vcpus_count - number of threads provided by @cpu object
>   */
> -typedef struct {
> +typedef struct CPUArchId {
>      uint64_t arch_id;
>      int64_t vcpus_count;
>      CpuInstanceProperties props;
> diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> index 01a263eba2..4c4c1dee9b 100644
> --- a/include/sysemu/numa.h
> +++ b/include/sysemu/numa.h
> @@ -4,7 +4,10 @@
>  #include "qemu/bitmap.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/hostmem.h"
> -#include "hw/boards.h"
> +#include "qapi/qapi-types-machine.h"

This seems to be needed because of NodeInfo.

> +#include "exec/cpu-common.h"

This seems to be needed because of ram_addr_t.

Reviewed-by: Eduardo Habkost <address@hidden>


> +
> +struct CPUArchId;
>  

I wonder if we should do the same for other struct types like
NodeInfo.

Why is it bad to require the inclusion of hw/boards.h just
because of CPUArchId, but acceptable to require the inclusion of
qapi-types-machine.h just to be able to write "NodeInfo *nodes"
instead of "struct NodeInfo *nodes" below?

>  extern int nb_numa_nodes;   /* Number of NUMA nodes */
>  extern bool have_numa_distance;
> @@ -32,5 +35,7 @@ void numa_legacy_auto_assign_ram(MachineClass *mc, NodeInfo 
> *nodes,
>                                   int nb_nodes, ram_addr_t size);
>  void numa_default_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
>                                    int nb_nodes, ram_addr_t size);
> -void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error 
> **errp);
> +void numa_cpu_pre_plug(const struct CPUArchId *slot, DeviceState *dev,
> +                       Error **errp);
> +
>  #endif
> -- 
> 2.21.0
> 

-- 
Eduardo