Re: [qemu-s390x] [PATCH v4 04/14] pc: prepare for multi stage hotplug handlers

[Igor Mammedov]

On Wed, 13 Jun 2018 12:58:46 +0200
David Hildenbrand <address@hidden> 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  
> 
>