[PATCH 0/5] hw/acpi: Include missing headers

[PATCH 0/5] hw/acpi: Include missing headers

[Philippe Mathieu-Daudé]

Patches extracted from an ICH9 model rework. Explicitly include
required headers instead of having them being randomply pulled.

Philippe Mathieu-Daudé (5):
  hw/acpi: Include missing 'qapi/qapi-types-acpi.h' generated header
  hw/acpi/cpu: Use CPUState typedef
  hw/acpi/ich9: Include missing headers
  hw/acpi/ich9_tco: Include missing 'migration/vmstate.h' header
  hw/i386/q35: Include missing 'hw/acpi/acpi.h' header

 include/hw/acpi/cpu.h            | 3 ++-
 include/hw/acpi/ich9.h           | 3 ++-
 include/hw/acpi/ich9_tco.h       | 1 +
 include/hw/acpi/memory_hotplug.h | 1 +
 hw/acpi/ich9.c                   | 2 +-
 hw/i386/pc_q35.c                 | 1 +
 6 files changed, 8 insertions(+), 3 deletions(-)

-- 
2.41.0

[PATCH 1/5] hw/acpi: Include missing 'qapi/qapi-types-acpi.h' generated header

[Philippe Mathieu-Daudé]

ACPIOSTInfo is a QAPI generated structure:

  $ git grep -w ACPIOSTInfo
  qapi/acpi.json:81:# @ACPIOSTInfo:
  qapi/acpi.json:99:{ 'struct': 'ACPIOSTInfo',
  qapi/acpi.json:109:# Return a list of ACPIOSTInfo for devices that support status

Include the "qapi/qapi-types-acpi.h" header to avoid the following
errors when including "hw/acpi/cpu.h" or "hw/acpi/memory_hotplug.h"
elsewhere:

  include/hw/acpi/cpu.h:67:52: error: unknown type name 'ACPIOSTInfoList'
  void acpi_cpu_ospm_status(CPUHotplugState *cpu_st, ACPIOSTInfoList ***list);
                                                     ^
  include/hw/acpi/memory_hotplug.h:51:55: error: unknown type name 'ACPIOSTInfoList'
  void acpi_memory_ospm_status(MemHotplugState *mem_st, ACPIOSTInfoList ***list);
                                                        ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/acpi/cpu.h            | 1 +
 include/hw/acpi/memory_hotplug.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
index bc901660fb..e011d01adf 100644
--- a/include/hw/acpi/cpu.h
+++ b/include/hw/acpi/cpu.h
@@ -12,6 +12,7 @@
 #ifndef ACPI_CPU_H
 #define ACPI_CPU_H
 
+#include "qapi/qapi-types-acpi.h"
 #include "hw/qdev-core.h"
 #include "hw/acpi/acpi.h"
 #include "hw/acpi/aml-build.h"
diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
index dfe9cf3fde..38841d7b06 100644
--- a/include/hw/acpi/memory_hotplug.h
+++ b/include/hw/acpi/memory_hotplug.h
@@ -1,6 +1,7 @@
 #ifndef QEMU_HW_ACPI_MEMORY_HOTPLUG_H
 #define QEMU_HW_ACPI_MEMORY_HOTPLUG_H
 
+#include "qapi/qapi-types-acpi.h"
 #include "hw/qdev-core.h"
 #include "hw/acpi/acpi.h"
 #include "hw/acpi/aml-build.h"
-- 
2.41.0

[PATCH 2/5] hw/acpi/cpu: Use CPUState typedef

[Philippe Mathieu-Daudé]

QEMU coding style recommend using structure typedefs:
https://www.qemu.org/docs/master/devel/style.html#typedefs

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
---
 include/hw/acpi/cpu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
index e011d01adf..e6e1a9ef59 100644
--- a/include/hw/acpi/cpu.h
+++ b/include/hw/acpi/cpu.h
@@ -20,7 +20,7 @@
 #include "hw/hotplug.h"
 
 typedef struct AcpiCpuStatus {
-    struct CPUState *cpu;
+    CPUState *cpu;
     uint64_t arch_id;
     bool is_inserting;
     bool is_removing;
-- 
2.41.0

[PATCH 3/5] hw/acpi/ich9: Include missing headers

[Philippe Mathieu-Daudé]

The ICH9LPCPMRegs structure has MemoryRegion and
Notifier fields, so requires the "qemu/notify.h"
and "exec/memory.h" headers.

However nothing from "hw/acpi/acpi_dev_interface.h"
is required, so reduce its inclusion to hw/acpi/ich9.c
source file where it is used.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/acpi/ich9.h | 3 ++-
 hw/acpi/ich9.c         | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
index 2faf7f0cae..215de3c91f 100644
--- a/include/hw/acpi/ich9.h
+++ b/include/hw/acpi/ich9.h
@@ -21,12 +21,13 @@
 #ifndef HW_ACPI_ICH9_H
 #define HW_ACPI_ICH9_H
 
+#include "qemu/notify.h"
+#include "exec/memory.h"
 #include "hw/acpi/acpi.h"
 #include "hw/acpi/cpu_hotplug.h"
 #include "hw/acpi/cpu.h"
 #include "hw/acpi/pcihp.h"
 #include "hw/acpi/memory_hotplug.h"
-#include "hw/acpi/acpi_dev_interface.h"
 #include "hw/acpi/ich9_tco.h"
 
 #define ACPI_PCIHP_ADDR_ICH9 0x0cc0
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 573d032e8e..be375a8b9d 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -35,7 +35,7 @@
 #include "sysemu/runstate.h"
 #include "hw/acpi/acpi.h"
 #include "hw/acpi/ich9_tco.h"
-
+#include "hw/acpi/acpi_dev_interface.h"
 #include "hw/southbridge/ich9.h"
 #include "hw/mem/pc-dimm.h"
 #include "hw/mem/nvdimm.h"
-- 
2.41.0

[PATCH 4/5] hw/acpi/ich9_tco: Include missing 'migration/vmstate.h' header

[Philippe Mathieu-Daudé]

We need the VMStateDescription structure definition from
"migration/vmstate.h" in order to declare vmstate_tco_io_sts.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/acpi/ich9_tco.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/hw/acpi/ich9_tco.h b/include/hw/acpi/ich9_tco.h
index c4393caee0..2562a7cf39 100644
--- a/include/hw/acpi/ich9_tco.h
+++ b/include/hw/acpi/ich9_tco.h
@@ -11,6 +11,7 @@
 #define HW_ACPI_TCO_H
 
 #include "exec/memory.h"
+#include "migration/vmstate.h"
 
 /* As per ICH9 spec, the internal timer has an error of ~0.6s on every tick */
 #define TCO_TICK_NSEC 600000000LL
-- 
2.41.0

[PATCH 5/5] hw/i386/q35: Include missing 'hw/acpi/acpi.h' header

[Philippe Mathieu-Daudé]

"hw/acpi/acpi.h" is implicitly included. Included it
explicitly to avoid when refactoring headers:

hw/i386/pc_q35.c:209:43: error: use of undeclared identifier 'ACPI_PM_PROP_ACPI_PCIHP_BRIDGE'
                              ACPI_PM_PROP_ACPI_PCIHP_BRIDGE,
                              ^

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

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index d346fa3b1d..a91f414922 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -58,6 +58,7 @@
 #include "hw/hyperv/vmbus-bridge.h"
 #include "hw/mem/nvdimm.h"
 #include "hw/i386/acpi-build.h"
+#include "hw/acpi/acpi.h"
 #include "target/i386/cpu.h"
 
 /* ICH9 AHCI has 6 ports */
-- 
2.41.0

Re: [PATCH 1/5] hw/acpi: Include missing 'qapi/qapi-types-acpi.h' generated header

[Zhao Liu]

On Mon, Feb 19, 2024 at 03:14:07PM +0100, Philippe Mathieu-Daudé wrote:
> Date: Mon, 19 Feb 2024 15:14:07 +0100
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> Subject: [PATCH 1/5] hw/acpi: Include missing 'qapi/qapi-types-acpi.h'
>  generated header
> X-Mailer: git-send-email 2.41.0
> 
> ACPIOSTInfo is a QAPI generated structure:
> 
>   $ git grep -w ACPIOSTInfo
>   qapi/acpi.json:81:# @ACPIOSTInfo:
>   qapi/acpi.json:99:{ 'struct': 'ACPIOSTInfo',
>   qapi/acpi.json:109:# Return a list of ACPIOSTInfo for devices that support status
> 
> Include the "qapi/qapi-types-acpi.h" header to avoid the following
> errors when including "hw/acpi/cpu.h" or "hw/acpi/memory_hotplug.h"
> elsewhere:
> 
>   include/hw/acpi/cpu.h:67:52: error: unknown type name 'ACPIOSTInfoList'
>   void acpi_cpu_ospm_status(CPUHotplugState *cpu_st, ACPIOSTInfoList ***list);
>                                                      ^
>   include/hw/acpi/memory_hotplug.h:51:55: error: unknown type name 'ACPIOSTInfoList'
>   void acpi_memory_ospm_status(MemHotplugState *mem_st, ACPIOSTInfoList ***list);
>                                                         ^
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/hw/acpi/cpu.h            | 1 +
>  include/hw/acpi/memory_hotplug.h | 1 +
>  2 files changed, 2 insertions(+)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

> 
> diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
> index bc901660fb..e011d01adf 100644
> --- a/include/hw/acpi/cpu.h
> +++ b/include/hw/acpi/cpu.h
> @@ -12,6 +12,7 @@
>  #ifndef ACPI_CPU_H
>  #define ACPI_CPU_H
>  
> +#include "qapi/qapi-types-acpi.h"
>  #include "hw/qdev-core.h"
>  #include "hw/acpi/acpi.h"
>  #include "hw/acpi/aml-build.h"
> diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
> index dfe9cf3fde..38841d7b06 100644
> --- a/include/hw/acpi/memory_hotplug.h
> +++ b/include/hw/acpi/memory_hotplug.h
> @@ -1,6 +1,7 @@
>  #ifndef QEMU_HW_ACPI_MEMORY_HOTPLUG_H
>  #define QEMU_HW_ACPI_MEMORY_HOTPLUG_H
>  
> +#include "qapi/qapi-types-acpi.h"
>  #include "hw/qdev-core.h"
>  #include "hw/acpi/acpi.h"
>  #include "hw/acpi/aml-build.h"
> -- 
> 2.41.0
> 
> 

Re: [PATCH 3/5] hw/acpi/ich9: Include missing headers

[Zhao Liu]

Hi Philippe,

On Mon, Feb 19, 2024 at 03:14:09PM +0100, Philippe Mathieu-Daudé wrote:
> Date: Mon, 19 Feb 2024 15:14:09 +0100
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> Subject: [PATCH 3/5] hw/acpi/ich9: Include missing headers
> X-Mailer: git-send-email 2.41.0
> 
> The ICH9LPCPMRegs structure has MemoryRegion and
> Notifier fields, so requires the "qemu/notify.h"
> and "exec/memory.h" headers.
> 
> However nothing from "hw/acpi/acpi_dev_interface.h"
> is required, so reduce its inclusion to hw/acpi/ich9.c
> source file where it is used.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/hw/acpi/ich9.h | 3 ++-
>  hw/acpi/ich9.c         | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> index 2faf7f0cae..215de3c91f 100644
> --- a/include/hw/acpi/ich9.h
> +++ b/include/hw/acpi/ich9.h
> @@ -21,12 +21,13 @@
>  #ifndef HW_ACPI_ICH9_H
>  #define HW_ACPI_ICH9_H
>  
> +#include "qemu/notify.h"
> +#include "exec/memory.h"
>  #include "hw/acpi/acpi.h"
>  #include "hw/acpi/cpu_hotplug.h"
>  #include "hw/acpi/cpu.h"
>  #include "hw/acpi/pcihp.h"
>  #include "hw/acpi/memory_hotplug.h"
> -#include "hw/acpi/acpi_dev_interface.h"
>  #include "hw/acpi/ich9_tco.h"
>  
>  #define ACPI_PCIHP_ADDR_ICH9 0x0cc0
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 573d032e8e..be375a8b9d 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -35,7 +35,7 @@
>  #include "sysemu/runstate.h"
>  #include "hw/acpi/acpi.h"
>  #include "hw/acpi/ich9_tco.h"
> -
> +#include "hw/acpi/acpi_dev_interface.h"

Do we have the requirement for included header ordering?
If so, it would be better to put it before "hw/acpi/ich9_tco.h".

-Zhao

>  #include "hw/southbridge/ich9.h"
>  #include "hw/mem/pc-dimm.h"
>  #include "hw/mem/nvdimm.h"
> -- 
> 2.41.0
> 
> 

Re: [PATCH 4/5] hw/acpi/ich9_tco: Include missing 'migration/vmstate.h' header

[Zhao Liu]

On Mon, Feb 19, 2024 at 03:14:10PM +0100, Philippe Mathieu-Daudé wrote:
> Date: Mon, 19 Feb 2024 15:14:10 +0100
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> Subject: [PATCH 4/5] hw/acpi/ich9_tco: Include missing
>  'migration/vmstate.h' header
> X-Mailer: git-send-email 2.41.0
> 
> We need the VMStateDescription structure definition from
> "migration/vmstate.h" in order to declare vmstate_tco_io_sts.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/hw/acpi/ich9_tco.h | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

> 
> diff --git a/include/hw/acpi/ich9_tco.h b/include/hw/acpi/ich9_tco.h
> index c4393caee0..2562a7cf39 100644
> --- a/include/hw/acpi/ich9_tco.h
> +++ b/include/hw/acpi/ich9_tco.h
> @@ -11,6 +11,7 @@
>  #define HW_ACPI_TCO_H
>  
>  #include "exec/memory.h"
> +#include "migration/vmstate.h"
>  
>  /* As per ICH9 spec, the internal timer has an error of ~0.6s on every tick */
>  #define TCO_TICK_NSEC 600000000LL
> -- 
> 2.41.0
> 
> 

Re: [PATCH 5/5] hw/i386/q35: Include missing 'hw/acpi/acpi.h' header

[Zhao Liu]

Hi Philippe,

On Mon, Feb 19, 2024 at 03:14:11PM +0100, Philippe Mathieu-Daudé wrote:
> Date: Mon, 19 Feb 2024 15:14:11 +0100
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> Subject: [PATCH 5/5] hw/i386/q35: Include missing 'hw/acpi/acpi.h' header
> X-Mailer: git-send-email 2.41.0
> 
> "hw/acpi/acpi.h" is implicitly included.
> Included it

I'm guessing it's an imperative and shouldn't be in passive form. ;-) So,

s/Included/Include/

> explicitly to avoid when refactoring headers:

s/to avoid/to avoid the following error/

> 
> hw/i386/pc_q35.c:209:43: error: use of undeclared identifier 'ACPI_PM_PROP_ACPI_PCIHP_BRIDGE'
>                               ACPI_PM_PROP_ACPI_PCIHP_BRIDGE,
>                               ^
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/i386/pc_q35.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index d346fa3b1d..a91f414922 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -58,6 +58,7 @@
>  #include "hw/hyperv/vmbus-bridge.h"
>  #include "hw/mem/nvdimm.h"
>  #include "hw/i386/acpi-build.h"
> +#include "hw/acpi/acpi.h"
>  #include "target/i386/cpu.h"

Here it is unordered, and similarly, does it need to be sorted
alphabetically here?

Regards,
Zhao

>  
>  /* ICH9 AHCI has 6 ports */
> -- 
> 2.41.0
> 
> 

Re: [PATCH 3/5] hw/acpi/ich9: Include missing headers

[Philippe Mathieu-Daudé]

On 20/2/24 03:33, Zhao Liu wrote:
> Hi Philippe,
> 
> On Mon, Feb 19, 2024 at 03:14:09PM +0100, Philippe Mathieu-Daudé wrote:
>> Date: Mon, 19 Feb 2024 15:14:09 +0100
>> From: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Subject: [PATCH 3/5] hw/acpi/ich9: Include missing headers
>> X-Mailer: git-send-email 2.41.0
>>
>> The ICH9LPCPMRegs structure has MemoryRegion and
>> Notifier fields, so requires the "qemu/notify.h"
>> and "exec/memory.h" headers.
>>
>> However nothing from "hw/acpi/acpi_dev_interface.h"
>> is required, so reduce its inclusion to hw/acpi/ich9.c
>> source file where it is used.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   include/hw/acpi/ich9.h | 3 ++-
>>   hw/acpi/ich9.c         | 2 +-
>>   2 files changed, 3 insertions(+), 2 deletions(-)


>> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
>> index 573d032e8e..be375a8b9d 100644
>> --- a/hw/acpi/ich9.c
>> +++ b/hw/acpi/ich9.c
>> @@ -35,7 +35,7 @@
>>   #include "sysemu/runstate.h"
>>   #include "hw/acpi/acpi.h"
>>   #include "hw/acpi/ich9_tco.h"
>> -
>> +#include "hw/acpi/acpi_dev_interface.h"
> 
> Do we have the requirement for included header ordering?

We don't. If headers were properly including requisites and
self-contained, order shouldn't matter, and we could sort them
alphabetically to enforce a common style.

> If so, it would be better to put it before "hw/acpi/ich9_tco.h".

Sure, no problem.

> 
> -Zhao
> 
>>   #include "hw/southbridge/ich9.h"
>>   #include "hw/mem/pc-dimm.h"
>>   #include "hw/mem/nvdimm.h"
>> -- 
>> 2.41.0
>>
>>

Re: [PATCH 5/5] hw/i386/q35: Include missing 'hw/acpi/acpi.h' header

[Philippe Mathieu-Daudé]

On 20/2/24 03:45, Zhao Liu wrote:
> Hi Philippe,
> 
> On Mon, Feb 19, 2024 at 03:14:11PM +0100, Philippe Mathieu-Daudé wrote:
>> Date: Mon, 19 Feb 2024 15:14:11 +0100
>> From: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Subject: [PATCH 5/5] hw/i386/q35: Include missing 'hw/acpi/acpi.h' header
>> X-Mailer: git-send-email 2.41.0
>>
>> "hw/acpi/acpi.h" is implicitly included.
>> Included it
> 
> I'm guessing it's an imperative and shouldn't be in passive form. ;-) So,
> 
> s/Included/Include/
> 
>> explicitly to avoid when refactoring headers:
> 
> s/to avoid/to avoid the following error/

Oops, thanks :)

> 
>>
>> hw/i386/pc_q35.c:209:43: error: use of undeclared identifier 'ACPI_PM_PROP_ACPI_PCIHP_BRIDGE'
>>                                ACPI_PM_PROP_ACPI_PCIHP_BRIDGE,
>>                                ^
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/i386/pc_q35.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index d346fa3b1d..a91f414922 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -58,6 +58,7 @@
>>   #include "hw/hyperv/vmbus-bridge.h"
>>   #include "hw/mem/nvdimm.h"
>>   #include "hw/i386/acpi-build.h"
>> +#include "hw/acpi/acpi.h"
>>   #include "target/i386/cpu.h"
> 
> Here it is unordered, and similarly, does it need to be sorted
> alphabetically here?

It shouldn't hurt ;)