Re: [Qemu-devel] [PATCH v4 4/7] qdev: add hotpluggable to DeviceState

[Gerd Hoffmann]

  Hi,

> > The connection between QemuConsole and User Interface (i.e. gtk, spice,
> > ...) is a bit more flexible.  But also not really designed for hotplug
> > as QemuConsole is not hotpluggable in the first place ...
> > 
> > We could drop the display property and use two devices instead.
> > 
> >   new vfio-pci would behave like display=off with this series.
> >   added vfio-pci-display has display=on behavior.
> >   display=auto is not possible.
> 
> I expect libvirt and above would balk at creating a separate QEMU
> device for this purpose, easy for QEMU, hard for anything that manages
> QEMU.

Now as you've mentioned libvirt I remember we had the same discussion
before, with usb host adapters.  The uhci and ehci controllers have a
simliar issue:  If they are configured as companion setup (ehci for usb2
and uhci for usb1) they can't be hotplugged, as standalone controller
hotplugging works fine.

We ended up with splitting the controllers into two groups:  The ones
which can be used in a companion setup (basically all ich9-* devices)
which are not hotpluggable.   And the other ones which don't support
companion setups but can be hotplugged.  Commits:

   ec56214f6f usb: tag standalone ehci as hotpluggable
   638ca939d8 usb: tag standalone uhci as hotpluggable

The argument from the libvirt side was that it is actually easier for
them to handle if hotplugging is a fixed property of a device and
doesn't change magically depending on device configuration.  First
because they can query qemu then whenever a given device can be
hotplugged or not, and second because it'll work for both plug-in and
plug-out.

So this patch isn't going to fly, and unless someone can come up with a
better idea I'll go use the two-devices approach.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v4 4/7] qdev: add hotpluggable to DeviceState

[Alex Williamson]

On Fri, 23 Feb 2018 09:53:22 +0100
Gerd Hoffmann  wrote:

>   Hi,
> 
> > > What type of device is only sometimes hotpluggable ?
> > > The commit message says "display devices" and "consoles",
> > > but I would expect those to both be types of device which
> > > have a class which is never hotpluggable, so you can mark
> > > them non-hotpluggable with the existing class flag rather
> > > than needing a per-instance flag.  
> > 
> > With this series, a vfio-pci device optionally supports a display.  The
> > vfio-pci device is hotpluggable, but QEMU display support is not.  So
> > the solution here is to make the vfio-pci device non-hotpluggable only
> > when it supports and enables a display.
> > 
> > Gerd, is there another solution that the display object is instantiated
> > separately from the vfio-pci object and the display support in the
> > vfio-pci device references the display object via an id.  
> 
> Well, not really.  At least not without putting much of qemu display
> support upside down.  The qemu display (aka QemuConsole) is created and
> managed by the display devices, they can't be created independant from
> a device ...
> 
> The connection between QemuConsole and User Interface (i.e. gtk, spice,
> ...) is a bit more flexible.  But also not really designed for hotplug
> as QemuConsole is not hotpluggable in the first place ...
> 
> We could drop the display property and use two devices instead.
> 
>   new vfio-pci would behave like display=off with this series.
>   added vfio-pci-display has display=on behavior.
>   display=auto is not possible.

I expect libvirt and above would balk at creating a separate QEMU
device for this purpose, easy for QEMU, hard for anything that manages
QEMU.  Thanks,

Alex

Re: [Qemu-devel] [PATCH v4 4/7] qdev: add hotpluggable to DeviceState

[Gerd Hoffmann]

  Hi,

> > What type of device is only sometimes hotpluggable ?
> > The commit message says "display devices" and "consoles",
> > but I would expect those to both be types of device which
> > have a class which is never hotpluggable, so you can mark
> > them non-hotpluggable with the existing class flag rather
> > than needing a per-instance flag.
> 
> With this series, a vfio-pci device optionally supports a display.  The
> vfio-pci device is hotpluggable, but QEMU display support is not.  So
> the solution here is to make the vfio-pci device non-hotpluggable only
> when it supports and enables a display.
> 
> Gerd, is there another solution that the display object is instantiated
> separately from the vfio-pci object and the display support in the
> vfio-pci device references the display object via an id.

Well, not really.  At least not without putting much of qemu display
support upside down.  The qemu display (aka QemuConsole) is created and
managed by the display devices, they can't be created independant from
a device ...

The connection between QemuConsole and User Interface (i.e. gtk, spice,
...) is a bit more flexible.  But also not really designed for hotplug
as QemuConsole is not hotpluggable in the first place ...

We could drop the display property and use two devices instead.

  new vfio-pci would behave like display=off with this series.
  added vfio-pci-display has display=on behavior.
  display=auto is not possible.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v4 4/7] qdev: add hotpluggable to DeviceState

[Zhenyu Wang]

On 2018.02.22 09:59:17 -0700, Alex Williamson wrote:
> On Thu, 22 Feb 2018 15:46:17 +
> Peter Maydell  wrote:
> 
> > On 22 February 2018 at 15:37, Gerd Hoffmann  wrote:
> > > On Mon, Feb 19, 2018 at 12:14:55PM +0100, Gerd Hoffmann wrote:  
> > >> vfio display support wants disable hotplug for certain devices, because
> > >> qemu doesn't support hotplugging display devices and qemu consoles.
> > >>
> > >> Add a hotpluggable bool to DeviceState, initialize it from
> > >> DeviceClass->hotpluggable, update device_get_hotpluggable accordingly.
> > >>
> > >> Devices can flip the new variable from true to false if needed.  
> > >
> > > Alex wants an ack for this one.  Who maintains it these days?
> > >
> > > MAINTAINERS doesn't list qdev.  For QOM which is closest probably
> > > Andreas Färber is listed.  Havn't seen him on the list for a while
> > > though.
> > >
> > > Ok, lets try some usual suspects...
> > > Markus? Eric? Paolo? Peter?  Any comments on this one?  
> > 
> > What type of device is only sometimes hotpluggable ?
> > The commit message says "display devices" and "consoles",
> > but I would expect those to both be types of device which
> > have a class which is never hotpluggable, so you can mark
> > them non-hotpluggable with the existing class flag rather
> > than needing a per-instance flag.
> 
> With this series, a vfio-pci device optionally supports a display.  The
> vfio-pci device is hotpluggable, but QEMU display support is not.  So
> the solution here is to make the vfio-pci device non-hotpluggable only
> when it supports and enables a display.
>

I'm not sure, how this is supposed to work?

And there's also case that vgpu's display is not required but still need
device hotplug feature e.g to use render/media acceleration, as supported
by current vfio mdev device on gvt-g.

> Gerd, is there another solution that the display object is instantiated
> separately from the vfio-pci object and the display support in the
> vfio-pci device references the display object via an id.  Possibly
> vfio-pci could remain hotpluggable while the display class device is
> not.  Potentially one display could be switched between multiple
> display capable devices, like an input control button on a monitor,
> losing signal if none are connected.  Possible?  Clearly I have no idea
> how display objects actually work in QEMU.  Thanks,
> 
> Alex
> ___
> intel-gvt-dev mailing list
> intel-gvt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v4 4/7] qdev: add hotpluggable to DeviceState

[Peter Maydell]

On 22 February 2018 at 16:59, Alex Williamson
 wrote:
> On Thu, 22 Feb 2018 15:46:17 +
> Peter Maydell  wrote:
>> What type of device is only sometimes hotpluggable ?
>> The commit message says "display devices" and "consoles",
>> but I would expect those to both be types of device which
>> have a class which is never hotpluggable, so you can mark
>> them non-hotpluggable with the existing class flag rather
>> than needing a per-instance flag.
>
> With this series, a vfio-pci device optionally supports a display.  The
> vfio-pci device is hotpluggable, but QEMU display support is not.  So
> the solution here is to make the vfio-pci device non-hotpluggable only
> when it supports and enables a display.

Ah, I see. I don't have a good suggestion offhand, but I suspect
the patch as it stands is insufficient if we want to take the
approach it uses. For instance qdev_unplug() in qdev-monitor.c
looks directly at dc->hotpluggable to decide whether it should
refuse a user request to hot-unplug a device.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 4/7] qdev: add hotpluggable to DeviceState

[Alex Williamson]

On Thu, 22 Feb 2018 15:46:17 +
Peter Maydell  wrote:

> On 22 February 2018 at 15:37, Gerd Hoffmann  wrote:
> > On Mon, Feb 19, 2018 at 12:14:55PM +0100, Gerd Hoffmann wrote:  
> >> vfio display support wants disable hotplug for certain devices, because
> >> qemu doesn't support hotplugging display devices and qemu consoles.
> >>
> >> Add a hotpluggable bool to DeviceState, initialize it from
> >> DeviceClass->hotpluggable, update device_get_hotpluggable accordingly.
> >>
> >> Devices can flip the new variable from true to false if needed.  
> >
> > Alex wants an ack for this one.  Who maintains it these days?
> >
> > MAINTAINERS doesn't list qdev.  For QOM which is closest probably
> > Andreas Färber is listed.  Havn't seen him on the list for a while
> > though.
> >
> > Ok, lets try some usual suspects...
> > Markus? Eric? Paolo? Peter?  Any comments on this one?  
> 
> What type of device is only sometimes hotpluggable ?
> The commit message says "display devices" and "consoles",
> but I would expect those to both be types of device which
> have a class which is never hotpluggable, so you can mark
> them non-hotpluggable with the existing class flag rather
> than needing a per-instance flag.

With this series, a vfio-pci device optionally supports a display.  The
vfio-pci device is hotpluggable, but QEMU display support is not.  So
the solution here is to make the vfio-pci device non-hotpluggable only
when it supports and enables a display.

Gerd, is there another solution that the display object is instantiated
separately from the vfio-pci object and the display support in the
vfio-pci device references the display object via an id.  Possibly
vfio-pci could remain hotpluggable while the display class device is
not.  Potentially one display could be switched between multiple
display capable devices, like an input control button on a monitor,
losing signal if none are connected.  Possible?  Clearly I have no idea
how display objects actually work in QEMU.  Thanks,

Alex

Re: [Qemu-devel] [PATCH v4 4/7] qdev: add hotpluggable to DeviceState

[Peter Maydell]

On 22 February 2018 at 15:37, Gerd Hoffmann  wrote:
> On Mon, Feb 19, 2018 at 12:14:55PM +0100, Gerd Hoffmann wrote:
>> vfio display support wants disable hotplug for certain devices, because
>> qemu doesn't support hotplugging display devices and qemu consoles.
>>
>> Add a hotpluggable bool to DeviceState, initialize it from
>> DeviceClass->hotpluggable, update device_get_hotpluggable accordingly.
>>
>> Devices can flip the new variable from true to false if needed.
>
> Alex wants an ack for this one.  Who maintains it these days?
>
> MAINTAINERS doesn't list qdev.  For QOM which is closest probably
> Andreas Färber is listed.  Havn't seen him on the list for a while
> though.
>
> Ok, lets try some usual suspects...
> Markus? Eric? Paolo? Peter?  Any comments on this one?

What type of device is only sometimes hotpluggable ?
The commit message says "display devices" and "consoles",
but I would expect those to both be types of device which
have a class which is never hotpluggable, so you can mark
them non-hotpluggable with the existing class flag rather
than needing a per-instance flag.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 4/7] qdev: add hotpluggable to DeviceState

[Gerd Hoffmann]

On Mon, Feb 19, 2018 at 12:14:55PM +0100, Gerd Hoffmann wrote:
> vfio display support wants disable hotplug for certain devices, because
> qemu doesn't support hotplugging display devices and qemu consoles.
> 
> Add a hotpluggable bool to DeviceState, initialize it from
> DeviceClass->hotpluggable, update device_get_hotpluggable accordingly.
> 
> Devices can flip the new variable from true to false if needed.

Alex wants an ack for this one.  Who maintains it these days?

MAINTAINERS doesn't list qdev.  For QOM which is closest probably
Andreas Färber is listed.  Havn't seen him on the list for a while
though.

Ok, lets try some usual suspects...
Markus? Eric? Paolo? Peter?  Any comments on this one?

thanks,
  Gerd

> Signed-off-by: Gerd Hoffmann 
> ---
>  include/hw/qdev-core.h | 1 +
>  hw/core/qdev.c | 5 +++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index fc9d617a76..37d86f26e5 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -154,6 +154,7 @@ struct DeviceState {
>  const char *id;
>  char *canonical_path;
>  bool realized;
> +bool hotpluggable;
>  bool pending_deleted_event;
>  QemuOpts *opts;
>  int hotplugged;
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 7ed1f431f0..89e03c7b47 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -946,8 +946,8 @@ static bool device_get_hotpluggable(Object *obj, Error 
> **errp)
>  DeviceClass *dc = DEVICE_GET_CLASS(obj);
>  DeviceState *dev = DEVICE(obj);
>  
> -return dc->hotpluggable && (dev->parent_bus == NULL ||
> -qbus_is_hotpluggable(dev->parent_bus));
> +return dc->hotpluggable && dev->hotpluggable &&
> +(dev->parent_bus == NULL || qbus_is_hotpluggable(dev->parent_bus));
>  }
>  
>  static bool device_get_hotplugged(Object *obj, Error **err)
> @@ -988,6 +988,7 @@ static void device_initfn(Object *obj)
>  class = object_class_get_parent(class);
>  } while (class != object_class_by_name(TYPE_DEVICE));
>  
> +dev->hotpluggable = DEVICE_CLASS(class)->hotpluggable;
>  object_property_add_link(OBJECT(dev), "parent_bus", TYPE_BUS,
>   (Object **)>parent_bus, NULL, 0,
>   _abort);
> -- 
> 2.9.3
> 

[Qemu-devel] [PATCH v4 4/7] qdev: add hotpluggable to DeviceState

[Gerd Hoffmann]

vfio display support wants disable hotplug for certain devices, because
qemu doesn't support hotplugging display devices and qemu consoles.

Add a hotpluggable bool to DeviceState, initialize it from
DeviceClass->hotpluggable, update device_get_hotpluggable accordingly.

Devices can flip the new variable from true to false if needed.

Signed-off-by: Gerd Hoffmann 
---
 include/hw/qdev-core.h | 1 +
 hw/core/qdev.c | 5 +++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index fc9d617a76..37d86f26e5 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -154,6 +154,7 @@ struct DeviceState {
 const char *id;
 char *canonical_path;
 bool realized;
+bool hotpluggable;
 bool pending_deleted_event;
 QemuOpts *opts;
 int hotplugged;
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 7ed1f431f0..89e03c7b47 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -946,8 +946,8 @@ static bool device_get_hotpluggable(Object *obj, Error 
**errp)
 DeviceClass *dc = DEVICE_GET_CLASS(obj);
 DeviceState *dev = DEVICE(obj);
 
-return dc->hotpluggable && (dev->parent_bus == NULL ||
-qbus_is_hotpluggable(dev->parent_bus));
+return dc->hotpluggable && dev->hotpluggable &&
+(dev->parent_bus == NULL || qbus_is_hotpluggable(dev->parent_bus));
 }
 
 static bool device_get_hotplugged(Object *obj, Error **err)
@@ -988,6 +988,7 @@ static void device_initfn(Object *obj)
 class = object_class_get_parent(class);
 } while (class != object_class_by_name(TYPE_DEVICE));
 
+dev->hotpluggable = DEVICE_CLASS(class)->hotpluggable;
 object_property_add_link(OBJECT(dev), "parent_bus", TYPE_BUS,
  (Object **)>parent_bus, NULL, 0,
  _abort);
-- 
2.9.3