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

[David Hildenbrand]

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?
}

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


-- 

Thanks,

David / dhildenb