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

[Igor Mammedov]

On Wed, 13 Jun 2018 17:51:01 +0200
David Hildenbrand <address@hidden> wrote:

> On 13.06.2018 17:48, Igor Mammedov wrote:
> > 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.  
> 
> I am fine with this, but Michael asked if this ("virtio_mem_plug()")
> could go via new DeviceClass functions. Michael, would that also work
> for your use case?
We can discuss DeviceClass functions when patches for failover surface
and if it's really need.