Mailing List Archive: [RFC PATCH-for-9.1 09/29] hw/i386/pc: Pass PCMachineState argument to acpi

[RFC PATCH-for-9.1 09/29] hw/i386/pc: Pass PCMachineState argument to acpi_setup()

Mar 28, 2024, 8:54 AM

Post #1 of 4 (27 views)

acpi_setup() caller knows about the machine state, so pass
it as argument to avoid a qdev_get_machine() call.

We already resolved X86_MACHINE(pcms) as 'x86ms' so use the
latter.

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

diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
index 0dce155c8c..31de5bddbd 100644
--- a/hw/i386/acpi-build.h
+++ b/hw/i386/acpi-build.h
@@ -2,6 +2,7 @@
#ifndef HW_I386_ACPI_BUILD_H
#define HW_I386_ACPI_BUILD_H
#include "hw/acpi/acpi-defs.h"
+#include "hw/i386/pc.h"

extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;

@@ -9,7 +10,7 @@ extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
#define ACPI_PCIHP_SEJ_BASE 0x8
#define ACPI_PCIHP_BNMR_BASE 0x10

-void acpi_setup(void);
+void acpi_setup(PCMachineState *pcms);
Object *acpi_get_i386_pci_host(void);

#endif
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 6e8e32e5d2..e702d5e9d2 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2749,9 +2749,8 @@ static const VMStateDescription vmstate_acpi_build = {
},
};

-void acpi_setup(void)
+void acpi_setup(PCMachineState *pcms)
{
- PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
X86MachineState *x86ms = X86_MACHINE(pcms);
AcpiBuildTables tables;
AcpiBuildState *build_state;
@@ -2771,7 +2770,7 @@ void acpi_setup(void)
return;
}

- if (!x86_machine_is_acpi_enabled(X86_MACHINE(pcms))) {
+ if (!x86_machine_is_acpi_enabled(x86ms)) {
ACPI_BUILD_DPRINTF("ACPI disabled. Bailing out.\n");
return;
}
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 6d87d1d4c2..dfc0247bb6 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -601,7 +601,7 @@ void pc_machine_done(Notifier *notifier, void *data)
/* set the number of CPUs */
x86_rtc_set_cpus_count(x86ms->rtc, x86ms->boot_cpus);

- acpi_setup();
+ acpi_setup(pcms);
if (x86ms->fw_cfg) {
fw_cfg_build_smbios(pcms, x86ms->fw_cfg, pcms->smbios_entry_point_type);
fw_cfg_build_feature_control(MACHINE(pcms), x86ms->fw_cfg);
--
2.41.0

Re: [RFC PATCH-for-9.1 09/29] hw/i386/pc: Pass PCMachineState argument to acpi_setup() [ In reply to ]

balaton at eik

Mar 28, 2024, 11:45 AM

Post #2 of 4 (27 views)

Permalink

On Thu, 28 Mar 2024, Philippe Mathieu-Daudé wrote:
> acpi_setup() caller knows about the machine state, so pass
> it as argument to avoid a qdev_get_machine() call.
>
> We already resolved X86_MACHINE(pcms) as 'x86ms' so use the
> latter.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> hw/i386/acpi-build.h | 3 ++-
> hw/i386/acpi-build.c | 5 ++---
> hw/i386/pc.c | 2 +-
> 3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> index 0dce155c8c..31de5bddbd 100644
> --- a/hw/i386/acpi-build.h
> +++ b/hw/i386/acpi-build.h
> @@ -2,6 +2,7 @@
> #ifndef HW_I386_ACPI_BUILD_H
> #define HW_I386_ACPI_BUILD_H
> #include "hw/acpi/acpi-defs.h"
> +#include "hw/i386/pc.h"
>
> extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
>
> @@ -9,7 +10,7 @@ extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
> #define ACPI_PCIHP_SEJ_BASE 0x8
> #define ACPI_PCIHP_BNMR_BASE 0x10
>
> -void acpi_setup(void);
> +void acpi_setup(PCMachineState *pcms);

This is changed to PcPciMachineState * in a following patch so can't you
already introduce it here to avoid some churn?

Regards,
BALATON Zoltan

> Object *acpi_get_i386_pci_host(void);
>
> #endif
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 6e8e32e5d2..e702d5e9d2 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2749,9 +2749,8 @@ static const VMStateDescription vmstate_acpi_build = {
> },
> };
>
> -void acpi_setup(void)
> +void acpi_setup(PCMachineState *pcms)
> {
> - PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> X86MachineState *x86ms = X86_MACHINE(pcms);
> AcpiBuildTables tables;
> AcpiBuildState *build_state;
> @@ -2771,7 +2770,7 @@ void acpi_setup(void)
> return;
> }
>
> - if (!x86_machine_is_acpi_enabled(X86_MACHINE(pcms))) {
> + if (!x86_machine_is_acpi_enabled(x86ms)) {
> ACPI_BUILD_DPRINTF("ACPI disabled. Bailing out.\n");
> return;
> }
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 6d87d1d4c2..dfc0247bb6 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -601,7 +601,7 @@ void pc_machine_done(Notifier *notifier, void *data)
> /* set the number of CPUs */
> x86_rtc_set_cpus_count(x86ms->rtc, x86ms->boot_cpus);
>
> - acpi_setup();
> + acpi_setup(pcms);
> if (x86ms->fw_cfg) {
> fw_cfg_build_smbios(pcms, x86ms->fw_cfg, pcms->smbios_entry_point_type);
> fw_cfg_build_feature_control(MACHINE(pcms), x86ms->fw_cfg);
>

Re: [RFC PATCH-for-9.1 09/29] hw/i386/pc: Pass PCMachineState argument to acpi_setup() [ In reply to ]

philmd at linaro

Mar 29, 2024, 3:23 AM

Post #3 of 4 (27 views)

Permalink

On 28/3/24 19:45, BALATON Zoltan wrote:
> On Thu, 28 Mar 2024, Philippe Mathieu-Daudé wrote:
>> acpi_setup() caller knows about the machine state, so pass
>> it as argument to avoid a qdev_get_machine() call.
>>
>> We already resolved X86_MACHINE(pcms) as 'x86ms' so use the
>> latter.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> hw/i386/acpi-build.h | 3 ++-
>> hw/i386/acpi-build.c | 5 ++---
>> hw/i386/pc.c | 2 +-
>> 3 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
>> index 0dce155c8c..31de5bddbd 100644
>> --- a/hw/i386/acpi-build.h
>> +++ b/hw/i386/acpi-build.h
>> @@ -2,6 +2,7 @@
>> #ifndef HW_I386_ACPI_BUILD_H
>> #define HW_I386_ACPI_BUILD_H
>> #include "hw/acpi/acpi-defs.h"
>> +#include "hw/i386/pc.h"
>>
>> extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
>>
>> @@ -9,7 +10,7 @@ extern const struct AcpiGenericAddress
>> x86_nvdimm_acpi_dsmio;
>> #define ACPI_PCIHP_SEJ_BASE 0x8
>> #define ACPI_PCIHP_BNMR_BASE 0x10
>>
>> -void acpi_setup(void);
>> +void acpi_setup(PCMachineState *pcms);
>
> This is changed to PcPciMachineState * in a following patch so can't you
> already introduce it here to avoid some churn?

Unfortunately not, because we'd need to use:

PcPciMachineState *ppms = PC_PCI_MACHINE(pcms);

which would trigger an assertion at this point.

>
> Regards,
> BALATON Zoltan

Re: [RFC PATCH-for-9.1 09/29] hw/i386/pc: Pass PCMachineState argument to acpi setup() [ In reply to ]

shentey at gmail

Apr 6, 2024, 3:38 AM

Post #4 of 4 (12 views)

Permalink

Am 28. März 2024 15:54:17 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>acpi_setup() caller knows about the machine state, so pass
>it as argument to avoid a qdev_get_machine() call.
>
>We already resolved X86_MACHINE(pcms) as 'x86ms' so use the
>latter.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

This patch looks like good material on its own.

Reviewed-by: Bernhard Beschow <shentey@gmail.com>

>---
> hw/i386/acpi-build.h | 3 ++-
> hw/i386/acpi-build.c | 5 ++---
> hw/i386/pc.c | 2 +-
> 3 files changed, 5 insertions(+), 5 deletions(-)
>
>diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
>index 0dce155c8c..31de5bddbd 100644
>--- a/hw/i386/acpi-build.h
>+++ b/hw/i386/acpi-build.h
>@@ -2,6 +2,7 @@
> #ifndef HW_I386_ACPI_BUILD_H
> #define HW_I386_ACPI_BUILD_H
> #include "hw/acpi/acpi-defs.h"
>+#include "hw/i386/pc.h"
>
> extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
>
>@@ -9,7 +10,7 @@ extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
> #define ACPI_PCIHP_SEJ_BASE 0x8
> #define ACPI_PCIHP_BNMR_BASE 0x10
>
>-void acpi_setup(void);
>+void acpi_setup(PCMachineState *pcms);
> Object *acpi_get_i386_pci_host(void);
>
> #endif
>diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>index 6e8e32e5d2..e702d5e9d2 100644
>--- a/hw/i386/acpi-build.c
>+++ b/hw/i386/acpi-build.c
>@@ -2749,9 +2749,8 @@ static const VMStateDescription vmstate_acpi_build = {
> },
> };
>
>-void acpi_setup(void)
>+void acpi_setup(PCMachineState *pcms)
> {
>- PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> X86MachineState *x86ms = X86_MACHINE(pcms);
> AcpiBuildTables tables;
> AcpiBuildState *build_state;
>@@ -2771,7 +2770,7 @@ void acpi_setup(void)
> return;
> }
>
>- if (!x86_machine_is_acpi_enabled(X86_MACHINE(pcms))) {
>+ if (!x86_machine_is_acpi_enabled(x86ms)) {
> ACPI_BUILD_DPRINTF("ACPI disabled. Bailing out.\n");
> return;
> }
>diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>index 6d87d1d4c2..dfc0247bb6 100644
>--- a/hw/i386/pc.c
>+++ b/hw/i386/pc.c
>@@ -601,7 +601,7 @@ void pc_machine_done(Notifier *notifier, void *data)
> /* set the number of CPUs */
> x86_rtc_set_cpus_count(x86ms->rtc, x86ms->boot_cpus);
>
>- acpi_setup();
>+ acpi_setup(pcms);
> if (x86ms->fw_cfg) {
> fw_cfg_build_smbios(pcms, x86ms->fw_cfg, pcms->smbios_entry_point_type);
> fw_cfg_build_feature_control(MACHINE(pcms), x86ms->fw_cfg);