| Message ID | 26c36cc56c0dd0021aebc9c2b7f9e6dfa1abb67e.1650379269.git.jag.raman@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | vfio-user server in QEMU | expand |
Jagannathan Raman <jag.raman@oracle.com> writes: > Add blocker to prevent hot-unplug of devices Why do you need this? I'm not doubting you do, I just want to read your reasons here :) > > Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com> > Signed-off-by: John G Johnson <john.g.johnson@oracle.com> > Signed-off-by: Jagannathan Raman <jag.raman@oracle.com> > --- > include/hw/qdev-core.h | 29 +++++++++++++++++++++++++++++ > hw/core/qdev.c | 24 ++++++++++++++++++++++++ > softmmu/qdev-monitor.c | 4 ++++ > 3 files changed, 57 insertions(+) > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index 92c3d65208..1b9fa25e5c 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -193,6 +193,7 @@ struct DeviceState { > int instance_id_alias; > int alias_required_for_version; > ResettableState reset; > + GSList *unplug_blockers; > }; > > struct DeviceListener { > @@ -419,6 +420,34 @@ void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev, > void qdev_machine_creation_done(void); > bool qdev_machine_modified(void); > > +/* > + * qdev_add_unplug_blocker: Adds an unplug blocker to a device > + * > + * @dev: Device to be blocked from unplug > + * @reason: Reason for blocking > + */ > +void qdev_add_unplug_blocker(DeviceState *dev, Error *reason); > + > +/* > + * qdev_del_unplug_blocker: Removes an unplug blocker from a device > + * > + * @dev: Device to be unblocked > + * @reason: Pointer to the Error used with qdev_add_unplug_blocker. > + * Used as a handle to lookup the blocker for deletion. > + */ > +void qdev_del_unplug_blocker(DeviceState *dev, Error *reason); > + > +/* > + * qdev_unplug_blocked: Confirms if a device is blocked from unplug > + * > + * @dev: Device to be tested > + * @reason: Returns one of the reasons why the device is blocked, > + * if any > + * > + * Returns: true if device is blocked from unplug, false otherwise > + */ > +bool qdev_unplug_blocked(DeviceState *dev, Error **errp); > + > /** > * GpioPolarity: Polarity of a GPIO line > * > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 84f3019440..0806d8fcaa 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -468,6 +468,28 @@ char *qdev_get_dev_path(DeviceState *dev) > return NULL; > } > > +void qdev_add_unplug_blocker(DeviceState *dev, Error *reason) > +{ > + dev->unplug_blockers = g_slist_prepend(dev->unplug_blockers, reason); > +} > + > +void qdev_del_unplug_blocker(DeviceState *dev, Error *reason) > +{ > + dev->unplug_blockers = g_slist_remove(dev->unplug_blockers, reason); > +} > + > +bool qdev_unplug_blocked(DeviceState *dev, Error **errp) > +{ > + ERRP_GUARD(); > + > + if (dev->unplug_blockers) { > + error_propagate(errp, error_copy(dev->unplug_blockers->data)); > + return true; > + } > + > + return false; > +} This cites the most recently added blocker as reason. Your function comment covers it: "Returns one of the reasons". Okay. > + > static bool device_get_realized(Object *obj, Error **errp) > { > DeviceState *dev = DEVICE(obj); > @@ -704,6 +726,8 @@ static void device_finalize(Object *obj) > > DeviceState *dev = DEVICE(obj); > > + g_assert(!dev->unplug_blockers); > + > QLIST_FOREACH_SAFE(ngl, &dev->gpios, node, next) { > QLIST_REMOVE(ngl, node); > qemu_free_irqs(ngl->in, ngl->num_in); > diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c > index 12fe60c467..9cfd59d17c 100644 > --- a/softmmu/qdev-monitor.c > +++ b/softmmu/qdev-monitor.c > @@ -898,6 +898,10 @@ void qdev_unplug(DeviceState *dev, Error **errp) > HotplugHandlerClass *hdc; > Error *local_err = NULL; > > + if (qdev_unplug_blocked(dev, errp)) { > + return; > + } > + > if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) { > error_setg(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name); > return;
> On Apr 21, 2022, at 10:55 AM, Markus Armbruster <armbru@redhat.com> wrote: > > Jagannathan Raman <jag.raman@oracle.com> writes: > >> Add blocker to prevent hot-unplug of devices > > Why do you need this? I'm not doubting you do, I just want to read your > reasons here :) Hi Markus, :) The x-vfio-user-server depends on an attached PCIDevice. As long as x-vfio-user-server is used, we don’t want the PCIDevice to be unplugged. This blocker prevents an user from removing PCIDevice while the vfio-user server is in use. > >> >> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com> >> Signed-off-by: John G Johnson <john.g.johnson@oracle.com> >> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com> I recall receiving a “Reviewed-by” from Stefan previously. I’m very sorry I didn’t add that here. I’ll go over all the patches once again to confirm that the “Reviewed-by” status reflects accurately. >> --- >> include/hw/qdev-core.h | 29 +++++++++++++++++++++++++++++ >> hw/core/qdev.c | 24 ++++++++++++++++++++++++ >> softmmu/qdev-monitor.c | 4 ++++ >> 3 files changed, 57 insertions(+) >> >> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h >> index 92c3d65208..1b9fa25e5c 100644 >> --- a/include/hw/qdev-core.h >> +++ b/include/hw/qdev-core.h >> @@ -193,6 +193,7 @@ struct DeviceState { >> int instance_id_alias; >> int alias_required_for_version; >> ResettableState reset; >> + GSList *unplug_blockers; >> }; >> >> struct DeviceListener { >> @@ -419,6 +420,34 @@ void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev, >> void qdev_machine_creation_done(void); >> bool qdev_machine_modified(void); >> >> +/* >> + * qdev_add_unplug_blocker: Adds an unplug blocker to a device >> + * >> + * @dev: Device to be blocked from unplug >> + * @reason: Reason for blocking >> + */ >> +void qdev_add_unplug_blocker(DeviceState *dev, Error *reason); >> + >> +/* >> + * qdev_del_unplug_blocker: Removes an unplug blocker from a device >> + * >> + * @dev: Device to be unblocked >> + * @reason: Pointer to the Error used with qdev_add_unplug_blocker. >> + * Used as a handle to lookup the blocker for deletion. >> + */ >> +void qdev_del_unplug_blocker(DeviceState *dev, Error *reason); >> + >> +/* >> + * qdev_unplug_blocked: Confirms if a device is blocked from unplug >> + * >> + * @dev: Device to be tested >> + * @reason: Returns one of the reasons why the device is blocked, >> + * if any >> + * >> + * Returns: true if device is blocked from unplug, false otherwise >> + */ >> +bool qdev_unplug_blocked(DeviceState *dev, Error **errp); >> + >> /** >> * GpioPolarity: Polarity of a GPIO line >> * >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c >> index 84f3019440..0806d8fcaa 100644 >> --- a/hw/core/qdev.c >> +++ b/hw/core/qdev.c >> @@ -468,6 +468,28 @@ char *qdev_get_dev_path(DeviceState *dev) >> return NULL; >> } >> >> +void qdev_add_unplug_blocker(DeviceState *dev, Error *reason) >> +{ >> + dev->unplug_blockers = g_slist_prepend(dev->unplug_blockers, reason); >> +} >> + >> +void qdev_del_unplug_blocker(DeviceState *dev, Error *reason) >> +{ >> + dev->unplug_blockers = g_slist_remove(dev->unplug_blockers, reason); >> +} >> + >> +bool qdev_unplug_blocked(DeviceState *dev, Error **errp) >> +{ >> + ERRP_GUARD(); >> + >> + if (dev->unplug_blockers) { >> + error_propagate(errp, error_copy(dev->unplug_blockers->data)); >> + return true; >> + } >> + >> + return false; >> +} > > This cites the most recently added blocker as reason. Your function > comment covers it: "Returns one of the reasons". Okay. I could change the comment to say that it returns the recently added reason. Thank you! -- Jag > >> + >> static bool device_get_realized(Object *obj, Error **errp) >> { >> DeviceState *dev = DEVICE(obj); >> @@ -704,6 +726,8 @@ static void device_finalize(Object *obj) >> >> DeviceState *dev = DEVICE(obj); >> >> + g_assert(!dev->unplug_blockers); >> + >> QLIST_FOREACH_SAFE(ngl, &dev->gpios, node, next) { >> QLIST_REMOVE(ngl, node); >> qemu_free_irqs(ngl->in, ngl->num_in); >> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c >> index 12fe60c467..9cfd59d17c 100644 >> --- a/softmmu/qdev-monitor.c >> +++ b/softmmu/qdev-monitor.c >> @@ -898,6 +898,10 @@ void qdev_unplug(DeviceState *dev, Error **errp) >> HotplugHandlerClass *hdc; >> Error *local_err = NULL; >> >> + if (qdev_unplug_blocked(dev, errp)) { >> + return; >> + } >> + >> if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) { >> error_setg(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name); >> return; >
Jag Raman <jag.raman@oracle.com> writes: >> On Apr 21, 2022, at 10:55 AM, Markus Armbruster <armbru@redhat.com> wrote: >> >> Jagannathan Raman <jag.raman@oracle.com> writes: >> >>> Add blocker to prevent hot-unplug of devices >> >> Why do you need this? I'm not doubting you do, I just want to read your >> reasons here :) > > Hi Markus, :) > > The x-vfio-user-server depends on an attached PCIDevice. As long as x-vfio-user-server > is used, we don’t want the PCIDevice to be unplugged. This blocker prevents an user > from removing PCIDevice while the vfio-user server is in use. Please work that into your commit message. Perhaps along the lines of One of the next commits will do <stuff>. <badness> will happen when the PCI device is unplugged. Create the means to prevent that. >>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com> >>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com> >>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com> > > I recall receiving a “Reviewed-by” from Stefan previously. > > I’m very sorry I didn’t add that here. I’ll go over all the patches once again to confirm that > the “Reviewed-by” status reflects accurately. > >>> --- >>> include/hw/qdev-core.h | 29 +++++++++++++++++++++++++++++ >>> hw/core/qdev.c | 24 ++++++++++++++++++++++++ >>> softmmu/qdev-monitor.c | 4 ++++ >>> 3 files changed, 57 insertions(+) >>> >>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h >>> index 92c3d65208..1b9fa25e5c 100644 >>> --- a/include/hw/qdev-core.h >>> +++ b/include/hw/qdev-core.h >>> @@ -193,6 +193,7 @@ struct DeviceState { >>> int instance_id_alias; >>> int alias_required_for_version; >>> ResettableState reset; >>> + GSList *unplug_blockers; >>> }; >>> >>> struct DeviceListener { >>> @@ -419,6 +420,34 @@ void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev, >>> void qdev_machine_creation_done(void); >>> bool qdev_machine_modified(void); >>> >>> +/* >>> + * qdev_add_unplug_blocker: Adds an unplug blocker to a device >>> + * >>> + * @dev: Device to be blocked from unplug >>> + * @reason: Reason for blocking >>> + */ >>> +void qdev_add_unplug_blocker(DeviceState *dev, Error *reason); >>> + >>> +/* >>> + * qdev_del_unplug_blocker: Removes an unplug blocker from a device >>> + * >>> + * @dev: Device to be unblocked >>> + * @reason: Pointer to the Error used with qdev_add_unplug_blocker. >>> + * Used as a handle to lookup the blocker for deletion. >>> + */ >>> +void qdev_del_unplug_blocker(DeviceState *dev, Error *reason); >>> + >>> +/* >>> + * qdev_unplug_blocked: Confirms if a device is blocked from unplug >>> + * >>> + * @dev: Device to be tested >>> + * @reason: Returns one of the reasons why the device is blocked, >>> + * if any >>> + * >>> + * Returns: true if device is blocked from unplug, false otherwise >>> + */ >>> +bool qdev_unplug_blocked(DeviceState *dev, Error **errp); >>> + >>> /** >>> * GpioPolarity: Polarity of a GPIO line >>> * >>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c >>> index 84f3019440..0806d8fcaa 100644 >>> --- a/hw/core/qdev.c >>> +++ b/hw/core/qdev.c >>> @@ -468,6 +468,28 @@ char *qdev_get_dev_path(DeviceState *dev) >>> return NULL; >>> } >>> >>> +void qdev_add_unplug_blocker(DeviceState *dev, Error *reason) >>> +{ >>> + dev->unplug_blockers = g_slist_prepend(dev->unplug_blockers, reason); >>> +} >>> + >>> +void qdev_del_unplug_blocker(DeviceState *dev, Error *reason) >>> +{ >>> + dev->unplug_blockers = g_slist_remove(dev->unplug_blockers, reason); >>> +} >>> + >>> +bool qdev_unplug_blocked(DeviceState *dev, Error **errp) >>> +{ >>> + ERRP_GUARD(); >>> + >>> + if (dev->unplug_blockers) { >>> + error_propagate(errp, error_copy(dev->unplug_blockers->data)); >>> + return true; >>> + } >>> + >>> + return false; >>> +} >> >> This cites the most recently added blocker as reason. Your function >> comment covers it: "Returns one of the reasons". Okay. > > I could change the comment to say that it returns the recently added reason. Up to you.
> On Apr 22, 2022, at 1:18 AM, Markus Armbruster <armbru@redhat.com> wrote: > > Jag Raman <jag.raman@oracle.com> writes: > >>> On Apr 21, 2022, at 10:55 AM, Markus Armbruster <armbru@redhat.com> wrote: >>> >>> Jagannathan Raman <jag.raman@oracle.com> writes: >>> >>>> Add blocker to prevent hot-unplug of devices >>> >>> Why do you need this? I'm not doubting you do, I just want to read your >>> reasons here :) >> >> Hi Markus, :) >> >> The x-vfio-user-server depends on an attached PCIDevice. As long as x-vfio-user-server >> is used, we don’t want the PCIDevice to be unplugged. This blocker prevents an user >> from removing PCIDevice while the vfio-user server is in use. > > Please work that into your commit message. Perhaps along the lines of > > One of the next commits will do <stuff>. <badness> will happen when > the PCI device is unplugged. Create the means to prevent that. OK, will do. Thank you! -- Jag > >>>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com> >>>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com> >>>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com> >> >> I recall receiving a “Reviewed-by” from Stefan previously. >> >> I’m very sorry I didn’t add that here. I’ll go over all the patches once again to confirm that >> the “Reviewed-by” status reflects accurately. >> >>>> --- >>>> include/hw/qdev-core.h | 29 +++++++++++++++++++++++++++++ >>>> hw/core/qdev.c | 24 ++++++++++++++++++++++++ >>>> softmmu/qdev-monitor.c | 4 ++++ >>>> 3 files changed, 57 insertions(+) >>>> >>>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h >>>> index 92c3d65208..1b9fa25e5c 100644 >>>> --- a/include/hw/qdev-core.h >>>> +++ b/include/hw/qdev-core.h >>>> @@ -193,6 +193,7 @@ struct DeviceState { >>>> int instance_id_alias; >>>> int alias_required_for_version; >>>> ResettableState reset; >>>> + GSList *unplug_blockers; >>>> }; >>>> >>>> struct DeviceListener { >>>> @@ -419,6 +420,34 @@ void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev, >>>> void qdev_machine_creation_done(void); >>>> bool qdev_machine_modified(void); >>>> >>>> +/* >>>> + * qdev_add_unplug_blocker: Adds an unplug blocker to a device >>>> + * >>>> + * @dev: Device to be blocked from unplug >>>> + * @reason: Reason for blocking >>>> + */ >>>> +void qdev_add_unplug_blocker(DeviceState *dev, Error *reason); >>>> + >>>> +/* >>>> + * qdev_del_unplug_blocker: Removes an unplug blocker from a device >>>> + * >>>> + * @dev: Device to be unblocked >>>> + * @reason: Pointer to the Error used with qdev_add_unplug_blocker. >>>> + * Used as a handle to lookup the blocker for deletion. >>>> + */ >>>> +void qdev_del_unplug_blocker(DeviceState *dev, Error *reason); >>>> + >>>> +/* >>>> + * qdev_unplug_blocked: Confirms if a device is blocked from unplug >>>> + * >>>> + * @dev: Device to be tested >>>> + * @reason: Returns one of the reasons why the device is blocked, >>>> + * if any >>>> + * >>>> + * Returns: true if device is blocked from unplug, false otherwise >>>> + */ >>>> +bool qdev_unplug_blocked(DeviceState *dev, Error **errp); >>>> + >>>> /** >>>> * GpioPolarity: Polarity of a GPIO line >>>> * >>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c >>>> index 84f3019440..0806d8fcaa 100644 >>>> --- a/hw/core/qdev.c >>>> +++ b/hw/core/qdev.c >>>> @@ -468,6 +468,28 @@ char *qdev_get_dev_path(DeviceState *dev) >>>> return NULL; >>>> } >>>> >>>> +void qdev_add_unplug_blocker(DeviceState *dev, Error *reason) >>>> +{ >>>> + dev->unplug_blockers = g_slist_prepend(dev->unplug_blockers, reason); >>>> +} >>>> + >>>> +void qdev_del_unplug_blocker(DeviceState *dev, Error *reason) >>>> +{ >>>> + dev->unplug_blockers = g_slist_remove(dev->unplug_blockers, reason); >>>> +} >>>> + >>>> +bool qdev_unplug_blocked(DeviceState *dev, Error **errp) >>>> +{ >>>> + ERRP_GUARD(); >>>> + >>>> + if (dev->unplug_blockers) { >>>> + error_propagate(errp, error_copy(dev->unplug_blockers->data)); >>>> + return true; >>>> + } >>>> + >>>> + return false; >>>> +} >>> >>> This cites the most recently added blocker as reason. Your function >>> comment covers it: "Returns one of the reasons". Okay. >> >> I could change the comment to say that it returns the recently added reason. > > Up to you. >
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 92c3d65208..1b9fa25e5c 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -193,6 +193,7 @@ struct DeviceState { int instance_id_alias; int alias_required_for_version; ResettableState reset; + GSList *unplug_blockers; }; struct DeviceListener { @@ -419,6 +420,34 @@ void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev, void qdev_machine_creation_done(void); bool qdev_machine_modified(void); +/* + * qdev_add_unplug_blocker: Adds an unplug blocker to a device + * + * @dev: Device to be blocked from unplug + * @reason: Reason for blocking + */ +void qdev_add_unplug_blocker(DeviceState *dev, Error *reason); + +/* + * qdev_del_unplug_blocker: Removes an unplug blocker from a device + * + * @dev: Device to be unblocked + * @reason: Pointer to the Error used with qdev_add_unplug_blocker. + * Used as a handle to lookup the blocker for deletion. + */ +void qdev_del_unplug_blocker(DeviceState *dev, Error *reason); + +/* + * qdev_unplug_blocked: Confirms if a device is blocked from unplug + * + * @dev: Device to be tested + * @reason: Returns one of the reasons why the device is blocked, + * if any + * + * Returns: true if device is blocked from unplug, false otherwise + */ +bool qdev_unplug_blocked(DeviceState *dev, Error **errp); + /** * GpioPolarity: Polarity of a GPIO line * diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 84f3019440..0806d8fcaa 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -468,6 +468,28 @@ char *qdev_get_dev_path(DeviceState *dev) return NULL; } +void qdev_add_unplug_blocker(DeviceState *dev, Error *reason) +{ + dev->unplug_blockers = g_slist_prepend(dev->unplug_blockers, reason); +} + +void qdev_del_unplug_blocker(DeviceState *dev, Error *reason) +{ + dev->unplug_blockers = g_slist_remove(dev->unplug_blockers, reason); +} + +bool qdev_unplug_blocked(DeviceState *dev, Error **errp) +{ + ERRP_GUARD(); + + if (dev->unplug_blockers) { + error_propagate(errp, error_copy(dev->unplug_blockers->data)); + return true; + } + + return false; +} + static bool device_get_realized(Object *obj, Error **errp) { DeviceState *dev = DEVICE(obj); @@ -704,6 +726,8 @@ static void device_finalize(Object *obj) DeviceState *dev = DEVICE(obj); + g_assert(!dev->unplug_blockers); + QLIST_FOREACH_SAFE(ngl, &dev->gpios, node, next) { QLIST_REMOVE(ngl, node); qemu_free_irqs(ngl->in, ngl->num_in); diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c index 12fe60c467..9cfd59d17c 100644 --- a/softmmu/qdev-monitor.c +++ b/softmmu/qdev-monitor.c @@ -898,6 +898,10 @@ void qdev_unplug(DeviceState *dev, Error **errp) HotplugHandlerClass *hdc; Error *local_err = NULL; + if (qdev_unplug_blocked(dev, errp)) { + return; + } + if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) { error_setg(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name); return;