Message ID | 1366705795-24732-20-git-send-email-imammedo@redhat.com |
---|---|
State | New |
Headers | show |
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)) { >
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 >
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).
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)) {
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(-)