| Message ID | 1396618620-27823-9-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/05/2014 12:36 AM, Igor Mammedov wrote: > Adds get_hotplug_handler() method to machine, and > makes bus-less device to use it during hotplug > as a means to discover hotplug handler controller. > Returned controller is used to permorm a hotplug > action. "returned controller" is a machine here? Why not to make a memory controller + bus instead? I thought this is the preferred approach in qom'ed QEMU :) > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/core/qdev.c | 13 +++++++++++++ > include/hw/boards.h | 8 ++++++++ > 2 files changed, 21 insertions(+) > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 60f9df1..50bb8f5 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -34,6 +34,7 @@ > #include "qapi/qmp/qjson.h" > #include "monitor/monitor.h" > #include "hw/hotplug.h" > +#include "hw/boards.h" > > int qdev_hotplug = 0; > static bool qdev_hot_added = false; > @@ -761,6 +762,18 @@ static void device_set_realized(Object *obj, bool value, Error **err) > local_err == NULL) { > hotplug_handler_plug(dev->parent_bus->hotplug_handler, > dev, &local_err); > + } else if (local_err == NULL && > + object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) { > + HotplugHandler *hotplug_ctrl; > + MachineState *machine = MACHINE(qdev_get_machine()); > + MachineClass *mc = MACHINE_GET_CLASS(machine); > + > + if (mc->get_hotplug_handler) { > + hotplug_ctrl = mc->get_hotplug_handler(machine, dev); > + if (hotplug_ctrl) { > + hotplug_handler_plug(hotplug_ctrl, dev, &local_err); > + } > + } > } > > if (qdev_get_vmsd(dev) && local_err == NULL) { > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 3567190..67750b5 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -73,6 +73,11 @@ extern MachineState *current_machine; > /** > * MachineClass: > * @qemu_machine: #QEMUMachine > + * @get_hotplug_handler: this function is called during bus-less > + * device hotplug. If defined it returns pointer to an instance > + * of HotplugHandler object, which handles hotplug operation > + * for a given @dev. It may return NULL if @dev doesn't require > + * any actions to be performed by hotplug handler. > */ > struct MachineClass { > /*< private >*/ > @@ -80,6 +85,9 @@ struct MachineClass { > /*< public >*/ > > QEMUMachine *qemu_machine; > + > + HotplugHandler *(*get_hotplug_handler)(MachineState *machine, > + DeviceState *dev); > }; > > /** >
On Mon, 07 Apr 2014 12:26:23 +1000 Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > On 04/05/2014 12:36 AM, Igor Mammedov wrote: > > Adds get_hotplug_handler() method to machine, and > > makes bus-less device to use it during hotplug > > as a means to discover hotplug handler controller. > > Returned controller is used to permorm a hotplug > > action. > > > "returned controller" is a machine here? not necessary, it could be any device that implements HotplugHandler interface. Machine only provides simple means to get it is a simplified version of https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg04356.html as suggested by Paolo. > > Why not to make a memory controller + bus instead? I thought this is the > preferred approach in qom'ed QEMU :) It's generic mechanism that could be used for hotplugging any bus-less device. As regarding bus approach that was used for memory hotplug in V7, Andreas've made suggestion to get rid of it as it doesn't map well for generic DIMM device and might be problematic to reuse on other targets. In V7 bus was used only as a means to discover a hotplug handler, with this path bus is not needed anymore. > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > hw/core/qdev.c | 13 +++++++++++++ > > include/hw/boards.h | 8 ++++++++ > > 2 files changed, 21 insertions(+) > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > index 60f9df1..50bb8f5 100644 > > --- a/hw/core/qdev.c > > +++ b/hw/core/qdev.c > > @@ -34,6 +34,7 @@ > > #include "qapi/qmp/qjson.h" > > #include "monitor/monitor.h" > > #include "hw/hotplug.h" > > +#include "hw/boards.h" > > > > int qdev_hotplug = 0; > > static bool qdev_hot_added = false; > > @@ -761,6 +762,18 @@ static void device_set_realized(Object *obj, bool value, Error **err) > > local_err == NULL) { > > hotplug_handler_plug(dev->parent_bus->hotplug_handler, > > dev, &local_err); > > + } else if (local_err == NULL && > > + object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) { > > + HotplugHandler *hotplug_ctrl; > > + MachineState *machine = MACHINE(qdev_get_machine()); > > + MachineClass *mc = MACHINE_GET_CLASS(machine); > > + > > + if (mc->get_hotplug_handler) { > > + hotplug_ctrl = mc->get_hotplug_handler(machine, dev); > > + if (hotplug_ctrl) { > > + hotplug_handler_plug(hotplug_ctrl, dev, &local_err); > > + } > > + } > > } > > > > if (qdev_get_vmsd(dev) && local_err == NULL) { > > diff --git a/include/hw/boards.h b/include/hw/boards.h > > index 3567190..67750b5 100644 > > --- a/include/hw/boards.h > > +++ b/include/hw/boards.h > > @@ -73,6 +73,11 @@ extern MachineState *current_machine; > > /** > > * MachineClass: > > * @qemu_machine: #QEMUMachine > > + * @get_hotplug_handler: this function is called during bus-less > > + * device hotplug. If defined it returns pointer to an instance > > + * of HotplugHandler object, which handles hotplug operation > > + * for a given @dev. It may return NULL if @dev doesn't require > > + * any actions to be performed by hotplug handler. > > */ > > struct MachineClass { > > /*< private >*/ > > @@ -80,6 +85,9 @@ struct MachineClass { > > /*< public >*/ > > > > QEMUMachine *qemu_machine; > > + > > + HotplugHandler *(*get_hotplug_handler)(MachineState *machine, > > + DeviceState *dev); > > }; > > > > /** > > > >
Am 07.04.2014 04:26, schrieb Alexey Kardashevskiy: > Why not to make a memory controller + bus instead? I thought this is the > preferred approach in qom'ed QEMU :) That's the historic approach of qdev, not QOM. See: http://www.linux-kvm.org/wiki/images/0/0b/Kvm-forum-2013-Modern-QEMU-devices.pdf http://wiki.qemu-project.org/Features/QOM Andreas
On Fri, Apr 4, 2014 at 11:36 PM, Igor Mammedov <imammedo@redhat.com> wrote: > Adds get_hotplug_handler() method to machine, and > makes bus-less device to use it during hotplug > as a means to discover hotplug handler controller. > Returned controller is used to permorm a hotplug > action. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/core/qdev.c | 13 +++++++++++++ > include/hw/boards.h | 8 ++++++++ > 2 files changed, 21 insertions(+) > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 60f9df1..50bb8f5 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -34,6 +34,7 @@ > #include "qapi/qmp/qjson.h" > #include "monitor/monitor.h" > #include "hw/hotplug.h" > +#include "hw/boards.h" > > int qdev_hotplug = 0; > static bool qdev_hot_added = false; > @@ -761,6 +762,18 @@ static void device_set_realized(Object *obj, bool value, Error **err) > local_err == NULL) { > hotplug_handler_plug(dev->parent_bus->hotplug_handler, > dev, &local_err); > + } else if (local_err == NULL && > + object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) { This doesn't look right - you are relying on global state to implement hotplug. Later in the series you then need to do RTTI at the machine level to properly determine if hotplug is really appropriate then do some machine specific attachment. This idea of "we dont need a bus just hotplug to the machine itself" doesnt seem generic at all. If you need to define hotplug socket-side functionality, then that seems to me to be a bus level problem - and you should use a bus - probably SYSBUS or an extension thereof. Then your hotplug work can be generalized to sysbus and a range of devices rather than us having independent competing "embedded vs PC" hotplug implementations. How do you implement hotplugging to multiple completely independent DIMM slots? (i.e. two slots at completely different places in the bus heir-achy). Regarding DIMM, I think it is a bus. I'm not sure if it actually needs its own class yet (TBH I haven't gone line-line on this series looking for DIMMisms). It is ultimately a memory mapped bus if anything I think it should be: .name = TYPE_DIMM_DEVICE, .parent = TYPE_SYSBUS_DEVICE, .name = TYPE_DIMM_SLOT, .parent = TYPE_SYSTEM_BUS, It is true that we still need to remove the global stateness from sysbus itself (there are a few threads on list on the issue) before this can really be nice. Regards, Peter > + HotplugHandler *hotplug_ctrl; > + MachineState *machine = MACHINE(qdev_get_machine()); > + MachineClass *mc = MACHINE_GET_CLASS(machine); > + > + if (mc->get_hotplug_handler) { > + hotplug_ctrl = mc->get_hotplug_handler(machine, dev); > + if (hotplug_ctrl) { > + hotplug_handler_plug(hotplug_ctrl, dev, &local_err); > + } > + } > } > > if (qdev_get_vmsd(dev) && local_err == NULL) { > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 3567190..67750b5 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -73,6 +73,11 @@ extern MachineState *current_machine; > /** > * MachineClass: > * @qemu_machine: #QEMUMachine > + * @get_hotplug_handler: this function is called during bus-less > + * device hotplug. If defined it returns pointer to an instance > + * of HotplugHandler object, which handles hotplug operation > + * for a given @dev. It may return NULL if @dev doesn't require > + * any actions to be performed by hotplug handler. > */ > struct MachineClass { > /*< private >*/ > @@ -80,6 +85,9 @@ struct MachineClass { > /*< public >*/ > > QEMUMachine *qemu_machine; > + > + HotplugHandler *(*get_hotplug_handler)(MachineState *machine, > + DeviceState *dev); > }; > > /** > -- > 1.9.0 > >
On Thu, 17 Apr 2014 09:46:28 +1000 Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Fri, Apr 4, 2014 at 11:36 PM, Igor Mammedov <imammedo@redhat.com> wrote: > > Adds get_hotplug_handler() method to machine, and > > makes bus-less device to use it during hotplug > > as a means to discover hotplug handler controller. > > Returned controller is used to permorm a hotplug > > action. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > hw/core/qdev.c | 13 +++++++++++++ > > include/hw/boards.h | 8 ++++++++ > > 2 files changed, 21 insertions(+) > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > index 60f9df1..50bb8f5 100644 > > --- a/hw/core/qdev.c > > +++ b/hw/core/qdev.c > > @@ -34,6 +34,7 @@ > > #include "qapi/qmp/qjson.h" > > #include "monitor/monitor.h" > > #include "hw/hotplug.h" > > +#include "hw/boards.h" > > > > int qdev_hotplug = 0; > > static bool qdev_hot_added = false; > > @@ -761,6 +762,18 @@ static void device_set_realized(Object *obj, bool value, Error **err) > > local_err == NULL) { > > hotplug_handler_plug(dev->parent_bus->hotplug_handler, > > dev, &local_err); > > + } else if (local_err == NULL && > > + object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) { > > This doesn't look right - you are relying on global state to implement > hotplug. Later in the series you then need to do RTTI at the machine > level to properly determine if hotplug is really appropriate then do > some machine specific attachment. This idea of "we dont need a bus > just hotplug to the machine itself" doesnt seem generic at all. If you It's rather "allow to define at board level" what should be hotplug-handler than using hardcoded in bus implementation one. The issue here is to discover which hotplug handler should be used for a bus-less device. Which MachineClass->get_hotplug_handler(machine, dev); does. QEMU so far is using bus to solve it, but it by no means is generic (i.e. applicable only bus-devices). Proposed bus-less hotplug is a simplified solution that is result of https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg04184.html discussion. > need to define hotplug socket-side functionality, then that seems to > me to be a bus level problem - and you should use a bus - probably > SYSBUS or an extension thereof. Then your hotplug work can be Socket object + attached bus is rather workaround due to the lack of bus-less hotplug than a generic solution. > generalized to sysbus and a range of devices rather than us having > independent competing "embedded vs PC" hotplug implementations. SYSBUS are definitely is no go for hotplug, there were numerous attempts to make it hotpluggable during past years, which were immediately rejected. Reasoning in gist was "Sysbus is legacy which shouldn't be used for anything new". That's one of the reasons why x86 APIC is not Sysbus device anymore and is attached to ICC bus. > How do you implement hotplugging to multiple completely independent > DIMM slots? (i.e. two slots at completely different places in the bus > heir-achy). I probably do not understand what is a problem here. Why plugging bus-less DIMM, one would need to care about buses? > Regarding DIMM, I think it is a bus. I'm not sure if it actually needs > its own class yet (TBH I haven't gone line-line on this series looking > for DIMMisms). It is ultimately a memory mapped bus if anything I > think it should be: v7 of this series was using DIMMBus + DIMMDevice, but afaerber was pushing towards making DIMM bus-less device as a more generic solution so that it could be easily reused by other targets. And it's more flexible in case of PC, considering plan to convert initial memory to DIMMs where it would involve low and high memory ranges, proper alignment and placement. It's all very machine specific and using machine level hotplug-handlers allows to do placement/ checks/mapping cleanly on board level where it belongs and having completely usable device by the time device becomes "realized". It's not so with SYSBUS device, I'll comment on your sysbus-memory series so it would be easy to compare approaches. > > .name = TYPE_DIMM_DEVICE, > .parent = TYPE_SYSBUS_DEVICE, > > .name = TYPE_DIMM_SLOT, > .parent = TYPE_SYSTEM_BUS, > > It is true that we still need to remove the global stateness from > sysbus itself (there are a few threads on list on the issue) before > this can really be nice. > > Regards, > Peter > > > + HotplugHandler *hotplug_ctrl; > > + MachineState *machine = MACHINE(qdev_get_machine()); > > + MachineClass *mc = MACHINE_GET_CLASS(machine); > > + > > + if (mc->get_hotplug_handler) { > > + hotplug_ctrl = mc->get_hotplug_handler(machine, dev); > > + if (hotplug_ctrl) { > > + hotplug_handler_plug(hotplug_ctrl, dev, &local_err); > > + } > > + } > > } > > > > if (qdev_get_vmsd(dev) && local_err == NULL) { > > diff --git a/include/hw/boards.h b/include/hw/boards.h > > index 3567190..67750b5 100644 > > --- a/include/hw/boards.h > > +++ b/include/hw/boards.h > > @@ -73,6 +73,11 @@ extern MachineState *current_machine; > > /** > > * MachineClass: > > * @qemu_machine: #QEMUMachine > > + * @get_hotplug_handler: this function is called during bus-less > > + * device hotplug. If defined it returns pointer to an instance > > + * of HotplugHandler object, which handles hotplug operation > > + * for a given @dev. It may return NULL if @dev doesn't require > > + * any actions to be performed by hotplug handler. > > */ > > struct MachineClass { > > /*< private >*/ > > @@ -80,6 +85,9 @@ struct MachineClass { > > /*< public >*/ > > > > QEMUMachine *qemu_machine; > > + > > + HotplugHandler *(*get_hotplug_handler)(MachineState *machine, > > + DeviceState *dev); > > }; > > > > /** > > -- > > 1.9.0 > > > > >
On Thu, Apr 17, 2014 at 7:40 PM, Igor Mammedov <imammedo@redhat.com> wrote: > On Thu, 17 Apr 2014 09:46:28 +1000 > Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > >> On Fri, Apr 4, 2014 at 11:36 PM, Igor Mammedov <imammedo@redhat.com> wrote: >> > Adds get_hotplug_handler() method to machine, and >> > makes bus-less device to use it during hotplug >> > as a means to discover hotplug handler controller. >> > Returned controller is used to permorm a hotplug >> > action. >> > >> > Signed-off-by: Igor Mammedov <imammedo@redhat.com> >> > --- >> > hw/core/qdev.c | 13 +++++++++++++ >> > include/hw/boards.h | 8 ++++++++ >> > 2 files changed, 21 insertions(+) >> > >> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c >> > index 60f9df1..50bb8f5 100644 >> > --- a/hw/core/qdev.c >> > +++ b/hw/core/qdev.c >> > @@ -34,6 +34,7 @@ >> > #include "qapi/qmp/qjson.h" >> > #include "monitor/monitor.h" >> > #include "hw/hotplug.h" >> > +#include "hw/boards.h" >> > >> > int qdev_hotplug = 0; >> > static bool qdev_hot_added = false; >> > @@ -761,6 +762,18 @@ static void device_set_realized(Object *obj, bool value, Error **err) >> > local_err == NULL) { >> > hotplug_handler_plug(dev->parent_bus->hotplug_handler, >> > dev, &local_err); >> > + } else if (local_err == NULL && >> > + object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) { >> >> This doesn't look right - you are relying on global state to implement >> hotplug. Later in the series you then need to do RTTI at the machine >> level to properly determine if hotplug is really appropriate then do >> some machine specific attachment. This idea of "we dont need a bus >> just hotplug to the machine itself" doesnt seem generic at all. If you > It's rather "allow to define at board level" what should be hotplug-handler > than using hardcoded in bus implementation one. > If different buses behave different on hotplug then they should be different busses. > The issue here is to discover which hotplug handler should be used for > a bus-less device. Which MachineClass->get_hotplug_handler(machine, dev); > does. QEMU so far is using bus to solve it, but it by no means is > generic (i.e. applicable only bus-devices). > Proposed bus-less hotplug is a simplified solution that is result of > https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg04184.html > discussion. > >> need to define hotplug socket-side functionality, then that seems to >> me to be a bus level problem - and you should use a bus - probably >> SYSBUS or an extension thereof. Then your hotplug work can be > Socket object + attached bus is rather workaround due to the lack of > bus-less hotplug than a generic solution. > >> generalized to sysbus and a range of devices rather than us having >> independent competing "embedded vs PC" hotplug implementations. > SYSBUS are definitely is no go for hotplug, there were numerous > attempts to make it hotpluggable during past years, which were immediately > rejected. Reasoning in gist was "Sysbus is legacy which shouldn't be used > for anything new". Lets divide and conquer this - I agree Sysbus as a bus (TYPE_SYSTEM_BUS) sucks as we are trying to get rid of busses in favor of linkages etc.. So long term, SYSTEM_BUS should go away (along with TYPE_BUS itself). But TYPE_SYS_BUS_DEVICE should live on. Its a highly useful abstraction which allows you to define devices with any number of memory mapped regions. And this device level API doesn't define any real busisms so it should be possible to convert SYS_BUS_DEVICE to this "bussless" regime you are proposing anyway. Infact I do wonder exactly how hard it would be to patch one of your handlers to just call into the sysbus API and grab the memory regions and attach as you already do for DIMMs. Then you work works for all of sysbus and we can continue doing our embedded thing which some decent code sharing. Regards, Peter That's one of the reasons why x86 APIC is not > Sysbus device anymore and is attached to ICC bus. > >> How do you implement hotplugging to multiple completely independent >> DIMM slots? (i.e. two slots at completely different places in the bus >> heir-achy). > I probably do not understand what is a problem here. > Why plugging bus-less DIMM, one would need to care about buses? > >> Regarding DIMM, I think it is a bus. I'm not sure if it actually needs >> its own class yet (TBH I haven't gone line-line on this series looking >> for DIMMisms). It is ultimately a memory mapped bus if anything I >> think it should be: > v7 of this series was using DIMMBus + DIMMDevice, but afaerber was > pushing towards making DIMM bus-less device as a more generic solution > so that it could be easily reused by other targets. > And it's more flexible in case of PC, considering plan to convert > initial memory to DIMMs where it would involve low and high memory > ranges, proper alignment and placement. It's all very machine specific > and using machine level hotplug-handlers allows to do placement/ > checks/mapping cleanly on board level where it belongs and having > completely usable device by the time device becomes "realized". > > It's not so with SYSBUS device, I'll comment on your sysbus-memory > series so it would be easy to compare approaches. > >> >> .name = TYPE_DIMM_DEVICE, >> .parent = TYPE_SYSBUS_DEVICE, >> >> .name = TYPE_DIMM_SLOT, >> .parent = TYPE_SYSTEM_BUS, >> >> It is true that we still need to remove the global stateness from >> sysbus itself (there are a few threads on list on the issue) before >> this can really be nice. >> >> Regards, >> Peter >> >> > + HotplugHandler *hotplug_ctrl; >> > + MachineState *machine = MACHINE(qdev_get_machine()); >> > + MachineClass *mc = MACHINE_GET_CLASS(machine); >> > + >> > + if (mc->get_hotplug_handler) { >> > + hotplug_ctrl = mc->get_hotplug_handler(machine, dev); >> > + if (hotplug_ctrl) { >> > + hotplug_handler_plug(hotplug_ctrl, dev, &local_err); >> > + } >> > + } >> > } >> > >> > if (qdev_get_vmsd(dev) && local_err == NULL) { >> > diff --git a/include/hw/boards.h b/include/hw/boards.h >> > index 3567190..67750b5 100644 >> > --- a/include/hw/boards.h >> > +++ b/include/hw/boards.h >> > @@ -73,6 +73,11 @@ extern MachineState *current_machine; >> > /** >> > * MachineClass: >> > * @qemu_machine: #QEMUMachine >> > + * @get_hotplug_handler: this function is called during bus-less >> > + * device hotplug. If defined it returns pointer to an instance >> > + * of HotplugHandler object, which handles hotplug operation >> > + * for a given @dev. It may return NULL if @dev doesn't require >> > + * any actions to be performed by hotplug handler. >> > */ >> > struct MachineClass { >> > /*< private >*/ >> > @@ -80,6 +85,9 @@ struct MachineClass { >> > /*< public >*/ >> > >> > QEMUMachine *qemu_machine; >> > + >> > + HotplugHandler *(*get_hotplug_handler)(MachineState *machine, >> > + DeviceState *dev); >> > }; >> > >> > /** >> > -- >> > 1.9.0 >> > >> > >> > >
On Fri, 18 Apr 2014 00:03:57 +1000 Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Thu, Apr 17, 2014 at 7:40 PM, Igor Mammedov <imammedo@redhat.com> wrote: > > On Thu, 17 Apr 2014 09:46:28 +1000 > > Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > > > >> On Fri, Apr 4, 2014 at 11:36 PM, Igor Mammedov <imammedo@redhat.com> wrote: > >> > Adds get_hotplug_handler() method to machine, and > >> > makes bus-less device to use it during hotplug > >> > as a means to discover hotplug handler controller. > >> > Returned controller is used to permorm a hotplug > >> > action. > >> > > >> > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > >> > --- > >> > hw/core/qdev.c | 13 +++++++++++++ > >> > include/hw/boards.h | 8 ++++++++ > >> > 2 files changed, 21 insertions(+) > >> > > >> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > >> > index 60f9df1..50bb8f5 100644 > >> > --- a/hw/core/qdev.c > >> > +++ b/hw/core/qdev.c > >> > @@ -34,6 +34,7 @@ > >> > #include "qapi/qmp/qjson.h" > >> > #include "monitor/monitor.h" > >> > #include "hw/hotplug.h" > >> > +#include "hw/boards.h" > >> > > >> > int qdev_hotplug = 0; > >> > static bool qdev_hot_added = false; > >> > @@ -761,6 +762,18 @@ static void device_set_realized(Object *obj, bool value, Error **err) > >> > local_err == NULL) { > >> > hotplug_handler_plug(dev->parent_bus->hotplug_handler, > >> > dev, &local_err); > >> > + } else if (local_err == NULL && > >> > + object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) { > >> > >> This doesn't look right - you are relying on global state to implement > >> hotplug. Later in the series you then need to do RTTI at the machine > >> level to properly determine if hotplug is really appropriate then do > >> some machine specific attachment. This idea of "we dont need a bus > >> just hotplug to the machine itself" doesnt seem generic at all. If you > > It's rather "allow to define at board level" what should be hotplug-handler > > than using hardcoded in bus implementation one. > > > > If different buses behave different on hotplug then they should be > different busses. > > > The issue here is to discover which hotplug handler should be used for > > a bus-less device. Which MachineClass->get_hotplug_handler(machine, dev); > > does. QEMU so far is using bus to solve it, but it by no means is > > generic (i.e. applicable only bus-devices). > > Proposed bus-less hotplug is a simplified solution that is result of > > https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg04184.html > > discussion. > > > >> need to define hotplug socket-side functionality, then that seems to > >> me to be a bus level problem - and you should use a bus - probably > >> SYSBUS or an extension thereof. Then your hotplug work can be > > Socket object + attached bus is rather workaround due to the lack of > > bus-less hotplug than a generic solution. > > > >> generalized to sysbus and a range of devices rather than us having > >> independent competing "embedded vs PC" hotplug implementations. > > SYSBUS are definitely is no go for hotplug, there were numerous > > attempts to make it hotpluggable during past years, which were immediately > > rejected. Reasoning in gist was "Sysbus is legacy which shouldn't be used > > for anything new". > > Lets divide and conquer this - I agree Sysbus as a bus > (TYPE_SYSTEM_BUS) sucks as we are trying to get rid of busses in favor > of linkages etc.. So long term, SYSTEM_BUS should go away (along with > TYPE_BUS itself). > > But TYPE_SYS_BUS_DEVICE should live on. Its a highly useful > abstraction which allows you to define devices with any number of > memory mapped regions. And this device level API doesn't define any > real busisms so it should be possible to convert SYS_BUS_DEVICE to > this "bussless" regime you are proposing anyway. Infact I do wonder > exactly how hard it would be to patch one of your handlers to just > call into the sysbus API and grab the memory regions and attach as you > already do for DIMMs. Then you work works for all of sysbus and we can > continue doing our embedded thing which some decent code sharing. It shouldn't be too hard to do globally if we initialize RAM address space in common QEMUMachine and provide default handler in there, that intercepts TYPE_SYS_BUS_DEVICE and does mapping of regions prepared by device. > Regards, > Peter > > That's one of the reasons why x86 APIC is not > > Sysbus device anymore and is attached to ICC bus. > > > >> How do you implement hotplugging to multiple completely independent > >> DIMM slots? (i.e. two slots at completely different places in the bus > >> heir-achy). > > I probably do not understand what is a problem here. > > Why plugging bus-less DIMM, one would need to care about buses? > > > >> Regarding DIMM, I think it is a bus. I'm not sure if it actually needs > >> its own class yet (TBH I haven't gone line-line on this series looking > >> for DIMMisms). It is ultimately a memory mapped bus if anything I > >> think it should be: > > v7 of this series was using DIMMBus + DIMMDevice, but afaerber was > > pushing towards making DIMM bus-less device as a more generic solution > > so that it could be easily reused by other targets. > > And it's more flexible in case of PC, considering plan to convert > > initial memory to DIMMs where it would involve low and high memory > > ranges, proper alignment and placement. It's all very machine specific > > and using machine level hotplug-handlers allows to do placement/ > > checks/mapping cleanly on board level where it belongs and having > > completely usable device by the time device becomes "realized". > > > > It's not so with SYSBUS device, I'll comment on your sysbus-memory > > series so it would be easy to compare approaches. > > > >> > >> .name = TYPE_DIMM_DEVICE, > >> .parent = TYPE_SYSBUS_DEVICE, > >> > >> .name = TYPE_DIMM_SLOT, > >> .parent = TYPE_SYSTEM_BUS, > >> > >> It is true that we still need to remove the global stateness from > >> sysbus itself (there are a few threads on list on the issue) before > >> this can really be nice. > >> > >> Regards, > >> Peter > >> > >> > + HotplugHandler *hotplug_ctrl; > >> > + MachineState *machine = MACHINE(qdev_get_machine()); > >> > + MachineClass *mc = MACHINE_GET_CLASS(machine); > >> > + > >> > + if (mc->get_hotplug_handler) { > >> > + hotplug_ctrl = mc->get_hotplug_handler(machine, dev); > >> > + if (hotplug_ctrl) { > >> > + hotplug_handler_plug(hotplug_ctrl, dev, &local_err); > >> > + } > >> > + } > >> > } > >> > > >> > if (qdev_get_vmsd(dev) && local_err == NULL) { > >> > diff --git a/include/hw/boards.h b/include/hw/boards.h > >> > index 3567190..67750b5 100644 > >> > --- a/include/hw/boards.h > >> > +++ b/include/hw/boards.h > >> > @@ -73,6 +73,11 @@ extern MachineState *current_machine; > >> > /** > >> > * MachineClass: > >> > * @qemu_machine: #QEMUMachine > >> > + * @get_hotplug_handler: this function is called during bus-less > >> > + * device hotplug. If defined it returns pointer to an instance > >> > + * of HotplugHandler object, which handles hotplug operation > >> > + * for a given @dev. It may return NULL if @dev doesn't require > >> > + * any actions to be performed by hotplug handler. > >> > */ > >> > struct MachineClass { > >> > /*< private >*/ > >> > @@ -80,6 +85,9 @@ struct MachineClass { > >> > /*< public >*/ > >> > > >> > QEMUMachine *qemu_machine; > >> > + > >> > + HotplugHandler *(*get_hotplug_handler)(MachineState *machine, > >> > + DeviceState *dev); > >> > }; > >> > > >> > /** > >> > -- > >> > 1.9.0 > >> > > >> > > >> > > > > >
diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 60f9df1..50bb8f5 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -34,6 +34,7 @@ #include "qapi/qmp/qjson.h" #include "monitor/monitor.h" #include "hw/hotplug.h" +#include "hw/boards.h" int qdev_hotplug = 0; static bool qdev_hot_added = false; @@ -761,6 +762,18 @@ static void device_set_realized(Object *obj, bool value, Error **err) local_err == NULL) { hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err); + } else if (local_err == NULL && + object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) { + HotplugHandler *hotplug_ctrl; + MachineState *machine = MACHINE(qdev_get_machine()); + MachineClass *mc = MACHINE_GET_CLASS(machine); + + if (mc->get_hotplug_handler) { + hotplug_ctrl = mc->get_hotplug_handler(machine, dev); + if (hotplug_ctrl) { + hotplug_handler_plug(hotplug_ctrl, dev, &local_err); + } + } } if (qdev_get_vmsd(dev) && local_err == NULL) { diff --git a/include/hw/boards.h b/include/hw/boards.h index 3567190..67750b5 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -73,6 +73,11 @@ extern MachineState *current_machine; /** * MachineClass: * @qemu_machine: #QEMUMachine + * @get_hotplug_handler: this function is called during bus-less + * device hotplug. If defined it returns pointer to an instance + * of HotplugHandler object, which handles hotplug operation + * for a given @dev. It may return NULL if @dev doesn't require + * any actions to be performed by hotplug handler. */ struct MachineClass { /*< private >*/ @@ -80,6 +85,9 @@ struct MachineClass { /*< public >*/ QEMUMachine *qemu_machine; + + HotplugHandler *(*get_hotplug_handler)(MachineState *machine, + DeviceState *dev); }; /**
Adds get_hotplug_handler() method to machine, and makes bus-less device to use it during hotplug as a means to discover hotplug handler controller. Returned controller is used to permorm a hotplug action. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- hw/core/qdev.c | 13 +++++++++++++ include/hw/boards.h | 8 ++++++++ 2 files changed, 21 insertions(+)