diff mbox series

[10/20] hw/i386/pc: Pass the boot_cpus value by argument

Message ID 20190524063553.5339-11-philmd@redhat.com (mailing list archive)
State New, archived
Headers show
Series hw/i386/pc: Do not restrict the fw_cfg functions to the PC machine | expand

Commit Message

Philippe Mathieu-Daudé May 24, 2019, 6:35 a.m. UTC
The boot_cpus is used once. Pass it by argument, this will
allow us to remove the PCMachineState argument later.

Suggested-by: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/pc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Li Qiang May 24, 2019, 3:30 p.m. UTC | #1
Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年5月24日周五 下午2:50写道:

> The boot_cpus is used once. Pass it by argument, this will
> allow us to remove the PCMachineState argument later.
>
> Suggested-by: Samuel Ortiz <sameo@linux.intel.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/i386/pc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 264074489b..01894b9875 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -928,7 +928,7 @@ static void pc_build_smbios(PCMachineState *pcms)
>      }
>  }
>
> -static FWCfgState *x86_create_fw_cfg(PCMachineState *pcms)
> +static FWCfgState *x86_create_fw_cfg(PCMachineState *pcms, uint16_t
> boot_cpus)
>


For the patches 10/11/12, I don't think this is an elegant solution. When
we add more data like 'boot_cpus'
we need add more arguments?


Thanks,
Li Qiang




>  {
>      FWCfgState *fw_cfg;
>      uint64_t *numa_fw_cfg;
> @@ -938,7 +938,7 @@ static FWCfgState *x86_create_fw_cfg(PCMachineState
> *pcms)
>
>      fw_cfg = fw_cfg_init_io_dma(FW_CFG_IO_BASE, FW_CFG_IO_BASE + 4,
>                                  &address_space_memory);
> -    fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus);
> +    fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, boot_cpus);
>
>      /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86:
>       *
> @@ -1762,7 +1762,7 @@ void pc_memory_init(PCMachineState *pcms,
>                                          option_rom_mr,
>                                          1);
>
> -    fw_cfg = x86_create_fw_cfg(pcms);
> +    fw_cfg = x86_create_fw_cfg(pcms, pcms->boot_cpus);
>
>      rom_set_fw(fw_cfg);
>
> --
> 2.20.1
>
>
>
Philippe Mathieu-Daudé May 30, 2019, 3:19 p.m. UTC | #2
On 5/24/19 5:30 PM, Li Qiang wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com <mailto:philmd@redhat.com>> 于
> 2019年5月24日周五 下午2:50写道:
> 
>     The boot_cpus is used once. Pass it by argument, this will
>     allow us to remove the PCMachineState argument later.
> 
>     Suggested-by: Samuel Ortiz <sameo@linux.intel.com
>     <mailto:sameo@linux.intel.com>>
>     Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com
>     <mailto:philmd@redhat.com>>
>     ---
>      hw/i386/pc.c | 6 +++---
>      1 file changed, 3 insertions(+), 3 deletions(-)
> 
>     diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>     index 264074489b..01894b9875 100644
>     --- a/hw/i386/pc.c
>     +++ b/hw/i386/pc.c
>     @@ -928,7 +928,7 @@ static void pc_build_smbios(PCMachineState *pcms)
>          }
>      }
> 
>     -static FWCfgState *x86_create_fw_cfg(PCMachineState *pcms)
>     +static FWCfgState *x86_create_fw_cfg(PCMachineState *pcms, uint16_t
>     boot_cpus)
> 
> 
> For the patches 10/11/12, I don't think this is an elegant solution.
> When we add more data like 'boot_cpus'
> we need add more arguments?

This fonction is called once at machine creation, so there is no
performance penalty. To keep the code modularizable (reusable) it is an
acceptable tradeoff :)

> 
>      {
>          FWCfgState *fw_cfg;
>          uint64_t *numa_fw_cfg;
>     @@ -938,7 +938,7 @@ static FWCfgState
>     *x86_create_fw_cfg(PCMachineState *pcms)
> 
>          fw_cfg = fw_cfg_init_io_dma(FW_CFG_IO_BASE, FW_CFG_IO_BASE + 4,
>                                      &address_space_memory);
>     -    fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus);
>     +    fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, boot_cpus);
> 
>          /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86:
>           *
>     @@ -1762,7 +1762,7 @@ void pc_memory_init(PCMachineState *pcms,
>                                              option_rom_mr,
>                                              1);
> 
>     -    fw_cfg = x86_create_fw_cfg(pcms);
>     +    fw_cfg = x86_create_fw_cfg(pcms, pcms->boot_cpus);
> 
>          rom_set_fw(fw_cfg);
> 
>     -- 
>     2.20.1
> 
>
diff mbox series

Patch

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 264074489b..01894b9875 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -928,7 +928,7 @@  static void pc_build_smbios(PCMachineState *pcms)
     }
 }
 
-static FWCfgState *x86_create_fw_cfg(PCMachineState *pcms)
+static FWCfgState *x86_create_fw_cfg(PCMachineState *pcms, uint16_t boot_cpus)
 {
     FWCfgState *fw_cfg;
     uint64_t *numa_fw_cfg;
@@ -938,7 +938,7 @@  static FWCfgState *x86_create_fw_cfg(PCMachineState *pcms)
 
     fw_cfg = fw_cfg_init_io_dma(FW_CFG_IO_BASE, FW_CFG_IO_BASE + 4,
                                 &address_space_memory);
-    fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, boot_cpus);
 
     /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86:
      *
@@ -1762,7 +1762,7 @@  void pc_memory_init(PCMachineState *pcms,
                                         option_rom_mr,
                                         1);
 
-    fw_cfg = x86_create_fw_cfg(pcms);
+    fw_cfg = x86_create_fw_cfg(pcms, pcms->boot_cpus);
 
     rom_set_fw(fw_cfg);