| Message ID | 20210922161405.140018-11-damien.hedde@greensocs.com |
|---|---|
| State | New |
| Headers | show |
| Series | Initial support for machine creation via QMP | expand |
On Wed, 22 Sep 2021, Damien Hedde wrote: > Skip the sysbus device type per-machine allow-list check before the > MACHINE_INIT_PHASE_READY phase. > > This patch permits adding any sysbus device (it still needs to be > user_creatable) when using the -preconfig experimental option. > > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> > --- > > This commit is RFC. Depending on the condition to allow a device > to be added, it may change. > --- > softmmu/qdev-monitor.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c > index f1c9242855..73b991adda 100644 > --- a/softmmu/qdev-monitor.c > +++ b/softmmu/qdev-monitor.c > @@ -269,8 +269,13 @@ static DeviceClass *qdev_get_device_class(const char **driver, Error **errp) > return NULL; > } > > - if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE)) { > - /* sysbus devices need to be allowed by the machine */ > + if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE) && > + phase_check(MACHINE_INIT_PHASE_READY)) { > + /* > + * Sysbus devices need to be allowed by the machine. > + * We only check that after the machine is ready in order to let > + * us add any user_creatable sysbus device during machine creation. > + */ > MachineClass *mc = MACHINE_CLASS(object_get_class(qdev_get_machine())); > if (!machine_class_is_dynamic_sysbus_dev_allowed(mc, *driver)) { > error_setg(errp, "'%s' is not an allowed pluggable sysbus device " Since now you are adding the state of the machine creation in the valiation condition, the failure error message becomes misleading. Better to do this I think : if (object class is TYPE_SYS_BUS_DEVICE) { if (!phase_check(MACHINE_INIT_PHASE_READY)) { // error out here saying the state of the machine creation is too early } // do the other check on dynamic sysbus }
On Thu, 23 Sep 2021, Ani Sinha wrote: > > > On Wed, 22 Sep 2021, Damien Hedde wrote: > > > Skip the sysbus device type per-machine allow-list check before the > > MACHINE_INIT_PHASE_READY phase. > > > > This patch permits adding any sysbus device (it still needs to be > > user_creatable) when using the -preconfig experimental option. > > > > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> > > --- > > > > This commit is RFC. Depending on the condition to allow a device > > to be added, it may change. > > --- > > softmmu/qdev-monitor.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c > > index f1c9242855..73b991adda 100644 > > --- a/softmmu/qdev-monitor.c > > +++ b/softmmu/qdev-monitor.c > > @@ -269,8 +269,13 @@ static DeviceClass *qdev_get_device_class(const char **driver, Error **errp) > > return NULL; > > } > > > > - if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE)) { > > - /* sysbus devices need to be allowed by the machine */ > > + if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE) && > > + phase_check(MACHINE_INIT_PHASE_READY)) { > > + /* > > + * Sysbus devices need to be allowed by the machine. > > + * We only check that after the machine is ready in order to let > > + * us add any user_creatable sysbus device during machine creation. > > + */ > > MachineClass *mc = MACHINE_CLASS(object_get_class(qdev_get_machine())); > > if (!machine_class_is_dynamic_sysbus_dev_allowed(mc, *driver)) { > > error_setg(errp, "'%s' is not an allowed pluggable sysbus device " > > Since now you are adding the state of the machine creation in the > valiation condition, the failure error message becomes misleading. > Better to do this I think : > > if (object class is TYPE_SYS_BUS_DEVICE) > { > if (!phase_check(MACHINE_INIT_PHASE_READY)) > { > // error out here saying the state of the machine creation is too > early > } > > // do the other check on dynamic sysbus > > } The other thing to consider is whether we should put the machine phaze check at a higher level, at qdev_device_add() perhaps. One might envison that different device types may be allowed to be added at different stages of machine creation. So this check needs be more generalized for the future.
On 9/23/21 13:55, Ani Sinha wrote: > > > On Thu, 23 Sep 2021, Ani Sinha wrote: > >> >> >> On Wed, 22 Sep 2021, Damien Hedde wrote: >> >>> Skip the sysbus device type per-machine allow-list check before the >>> MACHINE_INIT_PHASE_READY phase. >>> >>> This patch permits adding any sysbus device (it still needs to be >>> user_creatable) when using the -preconfig experimental option. >>> >>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> >>> --- >>> >>> This commit is RFC. Depending on the condition to allow a device >>> to be added, it may change. >>> --- >>> softmmu/qdev-monitor.c | 9 +++++++-- >>> 1 file changed, 7 insertions(+), 2 deletions(-) >>> >>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c >>> index f1c9242855..73b991adda 100644 >>> --- a/softmmu/qdev-monitor.c >>> +++ b/softmmu/qdev-monitor.c >>> @@ -269,8 +269,13 @@ static DeviceClass *qdev_get_device_class(const char **driver, Error **errp) >>> return NULL; >>> } >>> >>> - if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE)) { >>> - /* sysbus devices need to be allowed by the machine */ >>> + if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE) && >>> + phase_check(MACHINE_INIT_PHASE_READY)) { >>> + /* >>> + * Sysbus devices need to be allowed by the machine. >>> + * We only check that after the machine is ready in order to let >>> + * us add any user_creatable sysbus device during machine creation. >>> + */ >>> MachineClass *mc = MACHINE_CLASS(object_get_class(qdev_get_machine())); >>> if (!machine_class_is_dynamic_sysbus_dev_allowed(mc, *driver)) { >>> error_setg(errp, "'%s' is not an allowed pluggable sysbus device " >> >> Since now you are adding the state of the machine creation in the >> valiation condition, the failure error message becomes misleading. >> Better to do this I think : >> >> if (object class is TYPE_SYS_BUS_DEVICE) >> { >> if (!phase_check(MACHINE_INIT_PHASE_READY)) >> { >> // error out here saying the state of the machine creation is too >> early >> } >> >> // do the other check on dynamic sysbus >> >> } > > The other thing to consider is whether we should put the machine phaze > check at a higher level, at qdev_device_add() perhaps. One might envison > that different device types may be allowed to be added at different > stages of machine creation. So this check needs be more generalized for > the future. > Hi Ani, I think moving out the allowance check from qdev_get_device_class is a good idea. The code will be more clear even if the check is not generalized. Thanks, -- Damien
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c index f1c9242855..73b991adda 100644 --- a/softmmu/qdev-monitor.c +++ b/softmmu/qdev-monitor.c @@ -269,8 +269,13 @@ static DeviceClass *qdev_get_device_class(const char **driver, Error **errp) return NULL; } - if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE)) { - /* sysbus devices need to be allowed by the machine */ + if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE) && + phase_check(MACHINE_INIT_PHASE_READY)) { + /* + * Sysbus devices need to be allowed by the machine. + * We only check that after the machine is ready in order to let + * us add any user_creatable sysbus device during machine creation. + */ MachineClass *mc = MACHINE_CLASS(object_get_class(qdev_get_machine())); if (!machine_class_is_dynamic_sysbus_dev_allowed(mc, *driver)) { error_setg(errp, "'%s' is not an allowed pluggable sysbus device "
Skip the sysbus device type per-machine allow-list check before the MACHINE_INIT_PHASE_READY phase. This patch permits adding any sysbus device (it still needs to be user_creatable) when using the -preconfig experimental option. Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> --- This commit is RFC. Depending on the condition to allow a device to be added, it may change. --- softmmu/qdev-monitor.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)