| Message ID | 1342724013-1633-10-git-send-email-minyard@acm.org |
|---|---|
| State | New |
| Headers | show |
Am 19.07.2012 20:53, schrieb minyard@acm.org: > From: Corey Minyard <cminyard@mvista.com> > > Some devices may need to do some firmware-type initialization before > the firmware itself is initialized. For instance, any device that > adds SMBIOS table entries (like IPMI) will need to do that before > the BIOS is initialized. > > So add a list of devices that get initialized before the firmware > initialization. Since those devices get removed from the list so > they don't get initialized twice, convert the qemu_opt_foreach() to > use the safe list traversal. > > Signed-off-by: Corey Minyard <cminyard@mvista.com> I'm not so thrilled by this interface... This doesn't seem to have anything to do with the monitor, and prefw_list doesn't really indicate it's a list of type names either, nor does qdev_add_prefw_init() precisely indicate what it's doing. Then, your implementation of qdev_prefw_init_check() is based on re-iterating over -device options. That's not safe since there's other ways to create a device - object_new() et al. and soon a qom_create QMP command. So what exactly is the problem you are trying to solve here? What needs to happen before this step and until when does this need to happen? For example, is this an ordering problem between device creation and MemoryRegion setup in the machine? My thinking is that if this is a generic problem unrelated to QOM realize / DeviceState::init we could provide a new DeviceState::init_firmware hook for that purpose and call it recursively on all devices. If however this is just some PC quirk, we should not model this at qdev level but locally in hw/pc.c, then you have more degrees of freedom in designing some PC-internal API. Regards, Andreas > --- > hw/pc.c | 2 ++ > hw/qdev-monitor.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > hw/qdev.h | 3 +++ > qemu-option.c | 4 ++-- > 4 files changed, 59 insertions(+), 2 deletions(-) > > diff --git a/hw/pc.c b/hw/pc.c > index bedd5be..811d99c 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -1010,6 +1010,8 @@ void *pc_bios_init(const char *kernel_filename, > void *fw_cfg; > int linux_boot, i; > > + qdev_prefw_init_check(); > + > /* Initialize PC system firmware */ > pc_system_firmware_init(rom_memory); > > diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c > index 7915b45..bb8e955 100644 > --- a/hw/qdev-monitor.c > +++ b/hw/qdev-monitor.c > @@ -486,6 +486,58 @@ DeviceState *qdev_device_add(QemuOpts *opts) > return qdev; > } > > +/* > + * Some devices may need to be initialized before the firmware is > + * initialized, so they may add information to the firmware > + * structures. > + */ > +typedef struct prefw_list { > + const char *name; > + struct prefw_list *next; > +} prefw_list; > + > +prefw_list *prefw_init; > + > +void qdev_add_prefw_init(const char *name) > +{ > + prefw_list *e = g_malloc(sizeof(*e)); > + e->name = name; > + e->next = prefw_init; > + prefw_init = e; > +} > + > +static int prefw_check_func(QemuOpts *opts, void *opaque) > +{ > + DeviceState *dev; > + const char *driver; > + prefw_list *e; > + > + driver = qemu_opt_get(opts, "driver"); > + if (!driver) { > + return 0; > + } > + > + for (e = prefw_init; e; e = e->next) { > + if (strcmp(driver, e->name) == 0) { > + dev = qdev_device_add(opts); > + if (!dev) { > + return -1; > + } > + qemu_opts_del(opts); > + break; > + } > + } > + return 0; > +} > + > +void qdev_prefw_init_check(void) > +{ > + /* init generic devices */ > + if (qemu_opts_foreach(qemu_find_opts("device"), > + prefw_check_func, NULL, 1) != 0) { > + exit(1); > + } > +} > > #define qdev_printf(fmt, ...) monitor_printf(mon, "%*s" fmt, indent, "", ## __VA_ARGS__) > static void qbus_print(Monitor *mon, BusState *bus, int indent); > diff --git a/hw/qdev.h b/hw/qdev.h > index f4683dc..2be4c95 100644 > --- a/hw/qdev.h > +++ b/hw/qdev.h > @@ -167,6 +167,9 @@ int qdev_simple_unplug_cb(DeviceState *dev); > void qdev_machine_creation_done(void); > bool qdev_machine_modified(void); > > +void qdev_add_prefw_init(const char *name); > +void qdev_prefw_init_check(void); > + > qemu_irq qdev_get_gpio_in(DeviceState *dev, int n); > void qdev_connect_gpio_out(DeviceState *dev, int n, qemu_irq pin); > > diff --git a/qemu-option.c b/qemu-option.c > index bb3886c..1d000f8 100644 > --- a/qemu-option.c > +++ b/qemu-option.c > @@ -709,10 +709,10 @@ int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val) > int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque, > int abort_on_failure) > { > - QemuOpt *opt; > + QemuOpt *opt, *next_opt; > int rc = 0; > > - QTAILQ_FOREACH(opt, &opts->head, next) { > + QTAILQ_FOREACH_SAFE(opt, &opts->head, next, next_opt) { > rc = func(opt->name, opt->str, opaque); > if (abort_on_failure && rc != 0) > break; >
On 07/30/2012 09:36 AM, Andreas Färber wrote: > Am 19.07.2012 20:53, schrieb minyard@acm.org: >> From: Corey Minyard <cminyard@mvista.com> >> >> Some devices may need to do some firmware-type initialization before >> the firmware itself is initialized. For instance, any device that >> adds SMBIOS table entries (like IPMI) will need to do that before >> the BIOS is initialized. >> >> So add a list of devices that get initialized before the firmware >> initialization. Since those devices get removed from the list so >> they don't get initialized twice, convert the qemu_opt_foreach() to >> use the safe list traversal. >> >> Signed-off-by: Corey Minyard <cminyard@mvista.com> > I'm not so thrilled by this interface... Actually, I'm not terribly thrilled by it, either, but I need something like it. > > This doesn't seem to have anything to do with the monitor, and > prefw_list doesn't really indicate it's a list of type names either, nor > does qdev_add_prefw_init() precisely indicate what it's doing. > > Then, your implementation of qdev_prefw_init_check() is based on > re-iterating over -device options. That's not safe since there's other > ways to create a device - object_new() et al. and soon a qom_create QMP > command. > > So what exactly is the problem you are trying to solve here? What needs > to happen before this step and until when does this need to happen? For > example, is this an ordering problem between device creation and > MemoryRegion setup in the machine? > > My thinking is that if this is a generic problem unrelated to QOM > realize / DeviceState::init we could provide a new > DeviceState::init_firmware hook for that purpose and call it recursively > on all devices. If however this is just some PC quirk, we should not > model this at qdev level but locally in hw/pc.c, then you have more > degrees of freedom in designing some PC-internal API. The basic problem is that I need to do some level of initialization on the device, at least to determine the interface address, interrupt, etc., so the SMBIOS tables can be populated. I assume something similar will be required for OF if powerpc is ported, and perhaps for uboot type interfaces if those are ever ported. The basic problem, I guess, is that the SMBIOS tables are fixed very early, before devices have initialized. If that could be moved later, or device initialization moved earlier, the problem would go away. But with the current design, I couldn't see an easy way to delay setting the SMBIOS tables that far. I delayed it as long as I could see to do it reasonably and then put the firmware init right before that. Thanks, -corey > > Regards, > Andreas > >> --- >> hw/pc.c | 2 ++ >> hw/qdev-monitor.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >> hw/qdev.h | 3 +++ >> qemu-option.c | 4 ++-- >> 4 files changed, 59 insertions(+), 2 deletions(-) >> >> diff --git a/hw/pc.c b/hw/pc.c >> index bedd5be..811d99c 100644 >> --- a/hw/pc.c >> +++ b/hw/pc.c >> @@ -1010,6 +1010,8 @@ void *pc_bios_init(const char *kernel_filename, >> void *fw_cfg; >> int linux_boot, i; >> >> + qdev_prefw_init_check(); >> + >> /* Initialize PC system firmware */ >> pc_system_firmware_init(rom_memory); >> >> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c >> index 7915b45..bb8e955 100644 >> --- a/hw/qdev-monitor.c >> +++ b/hw/qdev-monitor.c >> @@ -486,6 +486,58 @@ DeviceState *qdev_device_add(QemuOpts *opts) >> return qdev; >> } >> >> +/* >> + * Some devices may need to be initialized before the firmware is >> + * initialized, so they may add information to the firmware >> + * structures. >> + */ >> +typedef struct prefw_list { >> + const char *name; >> + struct prefw_list *next; >> +} prefw_list; >> + >> +prefw_list *prefw_init; >> + >> +void qdev_add_prefw_init(const char *name) >> +{ >> + prefw_list *e = g_malloc(sizeof(*e)); >> + e->name = name; >> + e->next = prefw_init; >> + prefw_init = e; >> +} >> + >> +static int prefw_check_func(QemuOpts *opts, void *opaque) >> +{ >> + DeviceState *dev; >> + const char *driver; >> + prefw_list *e; >> + >> + driver = qemu_opt_get(opts, "driver"); >> + if (!driver) { >> + return 0; >> + } >> + >> + for (e = prefw_init; e; e = e->next) { >> + if (strcmp(driver, e->name) == 0) { >> + dev = qdev_device_add(opts); >> + if (!dev) { >> + return -1; >> + } >> + qemu_opts_del(opts); >> + break; >> + } >> + } >> + return 0; >> +} >> + >> +void qdev_prefw_init_check(void) >> +{ >> + /* init generic devices */ >> + if (qemu_opts_foreach(qemu_find_opts("device"), >> + prefw_check_func, NULL, 1) != 0) { >> + exit(1); >> + } >> +} >> >> #define qdev_printf(fmt, ...) monitor_printf(mon, "%*s" fmt, indent, "", ## __VA_ARGS__) >> static void qbus_print(Monitor *mon, BusState *bus, int indent); >> diff --git a/hw/qdev.h b/hw/qdev.h >> index f4683dc..2be4c95 100644 >> --- a/hw/qdev.h >> +++ b/hw/qdev.h >> @@ -167,6 +167,9 @@ int qdev_simple_unplug_cb(DeviceState *dev); >> void qdev_machine_creation_done(void); >> bool qdev_machine_modified(void); >> >> +void qdev_add_prefw_init(const char *name); >> +void qdev_prefw_init_check(void); >> + >> qemu_irq qdev_get_gpio_in(DeviceState *dev, int n); >> void qdev_connect_gpio_out(DeviceState *dev, int n, qemu_irq pin); >> >> diff --git a/qemu-option.c b/qemu-option.c >> index bb3886c..1d000f8 100644 >> --- a/qemu-option.c >> +++ b/qemu-option.c >> @@ -709,10 +709,10 @@ int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val) >> int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque, >> int abort_on_failure) >> { >> - QemuOpt *opt; >> + QemuOpt *opt, *next_opt; >> int rc = 0; >> >> - QTAILQ_FOREACH(opt, &opts->head, next) { >> + QTAILQ_FOREACH_SAFE(opt, &opts->head, next, next_opt) { >> rc = func(opt->name, opt->str, opaque); >> if (abort_on_failure && rc != 0) >> break; >> >
minyard@acm.org writes: > From: Corey Minyard <cminyard@mvista.com> > > Some devices may need to do some firmware-type initialization before > the firmware itself is initialized. For instance, any device that > adds SMBIOS table entries (like IPMI) will need to do that before > the BIOS is initialized. > > So add a list of devices that get initialized before the firmware > initialization. Since those devices get removed from the list so > they don't get initialized twice, convert the qemu_opt_foreach() to > use the safe list traversal. > > Signed-off-by: Corey Minyard <cminyard@mvista.com> This should go away if you move the SMBIOS changes to BIOS. Regards, Anthony Liguori > --- > hw/pc.c | 2 ++ > hw/qdev-monitor.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > hw/qdev.h | 3 +++ > qemu-option.c | 4 ++-- > 4 files changed, 59 insertions(+), 2 deletions(-) > > diff --git a/hw/pc.c b/hw/pc.c > index bedd5be..811d99c 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -1010,6 +1010,8 @@ void *pc_bios_init(const char *kernel_filename, > void *fw_cfg; > int linux_boot, i; > > + qdev_prefw_init_check(); > + > /* Initialize PC system firmware */ > pc_system_firmware_init(rom_memory); > > diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c > index 7915b45..bb8e955 100644 > --- a/hw/qdev-monitor.c > +++ b/hw/qdev-monitor.c > @@ -486,6 +486,58 @@ DeviceState *qdev_device_add(QemuOpts *opts) > return qdev; > } > > +/* > + * Some devices may need to be initialized before the firmware is > + * initialized, so they may add information to the firmware > + * structures. > + */ > +typedef struct prefw_list { > + const char *name; > + struct prefw_list *next; > +} prefw_list; > + > +prefw_list *prefw_init; > + > +void qdev_add_prefw_init(const char *name) > +{ > + prefw_list *e = g_malloc(sizeof(*e)); > + e->name = name; > + e->next = prefw_init; > + prefw_init = e; > +} > + > +static int prefw_check_func(QemuOpts *opts, void *opaque) > +{ > + DeviceState *dev; > + const char *driver; > + prefw_list *e; > + > + driver = qemu_opt_get(opts, "driver"); > + if (!driver) { > + return 0; > + } > + > + for (e = prefw_init; e; e = e->next) { > + if (strcmp(driver, e->name) == 0) { > + dev = qdev_device_add(opts); > + if (!dev) { > + return -1; > + } > + qemu_opts_del(opts); > + break; > + } > + } > + return 0; > +} > + > +void qdev_prefw_init_check(void) > +{ > + /* init generic devices */ > + if (qemu_opts_foreach(qemu_find_opts("device"), > + prefw_check_func, NULL, 1) != 0) { > + exit(1); > + } > +} > > #define qdev_printf(fmt, ...) monitor_printf(mon, "%*s" fmt, indent, "", ## __VA_ARGS__) > static void qbus_print(Monitor *mon, BusState *bus, int indent); > diff --git a/hw/qdev.h b/hw/qdev.h > index f4683dc..2be4c95 100644 > --- a/hw/qdev.h > +++ b/hw/qdev.h > @@ -167,6 +167,9 @@ int qdev_simple_unplug_cb(DeviceState *dev); > void qdev_machine_creation_done(void); > bool qdev_machine_modified(void); > > +void qdev_add_prefw_init(const char *name); > +void qdev_prefw_init_check(void); > + > qemu_irq qdev_get_gpio_in(DeviceState *dev, int n); > void qdev_connect_gpio_out(DeviceState *dev, int n, qemu_irq pin); > > diff --git a/qemu-option.c b/qemu-option.c > index bb3886c..1d000f8 100644 > --- a/qemu-option.c > +++ b/qemu-option.c > @@ -709,10 +709,10 @@ int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val) > int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque, > int abort_on_failure) > { > - QemuOpt *opt; > + QemuOpt *opt, *next_opt; > int rc = 0; > > - QTAILQ_FOREACH(opt, &opts->head, next) { > + QTAILQ_FOREACH_SAFE(opt, &opts->head, next, next_opt) { > rc = func(opt->name, opt->str, opaque); > if (abort_on_failure && rc != 0) > break; > -- > 1.7.4.1
diff --git a/hw/pc.c b/hw/pc.c index bedd5be..811d99c 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -1010,6 +1010,8 @@ void *pc_bios_init(const char *kernel_filename, void *fw_cfg; int linux_boot, i; + qdev_prefw_init_check(); + /* Initialize PC system firmware */ pc_system_firmware_init(rom_memory); diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c index 7915b45..bb8e955 100644 --- a/hw/qdev-monitor.c +++ b/hw/qdev-monitor.c @@ -486,6 +486,58 @@ DeviceState *qdev_device_add(QemuOpts *opts) return qdev; } +/* + * Some devices may need to be initialized before the firmware is + * initialized, so they may add information to the firmware + * structures. + */ +typedef struct prefw_list { + const char *name; + struct prefw_list *next; +} prefw_list; + +prefw_list *prefw_init; + +void qdev_add_prefw_init(const char *name) +{ + prefw_list *e = g_malloc(sizeof(*e)); + e->name = name; + e->next = prefw_init; + prefw_init = e; +} + +static int prefw_check_func(QemuOpts *opts, void *opaque) +{ + DeviceState *dev; + const char *driver; + prefw_list *e; + + driver = qemu_opt_get(opts, "driver"); + if (!driver) { + return 0; + } + + for (e = prefw_init; e; e = e->next) { + if (strcmp(driver, e->name) == 0) { + dev = qdev_device_add(opts); + if (!dev) { + return -1; + } + qemu_opts_del(opts); + break; + } + } + return 0; +} + +void qdev_prefw_init_check(void) +{ + /* init generic devices */ + if (qemu_opts_foreach(qemu_find_opts("device"), + prefw_check_func, NULL, 1) != 0) { + exit(1); + } +} #define qdev_printf(fmt, ...) monitor_printf(mon, "%*s" fmt, indent, "", ## __VA_ARGS__) static void qbus_print(Monitor *mon, BusState *bus, int indent); diff --git a/hw/qdev.h b/hw/qdev.h index f4683dc..2be4c95 100644 --- a/hw/qdev.h +++ b/hw/qdev.h @@ -167,6 +167,9 @@ int qdev_simple_unplug_cb(DeviceState *dev); void qdev_machine_creation_done(void); bool qdev_machine_modified(void); +void qdev_add_prefw_init(const char *name); +void qdev_prefw_init_check(void); + qemu_irq qdev_get_gpio_in(DeviceState *dev, int n); void qdev_connect_gpio_out(DeviceState *dev, int n, qemu_irq pin); diff --git a/qemu-option.c b/qemu-option.c index bb3886c..1d000f8 100644 --- a/qemu-option.c +++ b/qemu-option.c @@ -709,10 +709,10 @@ int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val) int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque, int abort_on_failure) { - QemuOpt *opt; + QemuOpt *opt, *next_opt; int rc = 0; - QTAILQ_FOREACH(opt, &opts->head, next) { + QTAILQ_FOREACH_SAFE(opt, &opts->head, next, next_opt) { rc = func(opt->name, opt->str, opaque); if (abort_on_failure && rc != 0) break;