On Wed, 13 Jun 2018 12:58:46 +0200
David Hildenbrand <da...@redhat.com> wrote:
> On 08.06.2018 17:12, Michael S. Tsirkin wrote:
> > On Fri, Jun 08, 2018 at 03:07:53PM +0200, David Hildenbrand wrote:
> >> On 08.06.2018 14:55, Michael S. Tsirkin wrote:
> >>> On Fri, Jun 08, 2018 at 02:32:09PM +0200, David Hildenbrand wrote:
> >>>>
> >>>>>>> if (TYPE_PC_DIMM) {
> >>>>>>> pc_dimm_plug()
> >>>>>>> /* do here additional concrete machine specific things */
> >>>>>>> } else if (TYPE_VIRTIO_MEM) {
> >>>>>>> virtio_mem_plug() <- do forwarding in there
> >>>>>>> /* and do here additional concrete machine specific things */
> >>>>>>> } else if (TYPE_CPU) {
> >>>>>>> cpu_plug()
> >>>>>>> /* do here additional concrete machine specific things */
> >>>>>>> }
> >>>>>>
> >>>>>> That will result in a lot of duplicate code - for every machine we
> >>>>>> support (dimm/virtio-mem/virtio-pmem/*add more memory devices here*) -
> >>>>>> virtio-mem and virtio-pmem could most probably share the code.
> >>>>> maybe or maybe not, depending on if pmem endups as memory device or
> >>>>> separate controller. And even with duplication, machine code would
> >>>>> be easy to follow just down one explicit call chain.
> >>>>
> >>>> Not 100% convinced but I am now going into that direction.
> >>>
> >>> Can this go into DeviceClass? Failover has the same need to
> >>> allocate/free resources for vfio without a full realize/unrealize.
> >>>
> >>
> >> Conceptually, I would have called here something like
> >>
> >> virtio_mem_plug() ...
> >>
> >> Which would end up calling memory_device_plug() and triggering the
> >> target hotplug handler.
> >>
> >> I assume this can also be done from a device class callback.
> >>
> >> So we would need a total of 3 callbacks for
> >>
> >> a) pre_plug
> >> b) plug
> >> c) unplug
> >>
> >> In addition, unplug requests might be necessary, so
> >>
> >> d) unplug_request
> >
> >
> > Right - basically HotplugHandlerClass.
>
> Looking into this idea:
>
> What I would have right now (conceptually)
>
> if (TYPE_PC_DIMM) {
> pc_dimm_plug(machine);
> } else if (TYPE_CPU) {
> cpu_plug(machine);
> } else if (TYPE_VIRTIO_MEM) {
> virtio_mem_plug(machine);
> }
>
> Instead you want something like:
>
> if (TYPE_PC_DIMM) {
> pc_dimm_plug(machine);
> } else if (TYPE_CPU) {
> cpu_plug(machine);
> // igor requested an explicit list here, we could also check for
> // DEVICE_CLASS(device)->plug and make it generic
> } else if (TYPE_VIRTIO_MEM) {
> DEVICE_CLASS(device)->plug(machine);
> // call bus hotplug handler if necessary, or let the previous call
> // handle it?
not exactly this, I suggested following:
[ ... specific to machine_foo wiring ...]
virtio_mem_plug() {
[... some machine specific wiring ...]
bus_hotplug_ctrl = qdev_get_bus_hotplug_handler()
bus_hotplug_ctrl->plug(bus_hotplug_ctrl, dev)
[... some more machine specific wiring ...]
}
[ ... specific to machine_foo wiring ...]
i.e. device itself doesn't participate in attaching to external entities,
those entities (machine or bus controller virtio device is attached to)
do wiring on their own within their own domain.
> }
>
> We cannot pass the machine directly (due to board.h and user-only),
> instead we would have to pass it as hotplug handler. Then, the device
> class code would however make assumptions that always a machine is passed.
>
> Any opinions?
>
>
>
> >>>> --
> >>>>
> >>>> Thanks,
> >>>>
> >>>> David / dhildenb
> >>
> >>
> >> --
> >>
> >> Thanks,
> >>
> >> David / dhildenb
>
>
