[Qemu-devel] [PATCH] qdev: Fix bus dependency of DeviceState::hotpluggable getter

[Qemu-devel] [PATCH] qdev: Fix bus dependency of DeviceState::hotpluggable getter

[Andreas Färber]

Commit 1a37eca107cece3ed454bae29eef0bd1fac4a244 (qdev: add
"hotpluggable" property to Device) added a property "hotpluggable" to
each device, with its getter accessing parent_bus->allow_hotplug.

Add a NULL check.

Cc: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/core/qdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 749c83a..cb07863 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -735,7 +735,8 @@ static bool device_get_hotpluggable(Object *obj, Error **err)
     DeviceClass *dc = DEVICE_GET_CLASS(obj);
     DeviceState *dev = DEVICE(obj);
 
-    return dc->hotpluggable && dev->parent_bus->allow_hotplug;
+    return dc->hotpluggable && (dev->parent_bus == NULL ||
+                                dev->parent_bus->allow_hotplug);
 }
 
 static void device_initfn(Object *obj)
-- 
1.8.4.5

Re: [Qemu-devel] [PATCH] qdev: Fix bus dependency of DeviceState::hotpluggable getter

[Michael S. Tsirkin]

On Fri, Mar 07, 2014 at 07:13:18PM +0100, Andreas Färber wrote:
> Commit 1a37eca107cece3ed454bae29eef0bd1fac4a244 (qdev: add
> "hotpluggable" property to Device) added a property "hotpluggable" to
> each device, with its getter accessing parent_bus->allow_hotplug.
> 
> Add a NULL check.
> 
> Cc: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>

Fair enough but I'm guessing we should
assume devices without a parent are not
hotpluggable, should we not?

Without a bus what handles hotplug?

> ---
>  hw/core/qdev.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 749c83a..cb07863 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -735,7 +735,8 @@ static bool device_get_hotpluggable(Object *obj, Error **err)
>      DeviceClass *dc = DEVICE_GET_CLASS(obj);
>      DeviceState *dev = DEVICE(obj);
>  
> -    return dc->hotpluggable && dev->parent_bus->allow_hotplug;
> +    return dc->hotpluggable && (dev->parent_bus == NULL ||
> +                                dev->parent_bus->allow_hotplug);

So maybe this should be:
>     return dc->hotpluggable && dev->parent_bus &&
>                                dev->parent_bus->allow_hotplug;

?

>  }
>  
>  static void device_initfn(Object *obj)
> -- 
> 1.8.4.5

Re: [Qemu-devel] [PATCH] qdev: Fix bus dependency of DeviceState::hotpluggable getter

[Andreas Färber]

Am 09.03.2014 17:40, schrieb Michael S. Tsirkin:
> On Fri, Mar 07, 2014 at 07:13:18PM +0100, Andreas Färber wrote:
>> Commit 1a37eca107cece3ed454bae29eef0bd1fac4a244 (qdev: add
>> "hotpluggable" property to Device) added a property "hotpluggable" to
>> each device, with its getter accessing parent_bus->allow_hotplug.
>>
>> Add a NULL check.
>>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
> 
> Fair enough but I'm guessing we should
> assume devices without a parent are not
> hotpluggable, should we not?
> 
> Without a bus what handles hotplug?

The device has to, in its realize function.

>> ---
>>  hw/core/qdev.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 749c83a..cb07863 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -735,7 +735,8 @@ static bool device_get_hotpluggable(Object *obj, Error **err)
>>      DeviceClass *dc = DEVICE_GET_CLASS(obj);
>>      DeviceState *dev = DEVICE(obj);
>>  
>> -    return dc->hotpluggable && dev->parent_bus->allow_hotplug;
>> +    return dc->hotpluggable && (dev->parent_bus == NULL ||
>> +                                dev->parent_bus->allow_hotplug);
> 
> So maybe this should be:
>>     return dc->hotpluggable && dev->parent_bus &&
>>                                dev->parent_bus->allow_hotplug;
> 
> ?

Hmm... that would render non-x86 CPUs non-hotpluggable. I guess we need
to check dc->bus_type for a NULL value and if non-NULL use your variant
and otherwise mine. I'll give it some more thought.

Thanks,
Andreas

> 
>>  }
>>  
>>  static void device_initfn(Object *obj)
>> -- 
>> 1.8.4.5

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] qdev: Fix bus dependency of DeviceState::hotpluggable getter

[Michael S. Tsirkin]

On Sun, Mar 09, 2014 at 05:48:57PM +0100, Andreas Färber wrote:
> Am 09.03.2014 17:40, schrieb Michael S. Tsirkin:
> > On Fri, Mar 07, 2014 at 07:13:18PM +0100, Andreas Färber wrote:
> >> Commit 1a37eca107cece3ed454bae29eef0bd1fac4a244 (qdev: add
> >> "hotpluggable" property to Device) added a property "hotpluggable" to
> >> each device, with its getter accessing parent_bus->allow_hotplug.
> >>
> >> Add a NULL check.
> >>
> >> Cc: Igor Mammedov <imammedo@redhat.com>
> >> Signed-off-by: Andreas Färber <afaerber@suse.de>
> > 
> > Fair enough but I'm guessing we should
> > assume devices without a parent are not
> > hotpluggable, should we not?
> > 
> > Without a bus what handles hotplug?
> 
> The device has to, in its realize function.
> >> ---
> >>  hw/core/qdev.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> >> index 749c83a..cb07863 100644
> >> --- a/hw/core/qdev.c
> >> +++ b/hw/core/qdev.c
> >> @@ -735,7 +735,8 @@ static bool device_get_hotpluggable(Object *obj, Error **err)
> >>      DeviceClass *dc = DEVICE_GET_CLASS(obj);
> >>      DeviceState *dev = DEVICE(obj);
> >>  
> >> -    return dc->hotpluggable && dev->parent_bus->allow_hotplug;
> >> +    return dc->hotpluggable && (dev->parent_bus == NULL ||
> >> +                                dev->parent_bus->allow_hotplug);
> > 
> > So maybe this should be:
> >>     return dc->hotpluggable && dev->parent_bus &&
> >>                                dev->parent_bus->allow_hotplug;
> > 
> > ?
> 
> Hmm... that would render non-x86 CPUs non-hotpluggable. I guess we need
> to check dc->bus_type for a NULL value and if non-NULL use your variant
> and otherwise mine.
> I'll give it some more thought.
> 
> Thanks,
> Andreas

Or clear dc->hotpluggable if there's no parent bus
and we don't want to allow hotplug.

I'm beginning to think your patch is the right one
after all...

> 
> > 
> >>  }
> >>  
> >>  static void device_initfn(Object *obj)
> >> -- 
> >> 1.8.4.5
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] qdev: Fix bus dependency of DeviceState::hotpluggable getter

[Igor Mammedov]

On Sun, 9 Mar 2014 18:55:05 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Sun, Mar 09, 2014 at 05:48:57PM +0100, Andreas Färber wrote:
> > Am 09.03.2014 17:40, schrieb Michael S. Tsirkin:
> > > On Fri, Mar 07, 2014 at 07:13:18PM +0100, Andreas Färber wrote:
> > >> Commit 1a37eca107cece3ed454bae29eef0bd1fac4a244 (qdev: add
> > >> "hotpluggable" property to Device) added a property "hotpluggable" to
> > >> each device, with its getter accessing parent_bus->allow_hotplug.
> > >>
> > >> Add a NULL check.
> > >>
> > >> Cc: Igor Mammedov <imammedo@redhat.com>
> > >> Signed-off-by: Andreas Färber <afaerber@suse.de>
> > > 
> > > Fair enough but I'm guessing we should
> > > assume devices without a parent are not
> > > hotpluggable, should we not?
> > > 
> > > Without a bus what handles hotplug?
> > 
> > The device has to, in its realize function.
> > >> ---
> > >>  hw/core/qdev.c | 3 ++-
> > >>  1 file changed, 2 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > >> index 749c83a..cb07863 100644
> > >> --- a/hw/core/qdev.c
> > >> +++ b/hw/core/qdev.c
> > >> @@ -735,7 +735,8 @@ static bool device_get_hotpluggable(Object *obj, Error **err)
> > >>      DeviceClass *dc = DEVICE_GET_CLASS(obj);
> > >>      DeviceState *dev = DEVICE(obj);
> > >>  
> > >> -    return dc->hotpluggable && dev->parent_bus->allow_hotplug;
> > >> +    return dc->hotpluggable && (dev->parent_bus == NULL ||
> > >> +                                dev->parent_bus->allow_hotplug);
> > > 
> > > So maybe this should be:
> > >>     return dc->hotpluggable && dev->parent_bus &&
> > >>                                dev->parent_bus->allow_hotplug;
> > > 
> > > ?
> > 
> > Hmm... that would render non-x86 CPUs non-hotpluggable. I guess we need
> > to check dc->bus_type for a NULL value and if non-NULL use your variant
> > and otherwise mine.
> > I'll give it some more thought.
> > 
> > Thanks,
> > Andreas
> 
> Or clear dc->hotpluggable if there's no parent bus
> and we don't want to allow hotplug.
> 
> I'm beginning to think your patch is the right one
> after all...
Before hotplug rework all devices were assumed to be hotpluggable,
the non-hotpluggable ones explicitly cleared dc->hotpluggable in
class_init(), rework hasn't changed it so Andreas's patch looks
better as to not block hotplug on bus-less devices.

Later if we implement bus-less hotplug with link<>s, we can
add link<> set hook to that will call hotplug handler in the same manner
as it's done for bus based devices.

> > 
> > > 
> > >>  }
> > >>  
> > >>  static void device_initfn(Object *obj)
> > >> -- 
> > >> 1.8.4.5
> > 
> > -- 
> > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg