[v4,04/14] pc: prepare for multi stage hotplug handlers

Message ID 20180517081527.14410-5-david@redhat.com
State New
Headers show
Series
  • MemoryDevice: use multi stage hotplug handlers
Related show

Commit Message

David Hildenbrand May 17, 2018, 8:15 a.m.
For multi stage hotplug handlers, we'll have to do some error handling
in some hotplug functions, so let's use a local error variable (except
for unplug requests).

Also, add code to pass control to the final stage hotplug handler at the
parent bus.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/i386/pc.c | 39 +++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

Comments

Igor Mammedov May 30, 2018, 1:08 p.m. | #1
On Thu, 17 May 2018 10:15:17 +0200
David Hildenbrand <david@redhat.com> wrote:

> For multi stage hotplug handlers, we'll have to do some error handling
> in some hotplug functions, so let's use a local error variable (except
> for unplug requests).
I'd split out introducing local error into separate patch
so patch would do a single thing.

> Also, add code to pass control to the final stage hotplug handler at the
> parent bus.
But I don't agree with generic
 "} else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {"
forwarding, it's done by 3/14 for generic case and in case of
special device that needs bus handler called from machine one,
I'd suggest to do forwarding explicitly for that device only
like we do with acpi_dev.


> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/i386/pc.c | 39 +++++++++++++++++++++++++++++++--------
>  1 file changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index d768930d02..510076e156 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2007,19 +2007,32 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>  static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>                                            DeviceState *dev, Error **errp)
>  {
> +    Error *local_err = NULL;
> +
> +    /* final stage hotplug handler */
>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> -        pc_cpu_pre_plug(hotplug_dev, dev, errp);
> +        pc_cpu_pre_plug(hotplug_dev, dev, &local_err);
> +    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> +        hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev,
> +                                 &local_err);
>      }
> +    error_propagate(errp, local_err);
>  }
>  
>  static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>                                        DeviceState *dev, Error **errp)
>  {
> +    Error *local_err = NULL;
> +
> +    /* final stage hotplug handler */
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        pc_dimm_plug(hotplug_dev, dev, errp);
> +        pc_dimm_plug(hotplug_dev, dev, &local_err);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> -        pc_cpu_plug(hotplug_dev, dev, errp);
> +        pc_cpu_plug(hotplug_dev, dev, &local_err);
> +    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> +        hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
>      }
> +    error_propagate(errp, local_err);
>  }
>  
>  static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> @@ -2029,7 +2042,10 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>          pc_dimm_unplug_request(hotplug_dev, dev, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>          pc_cpu_unplug_request_cb(hotplug_dev, dev, errp);
> -    } else {
> +    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> +        hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler, dev,
> +                                       errp);
> +    } else if (!dev->parent_bus) {
>          error_setg(errp, "acpi: device unplug request for not supported device"
>                     " type: %s", object_get_typename(OBJECT(dev)));
>      }
> @@ -2038,14 +2054,21 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>  static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
>                                          DeviceState *dev, Error **errp)
>  {
> +    Error *local_err = NULL;
> +
> +    /* final stage hotplug handler */
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        pc_dimm_unplug(hotplug_dev, dev, errp);
> +        pc_dimm_unplug(hotplug_dev, dev, &local_err);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> -        pc_cpu_unplug_cb(hotplug_dev, dev, errp);
> -    } else {
> -        error_setg(errp, "acpi: device unplug for not supported device"
> +        pc_cpu_unplug_cb(hotplug_dev, dev, &local_err);
> +    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> +        hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev,
> +                               &local_err);
> +    } else if (!dev->parent_bus) {
> +        error_setg(&local_err, "acpi: device unplug for not supported device"
>                     " type: %s", object_get_typename(OBJECT(dev)));
>      }
> +    error_propagate(errp, local_err);
>  }
>  
>  static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,
David Hildenbrand May 30, 2018, 2:13 p.m. | #2
On 30.05.2018 15:08, Igor Mammedov wrote:
> On Thu, 17 May 2018 10:15:17 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> For multi stage hotplug handlers, we'll have to do some error handling
>> in some hotplug functions, so let's use a local error variable (except
>> for unplug requests).
> I'd split out introducing local error into separate patch
> so patch would do a single thing.
> 
>> Also, add code to pass control to the final stage hotplug handler at the
>> parent bus.
> But I don't agree with generic
>  "} else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {"
> forwarding, it's done by 3/14 for generic case and in case of
> special device that needs bus handler called from machine one,
> I'd suggest to do forwarding explicitly for that device only
> like we do with acpi_dev.

I decided to do it that way because it is generic and results in nicer
recovery handling (e.g. in case pc_dimm plug fails, we can simply
rollback all (for now MemoryDevice) previous plug operations).

IMHO, the resulting code is easier to read.

From this handling it is clear that
"if we reach the hotplug handler, and it is not some special device
plugged by the machine (CPU, PC_DIMM), pass it on to the actual hotplug
handler if any exists"

> 
> 
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/i386/pc.c | 39 +++++++++++++++++++++++++++++++--------
>>  1 file changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index d768930d02..510076e156 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -2007,19 +2007,32 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>>  static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>>                                            DeviceState *dev, Error **errp)
>>  {
>> +    Error *local_err = NULL;
>> +
>> +    /* final stage hotplug handler */
>>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>> -        pc_cpu_pre_plug(hotplug_dev, dev, errp);
>> +        pc_cpu_pre_plug(hotplug_dev, dev, &local_err);
>> +    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
>> +        hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev,
>> +                                 &local_err);
>>      }
>> +    error_propagate(errp, local_err);
>>  }
>>  
>>  static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>>                                        DeviceState *dev, Error **errp)
>>  {
>> +    Error *local_err = NULL;
>> +
>> +    /* final stage hotplug handler */
>>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>> -        pc_dimm_plug(hotplug_dev, dev, errp);
>> +        pc_dimm_plug(hotplug_dev, dev, &local_err);
>>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>> -        pc_cpu_plug(hotplug_dev, dev, errp);
>> +        pc_cpu_plug(hotplug_dev, dev, &local_err);
>> +    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
>> +        hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
>>      }
>> +    error_propagate(errp, local_err);
>>  }
>>  
>>  static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>> @@ -2029,7 +2042,10 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>>          pc_dimm_unplug_request(hotplug_dev, dev, errp);
>>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>          pc_cpu_unplug_request_cb(hotplug_dev, dev, errp);
>> -    } else {
>> +    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
>> +        hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler, dev,
>> +                                       errp);
>> +    } else if (!dev->parent_bus) {
>>          error_setg(errp, "acpi: device unplug request for not supported device"
>>                     " type: %s", object_get_typename(OBJECT(dev)));
>>      }
>> @@ -2038,14 +2054,21 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>>  static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
>>                                          DeviceState *dev, Error **errp)
>>  {
>> +    Error *local_err = NULL;
>> +
>> +    /* final stage hotplug handler */
>>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>> -        pc_dimm_unplug(hotplug_dev, dev, errp);
>> +        pc_dimm_unplug(hotplug_dev, dev, &local_err);
>>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>> -        pc_cpu_unplug_cb(hotplug_dev, dev, errp);
>> -    } else {
>> -        error_setg(errp, "acpi: device unplug for not supported device"
>> +        pc_cpu_unplug_cb(hotplug_dev, dev, &local_err);
>> +    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
>> +        hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev,
>> +                               &local_err);
>> +    } else if (!dev->parent_bus) {
>> +        error_setg(&local_err, "acpi: device unplug for not supported device"
>>                     " type: %s", object_get_typename(OBJECT(dev)));
>>      }
>> +    error_propagate(errp, local_err);
>>  }
>>  
>>  static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,
>
Igor Mammedov May 31, 2018, 2:13 p.m. | #3
On Wed, 30 May 2018 16:13:32 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 30.05.2018 15:08, Igor Mammedov wrote:
> > On Thu, 17 May 2018 10:15:17 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> For multi stage hotplug handlers, we'll have to do some error handling
> >> in some hotplug functions, so let's use a local error variable (except
> >> for unplug requests).  
> > I'd split out introducing local error into separate patch
> > so patch would do a single thing.
> >   
> >> Also, add code to pass control to the final stage hotplug handler at the
> >> parent bus.  
> > But I don't agree with generic
> >  "} else if ("dev->parent_bus && dev->parent_bus->hotplug_handler) {"
> > forwarding, it's done by 3/14 for generic case and in case of
> > special device that needs bus handler called from machine one,
> > I'd suggest to do forwarding explicitly for that device only
> > like we do with acpi_dev.  
> 
> I decided to do it that way because it is generic and results in nicer
> recovery handling (e.g. in case pc_dimm plug fails, we can simply
> rollback all (for now MemoryDevice) previous plug operations).
rollback should be managed by the caller of pc_dimm plug
directly, so it's not relevant here.

> IMHO, the resulting code is easier to read.
> 
> From this handling it is clear that
> "if we reach the hotplug handler, and it is not some special device
> plugged by the machine (CPU, PC_DIMM), pass it on to the actual hotplug
> handler if any exists"
I strongly disagree with that it's easier to deal with.
You are basically duplicating already generalized code
from qdev_get_hotplug_handler() back into boards.

If a device doesn't have to be handled by machine handler,
than qdev_get_hotplug_handler() must return its bus handler
if any directly. So branch in question that your are copying
is a dead one, pls drop it.


> 
> > 
> >   
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  hw/i386/pc.c | 39 +++++++++++++++++++++++++++++++--------
> >>  1 file changed, 31 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >> index d768930d02..510076e156 100644
> >> --- a/hw/i386/pc.c
> >> +++ b/hw/i386/pc.c
> >> @@ -2007,19 +2007,32 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> >>  static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> >>                                            DeviceState *dev, Error **errp)
> >>  {
> >> +    Error *local_err = NULL;
> >> +
> >> +    /* final stage hotplug handler */
> >>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> >> -        pc_cpu_pre_plug(hotplug_dev, dev, errp);
> >> +        pc_cpu_pre_plug(hotplug_dev, dev, &local_err);
> >> +    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> >> +        hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev,
> >> +                                 &local_err);
> >>      }
> >> +    error_propagate(errp, local_err);
> >>  }
> >>  
> >>  static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
> >>                                        DeviceState *dev, Error **errp)
> >>  {
> >> +    Error *local_err = NULL;
> >> +
> >> +    /* final stage hotplug handler */
> >>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> >> -        pc_dimm_plug(hotplug_dev, dev, errp);
> >> +        pc_dimm_plug(hotplug_dev, dev, &local_err);
> >>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> >> -        pc_cpu_plug(hotplug_dev, dev, errp);
> >> +        pc_cpu_plug(hotplug_dev, dev, &local_err);
> >> +    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> >> +        hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
> >>      }
> >> +    error_propagate(errp, local_err);
> >>  }
> >>  
> >>  static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> >> @@ -2029,7 +2042,10 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> >>          pc_dimm_unplug_request(hotplug_dev, dev, errp);
> >>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> >>          pc_cpu_unplug_request_cb(hotplug_dev, dev, errp);
> >> -    } else {
> >> +    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> >> +        hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler, dev,
> >> +                                       errp);
> >> +    } else if (!dev->parent_bus) {
> >>          error_setg(errp, "acpi: device unplug request for not supported device"
> >>                     " type: %s", object_get_typename(OBJECT(dev)));
> >>      }
> >> @@ -2038,14 +2054,21 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> >>  static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
> >>                                          DeviceState *dev, Error **errp)
> >>  {
> >> +    Error *local_err = NULL;
> >> +
> >> +    /* final stage hotplug handler */
> >>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> >> -        pc_dimm_unplug(hotplug_dev, dev, errp);
> >> +        pc_dimm_unplug(hotplug_dev, dev, &local_err);
> >>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> >> -        pc_cpu_unplug_cb(hotplug_dev, dev, errp);
> >> -    } else {
> >> -        error_setg(errp, "acpi: device unplug for not supported device"
> >> +        pc_cpu_unplug_cb(hotplug_dev, dev, &local_err);
> >> +    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> >> +        hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev,
> >> +                               &local_err);
> >> +    } else if (!dev->parent_bus) {
> >> +        error_setg(&local_err, "acpi: device unplug for not supported device"
> >>                     " type: %s", object_get_typename(OBJECT(dev)));
> >>      }
> >> +    error_propagate(errp, local_err);
> >>  }
> >>  
> >>  static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,  
> >   
> 
>
David Hildenbrand June 4, 2018, 11:27 a.m. | #4
On 31.05.2018 16:13, Igor Mammedov wrote:
> On Wed, 30 May 2018 16:13:32 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 30.05.2018 15:08, Igor Mammedov wrote:
>>> On Thu, 17 May 2018 10:15:17 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> For multi stage hotplug handlers, we'll have to do some error handling
>>>> in some hotplug functions, so let's use a local error variable (except
>>>> for unplug requests).  
>>> I'd split out introducing local error into separate patch
>>> so patch would do a single thing.

I can do that if it makes review easier.

>>>   
>>>> Also, add code to pass control to the final stage hotplug handler at the
>>>> parent bus.  
>>> But I don't agree with generic
>>>  "} else if ("dev->parent_bus && dev->parent_bus->hotplug_handler) {"
>>> forwarding, it's done by 3/14 for generic case and in case of
>>> special device that needs bus handler called from machine one,
>>> I'd suggest to do forwarding explicitly for that device only
>>> like we do with acpi_dev.  
>>
>> I decided to do it that way because it is generic and results in nicer
>> recovery handling (e.g. in case pc_dimm plug fails, we can simply
>> rollback all (for now MemoryDevice) previous plug operations).
> rollback should be managed by the caller of pc_dimm plug
> directly, so it's not relevant here.
> 
>> IMHO, the resulting code is easier to read.
>>
>> From this handling it is clear that
>> "if we reach the hotplug handler, and it is not some special device
>> plugged by the machine (CPU, PC_DIMM), pass it on to the actual hotplug
>> handler if any exists"
> I strongly disagree with that it's easier to deal with.
> You are basically duplicating already generalized code
> from qdev_get_hotplug_handler() back into boards.
> 
> If a device doesn't have to be handled by machine handler,
> than qdev_get_hotplug_handler() must return its bus handler
> if any directly. So branch in question that your are copying
> is a dead one, pls drop it.

We forward selected (pc_get_hotpug_handler()) devices to the
right hotplug handler. Nothing wrong about that. I don't agree
with "basically duplicating already generalized code" wrong.
We have to forward at some place. Your idea simply places that
code at some other place.


But I think we have to get the general idea sorted out first.

What you have in mind (e.g. plug):

if (TYPE_MEMORY_DEVICE) {
	memory_device_plug();
	if (local_err) {
		goto out;
	}

	if (TYPE_PC_DIMM) {
		pc_dimm_plug(hotplug_dev, dev, &local_err);
	} else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
		hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
	}
	if (local_err) {
		memory_device_unplug()
		goto out;
	}
} else if (TYPE_CPU)
...


What I have in mind (and implemented in this series):


if (TYPE_MEMORY_DEVICE) {
	memory_device_plug();
}
/* possibly other interfaces */
if (local_err) {
	error_handling();
	return;
}

if (TYPE_PC_DIMM) {
	pc_dimm_plug(hotplug_dev, dev, &local_err);
} ...
} else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
	hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
}

if (local_err) {
	if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
		memory_device_unplug()
	}
	/* possibly other interfaces */
}
...


I claim that my variant is more generic because:
- it easily supports multiple interfaces (like MemoryDevice)
  per Device that need a hotplug handler call
- there is only one call to hotplug_handler_plug() in case we
  add similar handling for another device

Apart from that they do exactly the same thing.
Igor Mammedov June 7, 2018, 1:44 p.m. | #5
On Mon, 4 Jun 2018 13:27:01 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 31.05.2018 16:13, Igor Mammedov wrote:
> > On Wed, 30 May 2018 16:13:32 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> On 30.05.2018 15:08, Igor Mammedov wrote:  
> >>> On Thu, 17 May 2018 10:15:17 +0200
> >>> David Hildenbrand <david@redhat.com> wrote:
> >>>     
> >>>> For multi stage hotplug handlers, we'll have to do some error handling
> >>>> in some hotplug functions, so let's use a local error variable (except
> >>>> for unplug requests).    
> >>> I'd split out introducing local error into separate patch
> >>> so patch would do a single thing.  
> 
> I can do that if it makes review easier.
> 
> >>>     
> >>>> Also, add code to pass control to the final stage hotplug handler at the
> >>>> parent bus.    
> >>> But I don't agree with generic
> >>>  "} else if ("dev->parent_bus && dev->parent_bus->hotplug_handler) {"
> >>> forwarding, it's done by 3/14 for generic case and in case of
> >>> special device that needs bus handler called from machine one,
> >>> I'd suggest to do forwarding explicitly for that device only
> >>> like we do with acpi_dev.    
> >>
> >> I decided to do it that way because it is generic and results in nicer
> >> recovery handling (e.g. in case pc_dimm plug fails, we can simply
> >> rollback all (for now MemoryDevice) previous plug operations).  
> > rollback should be managed by the caller of pc_dimm plug
> > directly, so it's not relevant here.
> >   
> >> IMHO, the resulting code is easier to read.
> >>
> >> From this handling it is clear that
> >> "if we reach the hotplug handler, and it is not some special device
> >> plugged by the machine (CPU, PC_DIMM), pass it on to the actual hotplug
> >> handler if any exists"  
> > I strongly disagree with that it's easier to deal with.
> > You are basically duplicating already generalized code
> > from qdev_get_hotplug_handler() back into boards.
> > 
> > If a device doesn't have to be handled by machine handler,
> > than qdev_get_hotplug_handler() must return its bus handler
> > if any directly. So branch in question that your are copying
> > is a dead one, pls drop it.  
> 
> We forward selected (pc_get_hotpug_handler()) devices to the
> right hotplug handler. Nothing wrong about that. I don't agree
> with "basically duplicating already generalized code" wrong.
> We have to forward at some place. Your idea simply places that
> code at some other place.
> 
> 
> But I think we have to get the general idea sorted out first.
> 
> What you have in mind (e.g. plug):
> 
> if (TYPE_MEMORY_DEVICE) {
> 	memory_device_plug();
> 	if (local_err) {
> 		goto out;
> 	}
> 
> 	if (TYPE_PC_DIMM) {
> 		pc_dimm_plug(hotplug_dev, dev, &local_err);
> 	} else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> 		hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
> 	}
> 	if (local_err) {
> 		memory_device_unplug()
> 		goto out;
> 	}
> } else if (TYPE_CPU)
> ...
> 
> 
> What I have in mind (and implemented in this series):
> 
> 
> if (TYPE_MEMORY_DEVICE) {
> 	memory_device_plug();
> }
> /* possibly other interfaces */
> if (local_err) {
> 	error_handling();
> 	return;
> }
> 
> if (TYPE_PC_DIMM) {
> 	pc_dimm_plug(hotplug_dev, dev, &local_err);
> } ...
> } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> 	hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
> }
I don't like this implicit wiring, where reader of this part of code
has no idea that TYPE_MEMORY_DEVICE might be also bus device that needs
request forwarding.
I'd prefer [pre/un]plug handlers be concrete and explicit spaghetti code,
something like this:

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 */
}

> if (local_err) {
> 	if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
> 		memory_device_unplug()
> 	}
> 	/* possibly other interfaces */
> }
> ...
> 
> 
> I claim that my variant is more generic because:
> - it easily supports multiple interfaces (like MemoryDevice)
>   per Device that need a hotplug handler call
> - there is only one call to hotplug_handler_plug() in case we
>   add similar handling for another device
As someone said "one more layer of indirection would solve problem".
But then one would have a clue how it works after a while (including
author of the feature).
I don't think we have a problem here and need generalization.

> 
> Apart from that they do exactly the same thing.
>
David Hildenbrand June 7, 2018, 2 p.m. | #6
On 07.06.2018 15:44, Igor Mammedov wrote:
> On Mon, 4 Jun 2018 13:27:01 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 31.05.2018 16:13, Igor Mammedov wrote:
>>> On Wed, 30 May 2018 16:13:32 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> On 30.05.2018 15:08, Igor Mammedov wrote:  
>>>>> On Thu, 17 May 2018 10:15:17 +0200
>>>>> David Hildenbrand <david@redhat.com> wrote:
>>>>>     
>>>>>> For multi stage hotplug handlers, we'll have to do some error handling
>>>>>> in some hotplug functions, so let's use a local error variable (except
>>>>>> for unplug requests).    
>>>>> I'd split out introducing local error into separate patch
>>>>> so patch would do a single thing.  
>>
>> I can do that if it makes review easier.
>>
>>>>>     
>>>>>> Also, add code to pass control to the final stage hotplug handler at the
>>>>>> parent bus.    
>>>>> But I don't agree with generic
>>>>>  "} else if ("dev->parent_bus && dev->parent_bus->hotplug_handler) {"
>>>>> forwarding, it's done by 3/14 for generic case and in case of
>>>>> special device that needs bus handler called from machine one,
>>>>> I'd suggest to do forwarding explicitly for that device only
>>>>> like we do with acpi_dev.    
>>>>
>>>> I decided to do it that way because it is generic and results in nicer
>>>> recovery handling (e.g. in case pc_dimm plug fails, we can simply
>>>> rollback all (for now MemoryDevice) previous plug operations).  
>>> rollback should be managed by the caller of pc_dimm plug
>>> directly, so it's not relevant here.
>>>   
>>>> IMHO, the resulting code is easier to read.
>>>>
>>>> From this handling it is clear that
>>>> "if we reach the hotplug handler, and it is not some special device
>>>> plugged by the machine (CPU, PC_DIMM), pass it on to the actual hotplug
>>>> handler if any exists"  
>>> I strongly disagree with that it's easier to deal with.
>>> You are basically duplicating already generalized code
>>> from qdev_get_hotplug_handler() back into boards.
>>>
>>> If a device doesn't have to be handled by machine handler,
>>> than qdev_get_hotplug_handler() must return its bus handler
>>> if any directly. So branch in question that your are copying
>>> is a dead one, pls drop it.  
>>
>> We forward selected (pc_get_hotpug_handler()) devices to the
>> right hotplug handler. Nothing wrong about that. I don't agree
>> with "basically duplicating already generalized code" wrong.
>> We have to forward at some place. Your idea simply places that
>> code at some other place.
>>
>>
>> But I think we have to get the general idea sorted out first.
>>
>> What you have in mind (e.g. plug):
>>
>> if (TYPE_MEMORY_DEVICE) {
>> 	memory_device_plug();
>> 	if (local_err) {
>> 		goto out;
>> 	}
>>
>> 	if (TYPE_PC_DIMM) {
>> 		pc_dimm_plug(hotplug_dev, dev, &local_err);
>> 	} else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
>> 		hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
>> 	}
>> 	if (local_err) {
>> 		memory_device_unplug()
>> 		goto out;
>> 	}
>> } else if (TYPE_CPU)
>> ...
>>
>>
>> What I have in mind (and implemented in this series):
>>
>>
>> if (TYPE_MEMORY_DEVICE) {
>> 	memory_device_plug();
>> }
>> /* possibly other interfaces */
>> if (local_err) {
>> 	error_handling();
>> 	return;
>> }
>>
>> if (TYPE_PC_DIMM) {
>> 	pc_dimm_plug(hotplug_dev, dev, &local_err);
>> } ...
>> } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
>> 	hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
>> }
> I don't like this implicit wiring, where reader of this part of code
> has no idea that TYPE_MEMORY_DEVICE might be also bus device that needs
> request forwarding.
> I'd prefer [pre/un]plug handlers be concrete and explicit spaghetti code,
> something like this:
> 
> 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.

> 
>> if (local_err) {
>> 	if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
>> 		memory_device_unplug()
>> 	}
>> 	/* possibly other interfaces */
>> }
>> ...
>>
>>
>> I claim that my variant is more generic because:
>> - it easily supports multiple interfaces (like MemoryDevice)
>>   per Device that need a hotplug handler call
>> - there is only one call to hotplug_handler_plug() in case we
>>   add similar handling for another device
> As someone said "one more layer of indirection would solve problem".
> But then one would have a clue how it works after a while (including
> author of the feature).
> I don't think we have a problem here and need generalization.
> 
>>
>> Apart from that they do exactly the same thing.
>>
>
Igor Mammedov June 8, 2018, 12:24 p.m. | #7
On Thu, 7 Jun 2018 16:00:54 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 07.06.2018 15:44, Igor Mammedov wrote:
> > On Mon, 4 Jun 2018 13:27:01 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> On 31.05.2018 16:13, Igor Mammedov wrote:  
> >>> On Wed, 30 May 2018 16:13:32 +0200
> >>> David Hildenbrand <david@redhat.com> wrote:
> >>>     
> >>>> On 30.05.2018 15:08, Igor Mammedov wrote:    
> >>>>> On Thu, 17 May 2018 10:15:17 +0200
> >>>>> David Hildenbrand <david@redhat.com> wrote:
> >>>>>       
> >>>>>> For multi stage hotplug handlers, we'll have to do some error handling
> >>>>>> in some hotplug functions, so let's use a local error variable (except
> >>>>>> for unplug requests).      
> >>>>> I'd split out introducing local error into separate patch
> >>>>> so patch would do a single thing.    
> >>
> >> I can do that if it makes review easier.
> >>  
> >>>>>       
> >>>>>> Also, add code to pass control to the final stage hotplug handler at the
> >>>>>> parent bus.      
> >>>>> But I don't agree with generic
> >>>>>  "} else if ("dev->parent_bus && dev->parent_bus->hotplug_handler) {"
> >>>>> forwarding, it's done by 3/14 for generic case and in case of
> >>>>> special device that needs bus handler called from machine one,
> >>>>> I'd suggest to do forwarding explicitly for that device only
> >>>>> like we do with acpi_dev.      
> >>>>
> >>>> I decided to do it that way because it is generic and results in nicer
> >>>> recovery handling (e.g. in case pc_dimm plug fails, we can simply
> >>>> rollback all (for now MemoryDevice) previous plug operations).    
> >>> rollback should be managed by the caller of pc_dimm plug
> >>> directly, so it's not relevant here.
> >>>     
> >>>> IMHO, the resulting code is easier to read.
> >>>>
> >>>> From this handling it is clear that
> >>>> "if we reach the hotplug handler, and it is not some special device
> >>>> plugged by the machine (CPU, PC_DIMM), pass it on to the actual hotplug
> >>>> handler if any exists"    
> >>> I strongly disagree with that it's easier to deal with.
> >>> You are basically duplicating already generalized code
> >>> from qdev_get_hotplug_handler() back into boards.
> >>>
> >>> If a device doesn't have to be handled by machine handler,
> >>> than qdev_get_hotplug_handler() must return its bus handler
> >>> if any directly. So branch in question that your are copying
> >>> is a dead one, pls drop it.    
> >>
> >> We forward selected (pc_get_hotpug_handler()) devices to the
> >> right hotplug handler. Nothing wrong about that. I don't agree
> >> with "basically duplicating already generalized code" wrong.
> >> We have to forward at some place. Your idea simply places that
> >> code at some other place.
> >>
> >>
> >> But I think we have to get the general idea sorted out first.
> >>
> >> What you have in mind (e.g. plug):
> >>
> >> if (TYPE_MEMORY_DEVICE) {
> >> 	memory_device_plug();
> >> 	if (local_err) {
> >> 		goto out;
> >> 	}
> >>
> >> 	if (TYPE_PC_DIMM) {
> >> 		pc_dimm_plug(hotplug_dev, dev, &local_err);
> >> 	} else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> >> 		hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
> >> 	}
> >> 	if (local_err) {
> >> 		memory_device_unplug()
> >> 		goto out;
> >> 	}
> >> } else if (TYPE_CPU)
> >> ...
> >>
> >>
> >> What I have in mind (and implemented in this series):
> >>
> >>
> >> if (TYPE_MEMORY_DEVICE) {
> >> 	memory_device_plug();
> >> }
> >> /* possibly other interfaces */
> >> if (local_err) {
> >> 	error_handling();
> >> 	return;
> >> }
> >>
> >> if (TYPE_PC_DIMM) {
> >> 	pc_dimm_plug(hotplug_dev, dev, &local_err);
> >> } ...
> >> } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> >> 	hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
> >> }  
> > I don't like this implicit wiring, where reader of this part of code
> > has no idea that TYPE_MEMORY_DEVICE might be also bus device that needs
> > request forwarding.
> > I'd prefer [pre/un]plug handlers be concrete and explicit spaghetti code,
> > something like this:
> > 
> > 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.

> 
> >   
> >> if (local_err) {
> >> 	if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
> >> 		memory_device_unplug()
> >> 	}
> >> 	/* possibly other interfaces */
> >> }
> >> ...
> >>
> >>
> >> I claim that my variant is more generic because:
> >> - it easily supports multiple interfaces (like MemoryDevice)
> >>   per Device that need a hotplug handler call
> >> - there is only one call to hotplug_handler_plug() in case we
> >>   add similar handling for another device  
> > As someone said "one more layer of indirection would solve problem".
> > But then one would have a clue how it works after a while (including
> > author of the feature).
> > I don't think we have a problem here and need generalization.
> >   
> >>
> >> Apart from that they do exactly the same thing.
> >>  
> >   
> 
>
David Hildenbrand June 8, 2018, 12:32 p.m. | #8
>>> 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.
Michael S. Tsirkin June 8, 2018, 12:55 p.m. | #9
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.

> -- 
> 
> Thanks,
> 
> David / dhildenb
David Hildenbrand June 8, 2018, 1:07 p.m. | #10
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

>> -- 
>>
>> Thanks,
>>
>> David / dhildenb
Michael S. Tsirkin June 8, 2018, 3:12 p.m. | #11
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.

> >> -- 
> >>
> >> Thanks,
> >>
> >> David / dhildenb
> 
> 
> -- 
> 
> Thanks,
> 
> David / dhildenb
David Hildenbrand June 13, 2018, 10:58 a.m. | #12
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
Igor Mammedov June 13, 2018, 3:48 p.m. | #13
On Wed, 13 Jun 2018 12:58:46 +0200
David Hildenbrand <david@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  
> 
>
David Hildenbrand June 13, 2018, 3:51 p.m. | #14
On 13.06.2018 17:48, Igor Mammedov wrote:
> On Wed, 13 Jun 2018 12:58:46 +0200
> David Hildenbrand <david@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.

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?
Michael S. Tsirkin June 13, 2018, 6:32 p.m. | #15
On Wed, Jun 13, 2018 at 05:51:01PM +0200, David Hildenbrand wrote:
> On 13.06.2018 17:48, Igor Mammedov wrote:
> > On Wed, 13 Jun 2018 12:58:46 +0200
> > David Hildenbrand <david@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.
> 
> 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?

It's not virtio specifically, I'm interested in how this will work for
PCI generally.  Right now we call do_pci_register_device which
immediately makes it guest visible.


> 
> -- 
> 
> Thanks,
> 
> David / dhildenb
David Hildenbrand June 13, 2018, 7:37 p.m. | #16
>>>       [ ... 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?
> 
> It's not virtio specifically, I'm interested in how this will work for
> PCI generally.  Right now we call do_pci_register_device which
> immediately makes it guest visible.

So you're telling me that a PCI device exposes itself to the system in
pci_qdev_realize() instead of letting a hotplug handler handle that? My
assumption is that the PCI bridge hotplug handler handles this. What am
I missing?

I can see that e.g. for a virtio device the realize call chain is

pci_qdev_realize() -> virtio_pci_realize() -> virtio_XXX__pci_realize ->
virtio_XXX_realize()

If any realization in pci_qdev_realize() fails, we do a
do_pci_unregister_device().

So if it is true what you're saying than we're already exposing
partially realized (and possibly unrealized again) devices via PCI. I
*guess* because we're holding the iothread mutex this is okay and
actually not visible. And we only seem to be sending events in the PCI
bridge hotplug handlers, so my assumption is that this is fine.

> 
> 
>>
>> -- 
>>
>> Thanks,
>>
>> David / dhildenb
Michael S. Tsirkin June 13, 2018, 10:05 p.m. | #17
On Wed, Jun 13, 2018 at 09:37:54PM +0200, David Hildenbrand wrote:
> >>>       [ ... 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?
> > 
> > It's not virtio specifically, I'm interested in how this will work for
> > PCI generally.  Right now we call do_pci_register_device which
> > immediately makes it guest visible.
> 
> So you're telling me that a PCI device exposes itself to the system in
> pci_qdev_realize() instead of letting a hotplug handler handle that? My
> assumption is that the PCI bridge hotplug handler handles this.

Well given how things work in qemu that's not exactly
the case. See below.

> What am
> I missing?
> 
> I can see that e.g. for a virtio device the realize call chain is
> 
> pci_qdev_realize() -> virtio_pci_realize() -> virtio_XXX__pci_realize ->
> virtio_XXX_realize()
> 
> If any realization in pci_qdev_realize() fails, we do a
> do_pci_unregister_device().
> 
> So if it is true what you're saying than we're already exposing
> partially realized (and possibly unrealized again) devices via PCI. I
> *guess* because we're holding the iothread mutex this is okay and
> actually not visible.

For now but we need ability to have separate new commands for
realize and plug, so we will drop the mutex.

> And we only seem to be sending events in the PCI
> bridge hotplug handlers, so my assumption is that this is fine.

For core PCI, it's mostly just this line:

    bus->devices[devfn] = pci_dev;

which makes it accessible to pci config cycles.

But failover also cares about vfio, which seems to set up
e.g. irqfs on realize.




> > 
> > 
> >>
> >> -- 
> >>
> >> Thanks,
> >>
> >> David / dhildenb
> 
> 
> -- 
> 
> Thanks,
> 
> David / dhildenb
David Hildenbrand June 14, 2018, 6:14 a.m. | #18
On 14.06.2018 00:05, Michael S. Tsirkin wrote:
> On Wed, Jun 13, 2018 at 09:37:54PM +0200, David Hildenbrand wrote:
>>>>>       [ ... 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?
>>>
>>> It's not virtio specifically, I'm interested in how this will work for
>>> PCI generally.  Right now we call do_pci_register_device which
>>> immediately makes it guest visible.
>>
>> So you're telling me that a PCI device exposes itself to the system in
>> pci_qdev_realize() instead of letting a hotplug handler handle that? My
>> assumption is that the PCI bridge hotplug handler handles this.
> 
> Well given how things work in qemu that's not exactly
> the case. See below.
> 
>> What am
>> I missing?
>>
>> I can see that e.g. for a virtio device the realize call chain is
>>
>> pci_qdev_realize() -> virtio_pci_realize() -> virtio_XXX__pci_realize ->
>> virtio_XXX_realize()
>>
>> If any realization in pci_qdev_realize() fails, we do a
>> do_pci_unregister_device().
>>
>> So if it is true what you're saying than we're already exposing
>> partially realized (and possibly unrealized again) devices via PCI. I
>> *guess* because we're holding the iothread mutex this is okay and
>> actually not visible.
> 
> For now but we need ability to have separate new commands for
> realize and plug, so we will drop the mutex.

If you want to actually drop the mutex, I assume that quite some rework
will be necessary (not only for this specific PCI hotplug handler case),
most probably also in other code parts (to) - all of the hotplug/realize
code seems to rely on the mutex being locked and not being dropped
temporarily.

> 
>> And we only seem to be sending events in the PCI
>> bridge hotplug handlers, so my assumption is that this is fine.
> 
> For core PCI, it's mostly just this line:
> 
>     bus->devices[devfn] = pci_dev;

This should go into the hotplug handler if I am not wrong. From what I
learned from Igor, this is a layer violation. Resource assignment should
happen during pre_plug / plug. But then you might need a different way
to "reserve" a function (if there could be races then with the lock
temporarily dropped).

> 
> which makes it accessible to pci config cycles.
> 
> But failover also cares about vfio, which seems to set up
> e.g. irqfs on realize.
Igor Mammedov June 14, 2018, 9:16 a.m. | #19
On Thu, 14 Jun 2018 08:14:05 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 14.06.2018 00:05, Michael S. Tsirkin wrote:
> > On Wed, Jun 13, 2018 at 09:37:54PM +0200, David Hildenbrand wrote:  
> >>>>>       [ ... 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?  
> >>>
> >>> It's not virtio specifically, I'm interested in how this will work for
> >>> PCI generally.  Right now we call do_pci_register_device which
> >>> immediately makes it guest visible.  
> >>
> >> So you're telling me that a PCI device exposes itself to the system in
> >> pci_qdev_realize() instead of letting a hotplug handler handle that? My
> >> assumption is that the PCI bridge hotplug handler handles this.  
> > 
> > Well given how things work in qemu that's not exactly
> > the case. See below.
> >   
> >> What am
> >> I missing?
> >>
> >> I can see that e.g. for a virtio device the realize call chain is
> >>
> >> pci_qdev_realize() -> virtio_pci_realize() -> virtio_XXX__pci_realize ->
> >> virtio_XXX_realize()
> >>
> >> If any realization in pci_qdev_realize() fails, we do a
> >> do_pci_unregister_device().
> >>
> >> So if it is true what you're saying than we're already exposing
> >> partially realized (and possibly unrealized again) devices via PCI. I
> >> *guess* because we're holding the iothread mutex this is okay and
> >> actually not visible.  
> > 
> > For now but we need ability to have separate new commands for
> > realize and plug, so we will drop the mutex.  
> 
> If you want to actually drop the mutex, I assume that quite some rework
> will be necessary (not only for this specific PCI hotplug handler case),
> most probably also in other code parts (to) - all of the hotplug/realize
> code seems to rely on the mutex being locked and not being dropped
> temporarily.
yep, all monitor actions and reactions from guest via mmio are now
protected by global lock that guaranties not parallel action could
executed at the same time. So it's save for now and dropping global
lock would require some refactoring (probably a lot).

> >   
> >> And we only seem to be sending events in the PCI
> >> bridge hotplug handlers, so my assumption is that this is fine.  
> > 
> > For core PCI, it's mostly just this line:
> > 
> >     bus->devices[devfn] = pci_dev;  
> 
> This should go into the hotplug handler if I am not wrong. From what I
> learned from Igor, this is a layer violation. Resource assignment should
> happen during pre_plug / plug. But then you might need a different way
> to "reserve" a function (if there could be races then with the lock
> temporarily dropped).
I agree, but it's a separate refactoring and it isn't pre-requisite for
virtio-mem work, so it shouldn't hold it.

> > which makes it accessible to pci config cycles.
> > 
> > But failover also cares about vfio, which seems to set up
> > e.g. irqfs on realize.  
Do we have a thread for failover somewhere on the list that discusses
ideas and requirements for it, where we can discuss it?
Otherwise this discussion will get buried in this unrelated thread.
Igor Mammedov June 14, 2018, 9:20 a.m. | #20
On Wed, 13 Jun 2018 17:51:01 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 13.06.2018 17:48, Igor Mammedov wrote:
> > On Wed, 13 Jun 2018 12:58:46 +0200
> > David Hildenbrand <david@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.  
> 
> 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.

Patch

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index d768930d02..510076e156 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2007,19 +2007,32 @@  static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
 static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
                                           DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_pre_plug(hotplug_dev, dev, errp);
+        pc_cpu_pre_plug(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev,
+                                 &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        pc_dimm_plug(hotplug_dev, dev, errp);
+        pc_dimm_plug(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_plug(hotplug_dev, dev, errp);
+        pc_cpu_plug(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
@@ -2029,7 +2042,10 @@  static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
         pc_dimm_unplug_request(hotplug_dev, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         pc_cpu_unplug_request_cb(hotplug_dev, dev, errp);
-    } else {
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler, dev,
+                                       errp);
+    } else if (!dev->parent_bus) {
         error_setg(errp, "acpi: device unplug request for not supported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
     }
@@ -2038,14 +2054,21 @@  static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
 static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
                                         DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        pc_dimm_unplug(hotplug_dev, dev, errp);
+        pc_dimm_unplug(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_unplug_cb(hotplug_dev, dev, errp);
-    } else {
-        error_setg(errp, "acpi: device unplug for not supported device"
+        pc_cpu_unplug_cb(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev,
+                               &local_err);
+    } else if (!dev->parent_bus) {
+        error_setg(&local_err, "acpi: device unplug for not supported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
     }
+    error_propagate(errp, local_err);
 }
 
 static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,