Patchwork [19/21] add hot_add_cpu hook to QEMUMachine and export machine_args

login
register
mail settings
Submitter Igor Mammedov
Date April 23, 2013, 8:29 a.m.
Message ID <1366705795-24732-20-git-send-email-imammedo@redhat.com>
Download mbox | patch
Permalink /patch/238797/
State New
Headers show

Comments

Igor Mammedov - April 23, 2013, 8:29 a.m.
hot_add_cpu hook should be overriden by target that implements
cpu hot-add via cpu-add QMP command.

Make machine_args available to machine init code, it allows
to centralize cpu_model starage instead of adding target
specific globals to keep it.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/boards.h |    3 +++
 vl.c                |    6 +++++-
 2 files changed, 8 insertions(+), 1 deletions(-)
Andreas Färber - April 24, 2013, 5:25 p.m.
Am 23.04.2013 10:29, schrieb Igor Mammedov:
> hot_add_cpu hook should be overriden by target that implements
> cpu hot-add via cpu-add QMP command.
> 
> Make machine_args available to machine init code, it allows
> to centralize cpu_model starage instead of adding target
> specific globals to keep it.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  include/hw/boards.h |    3 +++
>  vl.c                |    6 +++++-
>  2 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 425bdc7..de8f92a 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -18,6 +18,8 @@ typedef struct QEMUMachineInitArgs {
>      const char *cpu_model;
>  } QEMUMachineInitArgs;
>  
> +extern QEMUMachineInitArgs *machine_args;
> +
>  typedef void QEMUMachineInitFunc(QEMUMachineInitArgs *args);
>  
>  typedef void QEMUMachineResetFunc(void);
> @@ -43,6 +45,7 @@ typedef struct QEMUMachine {
>      GlobalProperty *compat_props;
>      struct QEMUMachine *next;
>      const char *hw_version;
> +    void (*hot_add_cpu)(const int64_t id, Error **errp);
>  } QEMUMachine;
>  
>  int qemu_register_machine(QEMUMachine *m);
> diff --git a/vl.c b/vl.c
> index 5612c33..6a612e6 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -179,6 +179,8 @@ int main(int argc, char **argv)
>  #define MAX_VIRTIO_CONSOLES 1
>  #define MAX_SCLP_CONSOLES 1
>  
> +QEMUMachineInitArgs *machine_args;
> +
>  static const char *data_dir[16];
>  static int data_dir_idx;
>  const char *bios_name = NULL;
> @@ -4295,13 +4297,15 @@ int main(int argc, char **argv, char **envp)
>                                   .kernel_cmdline = kernel_cmdline,
>                                   .initrd_filename = initrd_filename,
>                                   .cpu_model = cpu_model };
> +    machine_args = &args;

args is a stack variable, so making it global does not strike me as a
good idea... (I did have a patch to split main() in two at one time)

> +    current_machine = machine;
> +
>      machine->init(&args);
>  
>      cpu_synchronize_all_post_init();
>  
>      set_numa_modes();
>  
> -    current_machine = machine;

Did you look up why it was placed here? Eduardo?
I'm not strongly opinionated but I wonder whether we may want to leave
it after machine->init?

Andreas

>  
>      /* init USB devices */
>      if (usb_enabled(false)) {
>
Igor Mammedov - April 24, 2013, 5:42 p.m.
On Wed, 24 Apr 2013 19:25:17 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 23.04.2013 10:29, schrieb Igor Mammedov:
> > hot_add_cpu hook should be overriden by target that implements
> > cpu hot-add via cpu-add QMP command.
> > 
> > Make machine_args available to machine init code, it allows
> > to centralize cpu_model starage instead of adding target
> > specific globals to keep it.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  include/hw/boards.h |    3 +++
> >  vl.c                |    6 +++++-
> >  2 files changed, 8 insertions(+), 1 deletions(-)
> > 
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 425bdc7..de8f92a 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -18,6 +18,8 @@ typedef struct QEMUMachineInitArgs {
> >      const char *cpu_model;
> >  } QEMUMachineInitArgs;
> >  
> > +extern QEMUMachineInitArgs *machine_args;
> > +
> >  typedef void QEMUMachineInitFunc(QEMUMachineInitArgs *args);
> >  
> >  typedef void QEMUMachineResetFunc(void);
> > @@ -43,6 +45,7 @@ typedef struct QEMUMachine {
> >      GlobalProperty *compat_props;
> >      struct QEMUMachine *next;
> >      const char *hw_version;
> > +    void (*hot_add_cpu)(const int64_t id, Error **errp);
> >  } QEMUMachine;
> >  
> >  int qemu_register_machine(QEMUMachine *m);
> > diff --git a/vl.c b/vl.c
> > index 5612c33..6a612e6 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -179,6 +179,8 @@ int main(int argc, char **argv)
> >  #define MAX_VIRTIO_CONSOLES 1
> >  #define MAX_SCLP_CONSOLES 1
> >  
> > +QEMUMachineInitArgs *machine_args;
> > +
> >  static const char *data_dir[16];
> >  static int data_dir_idx;
> >  const char *bios_name = NULL;
> > @@ -4295,13 +4297,15 @@ int main(int argc, char **argv, char **envp)
> >                                   .kernel_cmdline = kernel_cmdline,
> >                                   .initrd_filename = initrd_filename,
> >                                   .cpu_model = cpu_model };
> > +    machine_args = &args;
> 
> args is a stack variable, so making it global does not strike me as a
> good idea... (I did have a patch to split main() in two at one time)

True, but it lives till main() exits, and by that time nothing accesses it.
I can make it global though it will produce more code churn.

> 
> > +    current_machine = machine;
> > +
> >      machine->init(&args);
> >  
> >      cpu_synchronize_all_post_init();
> >  
> >      set_numa_modes();
> >  
> > -    current_machine = machine;
> 
> Did you look up why it was placed here? Eduardo?
> I'm not strongly opinionated but I wonder whether we may want to leave
> it after machine->init?
it was added by 6f338c34 for the sake of device hotplug. Moving it earlier
allows to use it during machine->init() as well, it's improves variable
utilization. next patch sets hot_add_cpu hook from machine->init() for x86
target.

> 
> Andreas
> 
> >  
> >      /* init USB devices */
> >      if (usb_enabled(false)) {
> > 
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>
Eduardo Habkost - April 25, 2013, 4:58 p.m.
On Wed, Apr 24, 2013 at 07:25:17PM +0200, Andreas Färber wrote:
> Am 23.04.2013 10:29, schrieb Igor Mammedov:
> > hot_add_cpu hook should be overriden by target that implements
> > cpu hot-add via cpu-add QMP command.
> > 
> > Make machine_args available to machine init code, it allows
> > to centralize cpu_model starage instead of adding target
> > specific globals to keep it.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  include/hw/boards.h |    3 +++
> >  vl.c                |    6 +++++-
> >  2 files changed, 8 insertions(+), 1 deletions(-)
> > 
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 425bdc7..de8f92a 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -18,6 +18,8 @@ typedef struct QEMUMachineInitArgs {
> >      const char *cpu_model;
> >  } QEMUMachineInitArgs;
> >  
> > +extern QEMUMachineInitArgs *machine_args;
> > +
> >  typedef void QEMUMachineInitFunc(QEMUMachineInitArgs *args);
> >  
> >  typedef void QEMUMachineResetFunc(void);
> > @@ -43,6 +45,7 @@ typedef struct QEMUMachine {
> >      GlobalProperty *compat_props;
> >      struct QEMUMachine *next;
> >      const char *hw_version;
> > +    void (*hot_add_cpu)(const int64_t id, Error **errp);
> >  } QEMUMachine;
> >  
> >  int qemu_register_machine(QEMUMachine *m);
> > diff --git a/vl.c b/vl.c
> > index 5612c33..6a612e6 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -179,6 +179,8 @@ int main(int argc, char **argv)
> >  #define MAX_VIRTIO_CONSOLES 1
> >  #define MAX_SCLP_CONSOLES 1
> >  
> > +QEMUMachineInitArgs *machine_args;
> > +
> >  static const char *data_dir[16];
> >  static int data_dir_idx;
> >  const char *bios_name = NULL;
> > @@ -4295,13 +4297,15 @@ int main(int argc, char **argv, char **envp)
> >                                   .kernel_cmdline = kernel_cmdline,
> >                                   .initrd_filename = initrd_filename,
> >                                   .cpu_model = cpu_model };
> > +    machine_args = &args;
> 
> args is a stack variable, so making it global does not strike me as a
> good idea... (I did have a patch to split main() in two at one time)
> 
> > +    current_machine = machine;
> > +
> >      machine->init(&args);
> >  
> >      cpu_synchronize_all_post_init();
> >  
> >      set_numa_modes();
> >  
> > -    current_machine = machine;
> 
> Did you look up why it was placed here? Eduardo?
> I'm not strongly opinionated but I wonder whether we may want to leave
> it after machine->init?

It was always there since the first time I looked at that code. But I
believe it makes sense to set it earlier so initialization code can use
the machine object.

On the other hand, I would really like to reduce usage of global
variables during initialization, and instead pass a QEMUMachineInitArgs
argument to init functions that need it. But I understand that sometimes
we need to be pragmatic and a global variable is a simpler approach (if
considered just a temporary solution).

Patch

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 425bdc7..de8f92a 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -18,6 +18,8 @@  typedef struct QEMUMachineInitArgs {
     const char *cpu_model;
 } QEMUMachineInitArgs;
 
+extern QEMUMachineInitArgs *machine_args;
+
 typedef void QEMUMachineInitFunc(QEMUMachineInitArgs *args);
 
 typedef void QEMUMachineResetFunc(void);
@@ -43,6 +45,7 @@  typedef struct QEMUMachine {
     GlobalProperty *compat_props;
     struct QEMUMachine *next;
     const char *hw_version;
+    void (*hot_add_cpu)(const int64_t id, Error **errp);
 } QEMUMachine;
 
 int qemu_register_machine(QEMUMachine *m);
diff --git a/vl.c b/vl.c
index 5612c33..6a612e6 100644
--- a/vl.c
+++ b/vl.c
@@ -179,6 +179,8 @@  int main(int argc, char **argv)
 #define MAX_VIRTIO_CONSOLES 1
 #define MAX_SCLP_CONSOLES 1
 
+QEMUMachineInitArgs *machine_args;
+
 static const char *data_dir[16];
 static int data_dir_idx;
 const char *bios_name = NULL;
@@ -4295,13 +4297,15 @@  int main(int argc, char **argv, char **envp)
                                  .kernel_cmdline = kernel_cmdline,
                                  .initrd_filename = initrd_filename,
                                  .cpu_model = cpu_model };
+    machine_args = &args;
+    current_machine = machine;
+
     machine->init(&args);
 
     cpu_synchronize_all_post_init();
 
     set_numa_modes();
 
-    current_machine = machine;
 
     /* init USB devices */
     if (usb_enabled(false)) {