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

[David Hildenbrand]

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?


-- 

Thanks,

David / dhildenb