diff mbox series

[23/28] numa: Don't include hw/boards.h into sysemu/numa.h

Message ID 20190726120542.9894-24-armbru@redhat.com
State New
Headers show
Series Tame a few "touch this, recompile the world" headers | expand

Commit Message

Markus Armbruster July 26, 2019, 12:05 p.m. UTC
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 <ehabkost@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 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(-)

Comments

Eduardo Habkost July 29, 2019, 7:44 p.m. UTC | #1
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 <ehabkost@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  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 <ehabkost@redhat.com>


> +
> +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
>
Markus Armbruster July 30, 2019, 11:01 a.m. UTC | #2
Cc'ing a few more people who might be interested.

Eduardo Habkost <ehabkost@redhat.com> writes:

> 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 <ehabkost@redhat.com>
>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  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.

NumaOptions, actually.

>> +#include "exec/cpu-common.h"
>
> This seems to be needed because of ram_addr_t.

Correct.

> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>
>
>> +
>> +struct CPUArchId;
>>  
>
> I wonder if we should do the same for other struct types like
> NodeInfo.

NumaOptions, I think.

> 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?

hw/boards.h is worse.  Both headers pull in qapi/qapi-builtin-types.h
and qapi/qapi-types-common.h, but hw/boards.h pulls in ~60 more.

That doesn't mean including qapi/qapi-types-common.h is good.

For better or worse[*], our coding style mandates typedefs.

We generally declare a typedef name in exactly one place.  The obvious
place is together with the type it denotes.

Non-local users of the type then need to include the header that
declares the typedef name.

This can lead to inclusion cycles, in particular for complete struct and
union types that need more types for their members.

We move typedefs to qemu/typedefs.h to break such cycles.

We also do it to include less, for less frequent recompilation.  As this
series demonstrates, we're not very good at that.  When to put a typedef
in qemu/typedefs.h for this purpose is not obvious.  If we simply put
all of them there, we'd recompile the world every time we add a typedef,
which is pretty much exactly what we're trying to avoid.

Some of qemu/typedefs.h's typedefs are used in dozens or even hundreds
of other headers.  Quite a few only in one.  The latter likely should
not be there.

We occasionally give up and use types directly rather than their typedef
names, flouting the coding style.  This patch does.  Trades messing with
qemu/typedefs.h for having to write 'struct' a few times.

Should we give up more?  Or not at all?

Can we have some guidelines on proper use of qemu/typedefs.h?

>>  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
>> 


[*] Worse if you ask me.
Eric Blake July 30, 2019, 1:15 p.m. UTC | #3
On 7/30/19 6:01 AM, Markus Armbruster wrote:
> Cc'ing a few more people who might be interested.
> 
> Eduardo Habkost <ehabkost@redhat.com> writes:

>> 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?
> 
> hw/boards.h is worse.  Both headers pull in qapi/qapi-builtin-types.h
> and qapi/qapi-types-common.h, but hw/boards.h pulls in ~60 more.
> 
> That doesn't mean including qapi/qapi-types-common.h is good.
> 
> For better or worse[*], our coding style mandates typedefs.

I could live with a switch to kernel style of always writing the
'struct' at every use, instead of using typedefs; but it would have to
be a flag-day switchover, preferably aided by Coccinelle.  But I'm not
holding my breath for it happening.

> 
> We generally declare a typedef name in exactly one place.  The obvious
> place is together with the type it denotes.
> 
> Non-local users of the type then need to include the header that
> declares the typedef name.
> 
> This can lead to inclusion cycles, in particular for complete struct and
> union types that need more types for their members.
> 
> We move typedefs to qemu/typedefs.h to break such cycles.
> 
> We also do it to include less, for less frequent recompilation.  As this
> series demonstrates, we're not very good at that.  When to put a typedef
> in qemu/typedefs.h for this purpose is not obvious.  If we simply put
> all of them there, we'd recompile the world every time we add a typedef,
> which is pretty much exactly what we're trying to avoid.
> 
> Some of qemu/typedefs.h's typedefs are used in dozens or even hundreds
> of other headers.  Quite a few only in one.  The latter likely should
> not be there.
> 
> We occasionally give up and use types directly rather than their typedef
> names, flouting the coding style.  This patch does.  Trades messing with
> qemu/typedefs.h for having to write 'struct' a few times.
> 
> Should we give up more?  Or not at all?
> 
> Can we have some guidelines on proper use of qemu/typedefs.h?

How hard would it be to compute which typedefs already in
qemu/typedefs.h are necessary to avoid cyclic inclusion?
Paolo Bonzini July 30, 2019, 1:28 p.m. UTC | #4
On 30/07/19 15:15, Eric Blake wrote:
>> We occasionally give up and use types directly rather than their typedef
>> names, flouting the coding style.  This patch does.  Trades messing with
>> qemu/typedefs.h for having to write 'struct' a few times.

I think Markus made the right call here.  Using "struct Foo;" in headers
is a null price to pay if all you need is declaring a pointer-typed
field or parameter.  Of course this doesn't apply if you have to embed a
struct directly, but then qemu/typedefs.h wouldn't help either.

In general unless you're adding a new subsystem, qemu/typedefs.h should
only decrease in size, never increase.  (And there are certainly many
cases where typedefs.h are not needed, but cleaning that up is
understandably not high on the todo list).

Paolo
Eduardo Habkost July 30, 2019, 8:55 p.m. UTC | #5
On Tue, Jul 30, 2019 at 08:15:12AM -0500, Eric Blake wrote:
> How hard would it be to compute which typedefs already in
> qemu/typedefs.h are necessary to avoid cyclic inclusion?

I don't think it's just about cyclic inclusion.  It's also about
avoiding dependencies between headers just because of a single
typedef, like numa.h including hw/boards.h just because of the
CPUArchId typedef.
Markus Armbruster July 31, 2019, 6:37 a.m. UTC | #6
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 30/07/19 15:15, Eric Blake wrote:
>>> We occasionally give up and use types directly rather than their typedef
>>> names, flouting the coding style.  This patch does.  Trades messing with
>>> qemu/typedefs.h for having to write 'struct' a few times.
>
> I think Markus made the right call here.  Using "struct Foo;" in headers
> is a null price to pay if all you need is declaring a pointer-typed
> field or parameter.

Eduardo posted a patch to HACKING to clarify this non-usage of typedef
is okay.

Should we continue to mandate typedef names elsewhere?  It adds
cognitive load: you have to decide where to put the typedef, and when
not to use it.

>                      Of course this doesn't apply if you have to embed a
> struct directly, but then qemu/typedefs.h wouldn't help either.

Yes, and if this leads to an inclusion cycle, I strongly suspect "fat"
headers: since you can't embed something in itself, the cycle must
involve different things, all bunched together in the same header.

> In general unless you're adding a new subsystem, qemu/typedefs.h should
> only decrease in size, never increase.

This series grows it some.  I'll try to avoid that for v2.

>                                         (And there are certainly many
> cases where typedefs.h are not needed, but cleaning that up is
> understandably not high on the todo list).

On the other hand, low-hanging fruit.
Paolo Bonzini July 31, 2019, 6:43 a.m. UTC | #7
On 31/07/19 08:37, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 30/07/19 15:15, Eric Blake wrote:
>>>> We occasionally give up and use types directly rather than their typedef
>>>> names, flouting the coding style.  This patch does.  Trades messing with
>>>> qemu/typedefs.h for having to write 'struct' a few times.
>>
>> I think Markus made the right call here.  Using "struct Foo;" in headers
>> is a null price to pay if all you need is declaring a pointer-typed
>> field or parameter.
> 
> Eduardo posted a patch to HACKING to clarify this non-usage of typedef
> is okay.
> 
> Should we continue to mandate typedef names elsewhere?  It adds
> cognitive load: you have to decide where to put the typedef, and when
> not to use it.

A lot of libraries we use (especially GLib) use typedefs, so I'd rather
keep them.  Also, a mass replacement would be tens of thousands of
changed lines and not really practical.

>>                      Of course this doesn't apply if you have to embed a
>> struct directly, but then qemu/typedefs.h wouldn't help either.
> 
> Yes, and if this leads to an inclusion cycle, I strongly suspect "fat"
> headers: since you can't embed something in itself, the cycle must
> involve different things, all bunched together in the same header.
> 
>> In general unless you're adding a new subsystem, qemu/typedefs.h should
>> only decrease in size, never increase.
> 
> This series grows it some.  I'll try to avoid that for v2.

Let me review it first. :)  Maybe some of them are good to keep.

Paolo
Thomas Huth July 31, 2019, 8:40 a.m. UTC | #8
On 31/07/2019 08.37, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 30/07/19 15:15, Eric Blake wrote:
>>>> We occasionally give up and use types directly rather than their typedef
>>>> names, flouting the coding style.  This patch does.  Trades messing with
>>>> qemu/typedefs.h for having to write 'struct' a few times.
>>
>> I think Markus made the right call here.  Using "struct Foo;" in headers
>> is a null price to pay if all you need is declaring a pointer-typed
>> field or parameter.
> 
> Eduardo posted a patch to HACKING to clarify this non-usage of typedef
> is okay.
> 
> Should we continue to mandate typedef names elsewhere?  It adds
> cognitive load: you have to decide where to put the typedef, and when
> not to use it.

IMHO we should get rid of mandating typedefs. They are causing too much
trouble - e.g. do you also remember the issues with duplicated typedefs
in certain compiler versions in the past? (these should be hopefully
gone now, but still...)

And many QEMU developers are also working on the Linux kernel, which
rather forbids typedefs. Having to switch your mind back and forth
whether to use typedefs or not is really annoying.

So if you ask me, stop mandating it! It's ok as optional feature in QEMU
for types that are used all over the place, but we really should not
enforce it for each and every struct anymore.

 Thomas
Peter Maydell July 31, 2019, 10:45 a.m. UTC | #9
On Wed, 31 Jul 2019 at 09:40, Thomas Huth <thuth@redhat.com> wrote:
> IMHO we should get rid of mandating typedefs. They are causing too much
> trouble - e.g. do you also remember the issues with duplicated typedefs
> in certain compiler versions in the past? (these should be hopefully
> gone now, but still...)
>
> And many QEMU developers are also working on the Linux kernel, which
> rather forbids typedefs. Having to switch your mind back and forth
> whether to use typedefs or not is really annoying.

I would rather keep typedefs -- it's one of the style issues we're
reasonably consistent with. QEMU isn't the kernel, and its style
is not the same on many points. If we switch to "use 'struct Foo'"
we'll have a codebase which becomes rapidly very inconsistent
about whether we use 'struct' or not.

thanks
-- PMM
Daniel P. Berrangé July 31, 2019, 10:51 a.m. UTC | #10
On Wed, Jul 31, 2019 at 11:45:41AM +0100, Peter Maydell wrote:
> On Wed, 31 Jul 2019 at 09:40, Thomas Huth <thuth@redhat.com> wrote:
> > IMHO we should get rid of mandating typedefs. They are causing too much
> > trouble - e.g. do you also remember the issues with duplicated typedefs
> > in certain compiler versions in the past? (these should be hopefully
> > gone now, but still...)
> >
> > And many QEMU developers are also working on the Linux kernel, which
> > rather forbids typedefs. Having to switch your mind back and forth
> > whether to use typedefs or not is really annoying.
> 
> I would rather keep typedefs -- it's one of the style issues we're
> reasonably consistent with. QEMU isn't the kernel, and its style
> is not the same on many points. If we switch to "use 'struct Foo'"
> we'll have a codebase which becomes rapidly very inconsistent
> about whether we use 'struct' or not.

I tend to agree - while people may work on kernel code, plenty do not
work on kernel code & QEMU is not following kernel code pratices more
generally. I think it is more compelling to align with glib given that
it is a core part of QEMU codebase. I'd much rather QEMU more closely
align with glib and increasingly drop stuff that QEMU has reinvented
in favour of using GLib features. For example I could see GObject as
a  base for QOM in future, and typedefs are a normal practice in this
case.

Regards,
Daniel
diff mbox series

Patch

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"
+#include "exec/cpu-common.h"
+
+struct CPUArchId;
 
 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