[Qemu-devel] [RFC PATCH spice/qemu v2 0/2] QXL interface to set monitor ID

[Qemu-devel] [RFC PATCH spice/qemu v2 0/2] QXL interface to set monitor ID

[Lukáš Hrázký]

Hello,

sending version 2 of the RFC on the new QXL interface to identify the
graphics device monitors in the guest.

Still keeping it as RFC, I'd like to get this agreed upon but not merge
until the whole mechanism is implemented.

Gerd, I didn't split the QEMU patch, as I feel it kind of goes together,
one without the other is not really functional. I'll still do it if you
insist...

Changes since v1:
* Renamed both functions according to suggestions.
* Changed the spice_qxl_monitor_set_device_display_id() so that it
  actually takes the monitor_id as an argument from QEMU. Please see the
  documentation of the function for some explanation.
* Updated the docs.

Cheers,
Lukas

-- 
2.19.1

[Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest

[Lukáš Hrázký]

Adds two functions to let QEMU provide information to identify graphics
devices and their monitors in the guest:

* device address - The path identifying the device on the system (e.g. PCI
  path):
  spice_qxl_set_device_address(...)

* device display ID - The index of the monitor on the graphics device:
  spice_qxl_monitor_set_device_display_id(...)

Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
---
 server/red-qxl.c         | 89 ++++++++++++++++++++++++++++++++++++++++
 server/spice-qxl.h       |  5 +++
 server/spice-server.syms |  6 +++
 3 files changed, 100 insertions(+)

diff --git a/server/red-qxl.c b/server/red-qxl.c
index 97940611..0b2043e1 100644
--- a/server/red-qxl.c
+++ b/server/red-qxl.c
@@ -41,6 +41,9 @@
 #include "red-qxl.h"
 
 
+#define MAX_PATH_LEN 256
+#define MAX_MONITORS_COUNT 16
+
 struct QXLState {
     QXLWorker qxl_worker;
     QXLInstance *qxl;
@@ -53,6 +56,9 @@ struct QXLState {
     unsigned int max_monitors;
     RedsState *reds;
     RedWorker *worker;
+    char device_address[MAX_PATH_LEN];
+    uint32_t device_display_ids[MAX_MONITORS_COUNT];
+    size_t monitors_count;  // length of ^^^
 
     pthread_mutex_t scanout_mutex;
     SpiceMsgDisplayGlScanoutUnix scanout;
@@ -846,6 +852,89 @@ void red_qxl_gl_draw_async_complete(QXLInstance *qxl)
     red_qxl_async_complete(qxl, cookie);
 }
 
+/**
+ * spice_qxl_set_device_address:
+ * @instance the QXL instance to set the path to
+ * @device_address the path of the device this QXL instance belongs to
+ *
+ * Sets the hardware address (e.g. PCI) of the graphics device represented by
+ * this QXL interface instance.
+ *
+ * The supported format is:
+ * "pci/<DOMAIN>/<SLOT>.<FUNCTION>/.../<SLOT>.<FUNCTION>"
+ *
+ * The "pci" identifies the rest of the string as a PCI adress. It is the only
+ * supported address at the moment, other identifiers can be introduced later.
+ * <DOMAIN> is the PCI domain, followed by <SLOT>.<FUNCTION> of any PCI bridges
+ * in the chain leading to the device. The last <SLOT>.<FUNCTION> is the
+ * graphics device.
+ */
+SPICE_GNUC_VISIBLE
+void spice_qxl_set_device_address(QXLInstance *instance, const char *device_address)
+{
+    g_return_if_fail(device_address != NULL);
+
+    size_t dp_len = strnlen(device_address, MAX_PATH_LEN);
+    if (dp_len >= MAX_PATH_LEN) {
+        spice_error("PCI device path too long: %lu > %u", dp_len, MAX_PATH_LEN);
+        return;
+    }
+
+    strncpy(instance->st->device_address, device_address, MAX_PATH_LEN);
+
+    g_debug("QXL Instance %d setting device address \"%s\"", instance->id, device_address);
+}
+
+/**
+ * spice_qxl_monitor_set_device_display_id:
+ * @instance the QXL instance to set the device display ID to
+ * @monitor_id the SPICE monitor ID to set the device display ID to
+ * @device_display_id the actual ID of the display (output) on the graphics device
+ *
+ * Sets the device display ID for a given monitor ID in a QXL instance. The
+ * monitor IDs are expected and required to be a consecutive sequence starting
+ * at 0. The function requires the calls to be made in the sequence to prevent
+ * holes.
+ *
+ * The requirement for the monitor ID to be a sequence starting from 0 comes
+ * from the mechanism of generating a single display_id from channel_id and
+ * monitor_id on the client:
+ *
+ * display_id = channel_id + monitor_id
+ *
+ * This is unambiguous either if there is only a single channel with multiple
+ * monitors ("legacy" QXL on linux case) or multiple channels with only a
+ * single monitor. Also both channel_id and monitor_id need to be a sequence
+ * starting from 0, otherwise there is still a possibility of collisions.
+ */
+SPICE_GNUC_VISIBLE
+void spice_qxl_monitor_set_device_display_id(QXLInstance *instance,
+                                             uint32_t monitor_id,
+                                             uint32_t device_display_id)
+{
+    if (instance->st->monitors_count >= MAX_MONITORS_COUNT) {
+        spice_error("Cannot register more than %u monitors per QXL interface", MAX_MONITORS_COUNT);
+        return;
+    }
+
+    if (monitor_id > instance->st->monitors_count) {
+        spice_error("Monitor ID %u is not inside a consecutive sequence of monitor IDs "
+                    "starting from zero. Needs to be lower than or equal to %lu.",
+                    monitor_id, instance->st->monitors_count);
+        return;
+    }
+
+    instance->st->device_display_ids[monitor_id] = device_display_id;
+
+    g_debug("QXL Instance %d setting device display ID %u for monitor ID %u",
+        instance->id, device_display_id, monitor_id);
+
+    // if we're adding a new ID (and not resetting an existing one), increment the array length
+    if (monitor_id == instance->st->monitors_count) {
+        instance->st->monitors_count++;
+    }
+}
+
 void red_qxl_init(RedsState *reds, QXLInstance *qxl)
 {
     QXLState *qxl_state;
diff --git a/server/spice-qxl.h b/server/spice-qxl.h
index 0c4e75fc..c9ea6564 100644
--- a/server/spice-qxl.h
+++ b/server/spice-qxl.h
@@ -114,6 +114,11 @@ void spice_qxl_gl_draw_async(QXLInstance *instance,
                              uint32_t x, uint32_t y,
                              uint32_t w, uint32_t h,
                              uint64_t cookie);
+/* since spice 0.14.2 */
+void spice_qxl_set_device_address(QXLInstance *instance, const char *device_path);
+void spice_qxl_monitor_set_device_display_id(QXLInstance *instance,
+                                             uint32_t monitor_id,
+                                             uint32_t device_display_id);
 
 typedef struct QXLDevInitInfo {
     uint32_t num_memslots_groups;
diff --git a/server/spice-server.syms b/server/spice-server.syms
index edf04a42..6e9ffa93 100644
--- a/server/spice-server.syms
+++ b/server/spice-server.syms
@@ -173,3 +173,9 @@ SPICE_SERVER_0.13.2 {
 global:
     spice_server_set_video_codecs;
 } SPICE_SERVER_0.13.1;
+
+SPICE_SERVER_0.14.2 {
+global:
+    spice_qxl_set_device_address;
+    spice_qxl_monitor_set_device_display_id;
+} SPICE_SERVER_0.13.2;
-- 
2.19.1

[Qemu-devel] [RFC PATCH qemu v2 2/2] spice: set device address and device display ID in QXL interface

[Lukáš Hrázký]

Calls new SPICE QXL interface functions to set:

* The hardware address of the graphics device represented by the QXL
  interface (e.g. a PCI path):
  spice_qxl_set_device_address(...)

* The device display IDs (the IDs of the device's monitors that belong
  to this QXL interface):
  spice_qxl_monitor_set_device_display_id(...)

Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
---
 hw/display/qxl.c   |  8 ++++++++
 ui/spice-core.c    | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 ui/spice-display.c |  5 +++++
 3 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index f608abc769..63144f42b4 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2184,6 +2184,14 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error **errp)
                    SPICE_INTERFACE_QXL_MAJOR, SPICE_INTERFACE_QXL_MINOR);
         return;
     }
+
+#if SPICE_SERVER_VERSION >= 0x000e02 /* release 0.14.2 */
+    // register all possible device display IDs to SPICE server
+    for (int i = 0; i < qxl->max_outputs; ++i) {
+        spice_qxl_monitor_set_device_display_id(&qxl->ssd.qxl, i, i);
+    }
+#endif
+
     qemu_add_vm_change_state_handler(qxl_vm_change_state_handler, qxl);
 
     qxl->update_irq = qemu_bh_new(qxl_update_irq_bh, qxl);
diff --git a/ui/spice-core.c b/ui/spice-core.c
index a4fbbc3898..2e01beea03 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -35,6 +35,8 @@
 #include "qemu/option.h"
 #include "migration/misc.h"
 #include "hw/hw.h"
+#include "hw/pci/pci.h"
+#include "hw/pci/pci_bus.h"
 #include "ui/spice-display.h"
 
 /* core bits */
@@ -871,6 +873,26 @@ bool qemu_spice_have_display_interface(QemuConsole *con)
     return false;
 }
 
+/*
+ * Recursively (in reverse order) appends addresses of PCI devices as it moves
+ * up in the PCI hierarchy.
+ *
+ * @returns true on success, false when the buffer wasn't large enough
+ */
+static bool append_pci_address_recursive(char *buf, size_t buf_size, const PCIDevice *pci)
+{
+    PCIBus *bus = pci_get_bus(pci);
+    if (!pci_bus_is_root(bus)) {
+        append_pci_address_recursive(buf, buf_size, bus->parent_dev);
+    }
+
+    size_t len = strlen(buf);
+    size_t written = snprintf(buf + len, buf_size - len, "/%02x.%x",
+        PCI_SLOT(pci->devfn), PCI_FUNC(pci->devfn));
+
+    return written > 0 && written < buf_size - len;
+}
+
 int qemu_spice_add_display_interface(QXLInstance *qxlin, QemuConsole *con)
 {
     if (g_slist_find(spice_consoles, con)) {
@@ -878,7 +900,28 @@ int qemu_spice_add_display_interface(QXLInstance *qxlin, QemuConsole *con)
     }
     qxlin->id = qemu_console_get_index(con);
     spice_consoles = g_slist_append(spice_consoles, con);
-    return qemu_spice_add_interface(&qxlin->base);
+    int res = qemu_spice_add_interface(&qxlin->base);
+
+#if SPICE_SERVER_VERSION >= 0x000e02 /* release 0.14.2 */
+    DeviceState *dev = DEVICE(object_property_get_link(OBJECT(con), "device", &error_abort));
+    PCIDevice *pci = (PCIDevice *) object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE);
+
+    if (pci == NULL) {
+        warn_report("Setting PCI path of a display device to SPICE: Not a PCI device.");
+        return res;
+    }
+
+    char device_path[128] = "pci/0000";  // hardcoded PCI domain 0000
+    if (!append_pci_address_recursive(device_path, sizeof(device_path), pci)) {
+        warn_report("Setting PCI path of a display device to SPICE: "
+            "Too many PCI devices in the chain.");
+        return res;
+    }
+
+    spice_qxl_set_device_address(qxlin, device_path);
+#endif
+
+    return res;
 }
 
 static int qemu_spice_set_ticket(bool fail_if_conn, bool disconnect_if_conn)
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 2f8adb6b9f..f620750803 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -1129,6 +1129,11 @@ static void qemu_spice_display_init_one(QemuConsole *con)
 
     ssd->qxl.base.sif = &dpy_interface.base;
     qemu_spice_add_display_interface(&ssd->qxl, con);
+
+#if SPICE_SERVER_VERSION >= 0x000e02 /* release 0.14.2 */
+    spice_qxl_monitor_set_device_display_id(&ssd->qxl, 0, qemu_console_get_head(con));
+#endif
+
     qemu_spice_create_host_memslot(ssd);
 
     register_displaychangelistener(&ssd->dcl);
-- 
2.19.1

Re: [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest

[Frediano Ziglio]

> 
> Adds two functions to let QEMU provide information to identify graphics
> devices and their monitors in the guest:
> 
> * device address - The path identifying the device on the system (e.g. PCI
>   path):
>   spice_qxl_set_device_address(...)
> 
> * device display ID - The index of the monitor on the graphics device:
>   spice_qxl_monitor_set_device_display_id(...)

This seems to indicate that this device is bound in some way to the previous
information but having 2 APIs make more fragile, potentially one could
call a function and not the other or in different order or mismatch them.

> 
> Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
> ---
>  server/red-qxl.c         | 89 ++++++++++++++++++++++++++++++++++++++++
>  server/spice-qxl.h       |  5 +++
>  server/spice-server.syms |  6 +++
>  3 files changed, 100 insertions(+)
> 
> diff --git a/server/red-qxl.c b/server/red-qxl.c
> index 97940611..0b2043e1 100644
> --- a/server/red-qxl.c
> +++ b/server/red-qxl.c
> @@ -41,6 +41,9 @@
>  #include "red-qxl.h"
>  
>  
> +#define MAX_PATH_LEN 256

Different OSes uses MAX_PATH/MAXPATH to specify filename limit so this sounds
confusing, maybe MAX_DEVICE_PATH_LEN would be better.

> +#define MAX_MONITORS_COUNT 16
> +
>  struct QXLState {
>      QXLWorker qxl_worker;
>      QXLInstance *qxl;
> @@ -53,6 +56,9 @@ struct QXLState {
>      unsigned int max_monitors;
>      RedsState *reds;
>      RedWorker *worker;
> +    char device_address[MAX_PATH_LEN];
> +    uint32_t device_display_ids[MAX_MONITORS_COUNT];
> +    size_t monitors_count;  // length of ^^^

size_t look a bit too much for a number of item in a small array.

>  
>      pthread_mutex_t scanout_mutex;
>      SpiceMsgDisplayGlScanoutUnix scanout;
> @@ -846,6 +852,89 @@ void red_qxl_gl_draw_async_complete(QXLInstance *qxl)
>      red_qxl_async_complete(qxl, cookie);
>  }
>  
> +/**
> + * spice_qxl_set_device_address:
> + * @instance the QXL instance to set the path to
> + * @device_address the path of the device this QXL instance belongs to
> + *
> + * Sets the hardware address (e.g. PCI) of the graphics device represented
> by
> + * this QXL interface instance.
> + *
> + * The supported format is:
> + * "pci/<DOMAIN>/<SLOT>.<FUNCTION>/.../<SLOT>.<FUNCTION>"
> + *
> + * The "pci" identifies the rest of the string as a PCI adress. It is the
> only

typo: adress

> + * supported address at the moment, other identifiers can be introduced
> later.
> + * <DOMAIN> is the PCI domain, followed by <SLOT>.<FUNCTION> of any PCI
> bridges
> + * in the chain leading to the device. The last <SLOT>.<FUNCTION> is the
> + * graphics device.

Maybe better to specify also the encoding, like decimal/hexadecimal and number
of digits.

> + */

I would prefer documentation in the header so people don't have to open
the source to get it, also considering the the header is public.

> +SPICE_GNUC_VISIBLE
> +void spice_qxl_set_device_address(QXLInstance *instance, const char
> *device_address)
> +{
> +    g_return_if_fail(device_address != NULL);
> +
> +    size_t dp_len = strnlen(device_address, MAX_PATH_LEN);
> +    if (dp_len >= MAX_PATH_LEN) {
> +        spice_error("PCI device path too long: %lu > %u", dp_len,
> MAX_PATH_LEN);
> +        return;
> +    }
> +
> +    strncpy(instance->st->device_address, device_address, MAX_PATH_LEN);
> +
> +    g_debug("QXL Instance %d setting device address \"%s\"", instance->id,
> device_address);
> +}
> +
> +/**
> + * spice_qxl_monitor_set_device_display_id:
> + * @instance the QXL instance to set the device display ID to
> + * @monitor_id the SPICE monitor ID to set the device display ID to
> + * @device_display_id the actual ID of the display (output) on the graphics
> device
> + *
> + * Sets the device display ID for a given monitor ID in a QXL instance. The
> + * monitor IDs are expected and required to be a consecutive sequence
> starting
> + * at 0. The function requires the calls to be made in the sequence to
> prevent
> + * holes.
> + *
> + * The requirement for the monitor ID to be a sequence starting from 0 comes
> + * from the mechanism of generating a single display_id from channel_id and
> + * monitor_id on the client:
> + *
> + * display_id = channel_id + monitor_id
> + *

No, the monitor_id sequence is defined as a sequence from 0, has nothing
to do with display_id. Also this comment seems to indicate that this new
interface is bad designed having the same limit, which is not.

> + * This is unambiguous either if there is only a single channel with
> multiple
> + * monitors ("legacy" QXL on linux case) or multiple channels with only a
> + * single monitor. Also both channel_id and monitor_id need to be a sequence
> + * starting from 0, otherwise there is still a possibility of collisions.

They are both defined (channel_id and monitor_id) as sequences starting from 0.
I don't see why need to be specified here.

> + */
> +SPICE_GNUC_VISIBLE
> +void spice_qxl_monitor_set_device_display_id(QXLInstance *instance,
> +                                             uint32_t monitor_id,
> +                                             uint32_t device_display_id)

I still don't understand why, as suggested by Gerd, we need another function
instead of 2 additional parameters to the above API specifying start and
number, this API looks much more prone to errors.

Also there's no much documentation for this "device display ID" in the code,
potentially can be generated with something like:

uint32_t next_device_display_id(void)
{
   static unsigned id = 0;
   return (id++ + 0xf00) * 0xbeef;
}

which probably won't work for you.

> +{
> +    if (instance->st->monitors_count >= MAX_MONITORS_COUNT) {
> +        spice_error("Cannot register more than %u monitors per QXL
> interface", MAX_MONITORS_COUNT);
> +        return;
> +    }
> +
> +    if (monitor_id > instance->st->monitors_count) {
> +        spice_error("Monitor ID %u is not inside a consecutive sequence of
> monitor IDs "
> +                    "starting from zero. Needs to be lower than or equal to
> %lu.",
> +                    monitor_id, instance->st->monitors_count);
> +        return;
> +    }
> +
> +    instance->st->device_display_ids[monitor_id] = device_display_id;
> +
> +    g_debug("QXL Instance %d setting device display ID %u for monitor ID
> %u",
> +        instance->id, device_display_id, monitor_id);
> +
> +    // if we're adding a new ID (and not resetting an existing one),
> increment the array length
> +    if (monitor_id == instance->st->monitors_count) {
> +        instance->st->monitors_count++;
> +    }
> +}
> +
>  void red_qxl_init(RedsState *reds, QXLInstance *qxl)
>  {
>      QXLState *qxl_state;
> diff --git a/server/spice-qxl.h b/server/spice-qxl.h
> index 0c4e75fc..c9ea6564 100644
> --- a/server/spice-qxl.h
> +++ b/server/spice-qxl.h
> @@ -114,6 +114,11 @@ void spice_qxl_gl_draw_async(QXLInstance *instance,
>                               uint32_t x, uint32_t y,
>                               uint32_t w, uint32_t h,
>                               uint64_t cookie);
> +/* since spice 0.14.2 */
> +void spice_qxl_set_device_address(QXLInstance *instance, const char
> *device_path);
> +void spice_qxl_monitor_set_device_display_id(QXLInstance *instance,
> +                                             uint32_t monitor_id,
> +                                             uint32_t device_display_id);
>  
>  typedef struct QXLDevInitInfo {
>      uint32_t num_memslots_groups;
> diff --git a/server/spice-server.syms b/server/spice-server.syms
> index edf04a42..6e9ffa93 100644
> --- a/server/spice-server.syms
> +++ b/server/spice-server.syms
> @@ -173,3 +173,9 @@ SPICE_SERVER_0.13.2 {
>  global:
>      spice_server_set_video_codecs;
>  } SPICE_SERVER_0.13.1;
> +
> +SPICE_SERVER_0.14.2 {
> +global:
> +    spice_qxl_set_device_address;
> +    spice_qxl_monitor_set_device_display_id;
> +} SPICE_SERVER_0.13.2;

Frediano

Re: [Qemu-devel] [RFC PATCH qemu v2 2/2] spice: set device address and device display ID in QXL interface

[Frediano Ziglio]

> 
> Calls new SPICE QXL interface functions to set:
> 
> * The hardware address of the graphics device represented by the QXL
>   interface (e.g. a PCI path):
>   spice_qxl_set_device_address(...)
> 
> * The device display IDs (the IDs of the device's monitors that belong
>   to this QXL interface):
>   spice_qxl_monitor_set_device_display_id(...)
> 
> Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
> ---
>  hw/display/qxl.c   |  8 ++++++++
>  ui/spice-core.c    | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>  ui/spice-display.c |  5 +++++
>  3 files changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index f608abc769..63144f42b4 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -2184,6 +2184,14 @@ static void qxl_realize_common(PCIQXLDevice *qxl,
> Error **errp)
>                     SPICE_INTERFACE_QXL_MAJOR, SPICE_INTERFACE_QXL_MINOR);
>          return;
>      }
> +
> +#if SPICE_SERVER_VERSION >= 0x000e02 /* release 0.14.2 */
> +    // register all possible device display IDs to SPICE server
> +    for (int i = 0; i < qxl->max_outputs; ++i) {
> +        spice_qxl_monitor_set_device_display_id(&qxl->ssd.qxl, i, i);
> +    }
> +#endif
> +

I think would be better if this were the default in spice-server.

>      qemu_add_vm_change_state_handler(qxl_vm_change_state_handler, qxl);
>  
>      qxl->update_irq = qemu_bh_new(qxl_update_irq_bh, qxl);
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index a4fbbc3898..2e01beea03 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -35,6 +35,8 @@
>  #include "qemu/option.h"
>  #include "migration/misc.h"
>  #include "hw/hw.h"
> +#include "hw/pci/pci.h"
> +#include "hw/pci/pci_bus.h"
>  #include "ui/spice-display.h"
>  
>  /* core bits */
> @@ -871,6 +873,26 @@ bool qemu_spice_have_display_interface(QemuConsole *con)
>      return false;
>  }
>  
> +/*
> + * Recursively (in reverse order) appends addresses of PCI devices as it
> moves
> + * up in the PCI hierarchy.
> + *
> + * @returns true on success, false when the buffer wasn't large enough
> + */
> +static bool append_pci_address_recursive(char *buf, size_t buf_size, const
> PCIDevice *pci)

I won't use the "_recursive" in the name, looks like more an implementation
detail.

> +{
> +    PCIBus *bus = pci_get_bus(pci);
> +    if (!pci_bus_is_root(bus)) {
> +        append_pci_address_recursive(buf, buf_size, bus->parent_dev);
> +    }
> +
> +    size_t len = strlen(buf);
> +    size_t written = snprintf(buf + len, buf_size - len, "/%02x.%x",
> +        PCI_SLOT(pci->devfn), PCI_FUNC(pci->devfn));
> +
> +    return written > 0 && written < buf_size - len;

written is always > 0, unless there's a bug in snprintf. Note that Qemu uses
MingW snprintf on Windows so even on Windows this returns always > 0 (note
that snprintf returns an int which is signed while size_t is unsigned and
some implementation could return < 0, but not in Qemu).

> +}
> +
>  int qemu_spice_add_display_interface(QXLInstance *qxlin, QemuConsole *con)
>  {
>      if (g_slist_find(spice_consoles, con)) {
> @@ -878,7 +900,28 @@ int qemu_spice_add_display_interface(QXLInstance *qxlin,
> QemuConsole *con)
>      }
>      qxlin->id = qemu_console_get_index(con);
>      spice_consoles = g_slist_append(spice_consoles, con);
> -    return qemu_spice_add_interface(&qxlin->base);
> +    int res = qemu_spice_add_interface(&qxlin->base);
> +
> +#if SPICE_SERVER_VERSION >= 0x000e02 /* release 0.14.2 */
> +    DeviceState *dev = DEVICE(object_property_get_link(OBJECT(con),
> "device", &error_abort));
> +    PCIDevice *pci = (PCIDevice *) object_dynamic_cast(OBJECT(dev),
> TYPE_PCI_DEVICE);
> +
> +    if (pci == NULL) {
> +        warn_report("Setting PCI path of a display device to SPICE: Not a
> PCI device.");
> +        return res;
> +    }
> +
> +    char device_path[128] = "pci/0000";  // hardcoded PCI domain 0000
> +    if (!append_pci_address_recursive(device_path, sizeof(device_path),
> pci)) {
> +        warn_report("Setting PCI path of a display device to SPICE: "
> +            "Too many PCI devices in the chain.");
> +        return res;
> +    }
> +
> +    spice_qxl_set_device_address(qxlin, device_path);
> +#endif
> +
> +    return res;
>  }
>  
>  static int qemu_spice_set_ticket(bool fail_if_conn, bool disconnect_if_conn)
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 2f8adb6b9f..f620750803 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -1129,6 +1129,11 @@ static void qemu_spice_display_init_one(QemuConsole
> *con)
>  
>      ssd->qxl.base.sif = &dpy_interface.base;
>      qemu_spice_add_display_interface(&ssd->qxl, con);
> +
> +#if SPICE_SERVER_VERSION >= 0x000e02 /* release 0.14.2 */

Surely OT: won't be better to have something like

#if SPICE_SERVER_VERSION >= SPICE_SERVER_VERSION_NUMBER(0,14,2)

> +    spice_qxl_monitor_set_device_display_id(&ssd->qxl, 0,
> qemu_console_get_head(con));
> +#endif
> +
>      qemu_spice_create_host_memslot(ssd);
>  
>      register_displaychangelistener(&ssd->dcl);

Frediano

Re: [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest

[Gerd Hoffmann]

> > + * supported address at the moment, other identifiers can be introduced
> > later.
> > + * <DOMAIN> is the PCI domain, followed by <SLOT>.<FUNCTION> of any PCI
> > bridges
> > + * in the chain leading to the device. The last <SLOT>.<FUNCTION> is the
> > + * graphics device.
> 
> Maybe better to specify also the encoding, like decimal/hexadecimal and number
> of digits.

lspci style (i.e. slot is two digits hex, function is one digit).

> > +/**
> > + * spice_qxl_monitor_set_device_display_id:
> > + * @instance the QXL instance to set the device display ID to
> > + * @monitor_id the SPICE monitor ID to set the device display ID to
> > + * @device_display_id the actual ID of the display (output) on the graphics
> > device

Hmm, why do we need the monitor_id here?

The reason to have this function is to allow the guest agent figure
which channel is which head in case we have multiple display channels
registered for one device.

For linux-qxl multihead we don't need this as all heads use the same
display channel, right?

> > +SPICE_GNUC_VISIBLE
> > +void spice_qxl_monitor_set_device_display_id(QXLInstance *instance,
> > +                                             uint32_t monitor_id,
> > +                                             uint32_t device_display_id)
> 
> I still don't understand why, as suggested by Gerd, we need another function
> instead of 2 additional parameters to the above API specifying start and
> number, this API looks much more prone to errors.

We can also just add a device_display_id parameter to the other
function, that should work too.

> Also there's no much documentation for this "device display ID" in the code,
> potentially can be generated with something like:

It's the head index, starting at zero.

cheers,
  Gerd

Re: [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest

[Lukáš Hrázký]

Hello,

On Thu, 2018-10-18 at 03:16 -0400, Frediano Ziglio wrote:
> > 
> > Adds two functions to let QEMU provide information to identify graphics
> > devices and their monitors in the guest:
> > 
> > * device address - The path identifying the device on the system (e.g. PCI
> >   path):
> >   spice_qxl_set_device_address(...)
> > 
> > * device display ID - The index of the monitor on the graphics device:
> >   spice_qxl_monitor_set_device_display_id(...)
> 
> This seems to indicate that this device is bound in some way to the previous
> information but having 2 APIs make more fragile, potentially one could
> call a function and not the other or in different order or mismatch them.

Not sure what you mean by "previous information".

Yes, you need to call both functions... The order doesn't matter and
you can always send wrong data in a single function as well. I agree in
this regard a single function would be better but don't find it a big
issue. Note that the functions are called in different places in QEMU
too.

> > 
> > Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
> > ---
> >  server/red-qxl.c         | 89 ++++++++++++++++++++++++++++++++++++++++
> >  server/spice-qxl.h       |  5 +++
> >  server/spice-server.syms |  6 +++
> >  3 files changed, 100 insertions(+)
> > 
> > diff --git a/server/red-qxl.c b/server/red-qxl.c
> > index 97940611..0b2043e1 100644
> > --- a/server/red-qxl.c
> > +++ b/server/red-qxl.c
> > @@ -41,6 +41,9 @@
> >  #include "red-qxl.h"
> >  
> >  
> > +#define MAX_PATH_LEN 256
> 
> Different OSes uses MAX_PATH/MAXPATH to specify filename limit so this sounds
> confusing, maybe MAX_DEVICE_PATH_LEN would be better.

Will do.

> > +#define MAX_MONITORS_COUNT 16
> > +
> >  struct QXLState {
> >      QXLWorker qxl_worker;
> >      QXLInstance *qxl;
> > @@ -53,6 +56,9 @@ struct QXLState {
> >      unsigned int max_monitors;
> >      RedsState *reds;
> >      RedWorker *worker;
> > +    char device_address[MAX_PATH_LEN];
> > +    uint32_t device_display_ids[MAX_MONITORS_COUNT];
> > +    size_t monitors_count;  // length of ^^^
> 
> size_t look a bit too much for a number of item in a small array.

It's the go-to type for indexes for me, I'm not used to doing these
little optimizations. If you think it matters, I can change it to like
uint8_t...

> >  
> >      pthread_mutex_t scanout_mutex;
> >      SpiceMsgDisplayGlScanoutUnix scanout;
> > @@ -846,6 +852,89 @@ void red_qxl_gl_draw_async_complete(QXLInstance *qxl)
> >      red_qxl_async_complete(qxl, cookie);
> >  }
> >  
> > +/**
> > + * spice_qxl_set_device_address:
> > + * @instance the QXL instance to set the path to
> > + * @device_address the path of the device this QXL instance belongs to
> > + *
> > + * Sets the hardware address (e.g. PCI) of the graphics device represented
> > by
> > + * this QXL interface instance.
> > + *
> > + * The supported format is:
> > + * "pci/<DOMAIN>/<SLOT>.<FUNCTION>/.../<SLOT>.<FUNCTION>"
> > + *
> > + * The "pci" identifies the rest of the string as a PCI adress. It is the
> > only
> 
> typo: adress
> 
> > + * supported address at the moment, other identifiers can be introduced
> > later.
> > + * <DOMAIN> is the PCI domain, followed by <SLOT>.<FUNCTION> of any PCI
> > bridges
> > + * in the chain leading to the device. The last <SLOT>.<FUNCTION> is the
> > + * graphics device.
> 
> Maybe better to specify also the encoding, like decimal/hexadecimal and number
> of digits.

Right, I'll improve on that.

> > + */
> 
> I would prefer documentation in the header so people don't have to open
> the source to get it, also considering the the header is public.

Sure, makes sense. I just did it the same way as it's done in spice-
gtk, which has the docstrings in .c files for some reason...

> > +SPICE_GNUC_VISIBLE
> > +void spice_qxl_set_device_address(QXLInstance *instance, const char
> > *device_address)
> > +{
> > +    g_return_if_fail(device_address != NULL);
> > +
> > +    size_t dp_len = strnlen(device_address, MAX_PATH_LEN);
> > +    if (dp_len >= MAX_PATH_LEN) {
> > +        spice_error("PCI device path too long: %lu > %u", dp_len,
> > MAX_PATH_LEN);
> > +        return;
> > +    }
> > +
> > +    strncpy(instance->st->device_address, device_address, MAX_PATH_LEN);
> > +
> > +    g_debug("QXL Instance %d setting device address \"%s\"", instance->id,
> > device_address);
> > +}
> > +
> > +/**
> > + * spice_qxl_monitor_set_device_display_id:
> > + * @instance the QXL instance to set the device display ID to
> > + * @monitor_id the SPICE monitor ID to set the device display ID to
> > + * @device_display_id the actual ID of the display (output) on the graphics
> > device
> > + *
> > + * Sets the device display ID for a given monitor ID in a QXL instance. The
> > + * monitor IDs are expected and required to be a consecutive sequence
> > starting
> > + * at 0. The function requires the calls to be made in the sequence to
> > prevent
> > + * holes.
> > + *
> > + * The requirement for the monitor ID to be a sequence starting from 0 comes
> > + * from the mechanism of generating a single display_id from channel_id and
> > + * monitor_id on the client:
> > + *
> > + * display_id = channel_id + monitor_id
> > + *
> 
> No, the monitor_id sequence is defined as a sequence from 0, has nothing
> to do with display_id. Also this comment seems to indicate that this new
> interface is bad designed having the same limit, which is not.

I don't think monitor_id is defined anywhere to be a sequence starting
from 0. If it is, please point me to it. From what I understand,
monitor_id is an arbitrary identifier without further specification
which happens to always be a sequence starting from 0. The thing is
that the way display_id is calculated depends on it, so it is
practically required to start from 0. That's what the docstring is
trying to say.

If I understood correctly, Gerd actually at some point suggested to use
device_display_id as monitor_id, which would not always be a sequence
starting from 0 and that would break the display_id calculation.

Not sure if I can improve the documentation in any way, suggestions
welcome.

> > + * This is unambiguous either if there is only a single channel with
> > multiple
> > + * monitors ("legacy" QXL on linux case) or multiple channels with only a
> > + * single monitor. Also both channel_id and monitor_id need to be a sequence
> > + * starting from 0, otherwise there is still a possibility of collisions.
> 
> They are both defined (channel_id and monitor_id) as sequences starting from 0.
> I don't see why need to be specified here.

As I said, I don't know of such a definition...

> > + */
> > +SPICE_GNUC_VISIBLE
> > +void spice_qxl_monitor_set_device_display_id(QXLInstance *instance,
> > +                                             uint32_t monitor_id,
> > +                                             uint32_t device_display_id)
> 
> I still don't understand why, as suggested by Gerd, we need another function
> instead of 2 additional parameters to the above API specifying start and
> number, this API looks much more prone to errors.

Possibly. I (and I think Jonathon too?) didn't like the start and
number API, this was proposed as an alternative.

> Also there's no much documentation for this "device display ID" in the code,
> potentially can be generated with something like:

I'll add it to the documentation. I think we do agree it's the index of
the actual monitor output on the graphics device?

Cheers,
Lukas

> uint32_t next_device_display_id(void)
> {
>    static unsigned id = 0;
>    return (id++ + 0xf00) * 0xbeef;
> }
> 
> which probably won't work for you.
> 
> > +{
> > +    if (instance->st->monitors_count >= MAX_MONITORS_COUNT) {
> > +        spice_error("Cannot register more than %u monitors per QXL
> > interface", MAX_MONITORS_COUNT);
> > +        return;
> > +    }
> > +
> > +    if (monitor_id > instance->st->monitors_count) {
> > +        spice_error("Monitor ID %u is not inside a consecutive sequence of
> > monitor IDs "
> > +                    "starting from zero. Needs to be lower than or equal to
> > %lu.",
> > +                    monitor_id, instance->st->monitors_count);
> > +        return;
> > +    }
> > +
> > +    instance->st->device_display_ids[monitor_id] = device_display_id;
> > +
> > +    g_debug("QXL Instance %d setting device display ID %u for monitor ID
> > %u",
> > +        instance->id, device_display_id, monitor_id);
> > +
> > +    // if we're adding a new ID (and not resetting an existing one),
> > increment the array length
> > +    if (monitor_id == instance->st->monitors_count) {
> > +        instance->st->monitors_count++;
> > +    }
> > +}
> > +
> >  void red_qxl_init(RedsState *reds, QXLInstance *qxl)
> >  {
> >      QXLState *qxl_state;
> > diff --git a/server/spice-qxl.h b/server/spice-qxl.h
> > index 0c4e75fc..c9ea6564 100644
> > --- a/server/spice-qxl.h
> > +++ b/server/spice-qxl.h
> > @@ -114,6 +114,11 @@ void spice_qxl_gl_draw_async(QXLInstance *instance,
> >                               uint32_t x, uint32_t y,
> >                               uint32_t w, uint32_t h,
> >                               uint64_t cookie);
> > +/* since spice 0.14.2 */
> > +void spice_qxl_set_device_address(QXLInstance *instance, const char
> > *device_path);
> > +void spice_qxl_monitor_set_device_display_id(QXLInstance *instance,
> > +                                             uint32_t monitor_id,
> > +                                             uint32_t device_display_id);
> >  
> >  typedef struct QXLDevInitInfo {
> >      uint32_t num_memslots_groups;
> > diff --git a/server/spice-server.syms b/server/spice-server.syms
> > index edf04a42..6e9ffa93 100644
> > --- a/server/spice-server.syms
> > +++ b/server/spice-server.syms
> > @@ -173,3 +173,9 @@ SPICE_SERVER_0.13.2 {
> >  global:
> >      spice_server_set_video_codecs;
> >  } SPICE_SERVER_0.13.1;
> > +
> > +SPICE_SERVER_0.14.2 {
> > +global:
> > +    spice_qxl_set_device_address;
> > +    spice_qxl_monitor_set_device_display_id;
> > +} SPICE_SERVER_0.13.2;
> 
> Frediano

Re: [Qemu-devel] [RFC PATCH qemu v2 2/2] spice: set device address and device display ID in QXL interface

[Lukáš Hrázký]

On Thu, 2018-10-18 at 03:38 -0400, Frediano Ziglio wrote:
> > 
> > Calls new SPICE QXL interface functions to set:
> > 
> > * The hardware address of the graphics device represented by the QXL
> >   interface (e.g. a PCI path):
> >   spice_qxl_set_device_address(...)
> > 
> > * The device display IDs (the IDs of the device's monitors that belong
> >   to this QXL interface):
> >   spice_qxl_monitor_set_device_display_id(...)
> > 
> > Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
> > ---
> >  hw/display/qxl.c   |  8 ++++++++
> >  ui/spice-core.c    | 45 ++++++++++++++++++++++++++++++++++++++++++++-
> >  ui/spice-display.c |  5 +++++
> >  3 files changed, 57 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> > index f608abc769..63144f42b4 100644
> > --- a/hw/display/qxl.c
> > +++ b/hw/display/qxl.c
> > @@ -2184,6 +2184,14 @@ static void qxl_realize_common(PCIQXLDevice *qxl,
> > Error **errp)
> >                     SPICE_INTERFACE_QXL_MAJOR, SPICE_INTERFACE_QXL_MINOR);
> >          return;
> >      }
> > +
> > +#if SPICE_SERVER_VERSION >= 0x000e02 /* release 0.14.2 */
> > +    // register all possible device display IDs to SPICE server
> > +    for (int i = 0; i < qxl->max_outputs; ++i) {
> > +        spice_qxl_monitor_set_device_display_id(&qxl->ssd.qxl, i, i);
> > +    }
> > +#endif
> > +
> 
> I think would be better if this were the default in spice-server.

I don't know what you mean by this...

> >      qemu_add_vm_change_state_handler(qxl_vm_change_state_handler, qxl);
> >  
> >      qxl->update_irq = qemu_bh_new(qxl_update_irq_bh, qxl);
> > diff --git a/ui/spice-core.c b/ui/spice-core.c
> > index a4fbbc3898..2e01beea03 100644
> > --- a/ui/spice-core.c
> > +++ b/ui/spice-core.c
> > @@ -35,6 +35,8 @@
> >  #include "qemu/option.h"
> >  #include "migration/misc.h"
> >  #include "hw/hw.h"
> > +#include "hw/pci/pci.h"
> > +#include "hw/pci/pci_bus.h"
> >  #include "ui/spice-display.h"
> >  
> >  /* core bits */
> > @@ -871,6 +873,26 @@ bool qemu_spice_have_display_interface(QemuConsole *con)
> >      return false;
> >  }
> >  
> > +/*
> > + * Recursively (in reverse order) appends addresses of PCI devices as it
> > moves
> > + * up in the PCI hierarchy.
> > + *
> > + * @returns true on success, false when the buffer wasn't large enough
> > + */
> > +static bool append_pci_address_recursive(char *buf, size_t buf_size, const
> > PCIDevice *pci)
> 
> I won't use the "_recursive" in the name, looks like more an implementation
> detail.

Allright.

> > +{
> > +    PCIBus *bus = pci_get_bus(pci);
> > +    if (!pci_bus_is_root(bus)) {
> > +        append_pci_address_recursive(buf, buf_size, bus->parent_dev);
> > +    }
> > +
> > +    size_t len = strlen(buf);
> > +    size_t written = snprintf(buf + len, buf_size - len, "/%02x.%x",
> > +        PCI_SLOT(pci->devfn), PCI_FUNC(pci->devfn));
> > +
> > +    return written > 0 && written < buf_size - len;
> 
> written is always > 0, unless there's a bug in snprintf. Note that Qemu uses
> MingW snprintf on Windows so even on Windows this returns always > 0 (note
> that snprintf returns an int which is signed while size_t is unsigned and
> some implementation could return < 0, but not in Qemu).

Right. The man page says it returns negative values on errors. I've
missed the fact I'm storing the result in size_t. Do you think I should
drop the "written > 0" condition or change the type of "written" to
ssize_t for extra safety? :)

> > +}
> > +
> >  int qemu_spice_add_display_interface(QXLInstance *qxlin, QemuConsole *con)
> >  {
> >      if (g_slist_find(spice_consoles, con)) {
> > @@ -878,7 +900,28 @@ int qemu_spice_add_display_interface(QXLInstance *qxlin,
> > QemuConsole *con)
> >      }
> >      qxlin->id = qemu_console_get_index(con);
> >      spice_consoles = g_slist_append(spice_consoles, con);
> > -    return qemu_spice_add_interface(&qxlin->base);
> > +    int res = qemu_spice_add_interface(&qxlin->base);
> > +
> > +#if SPICE_SERVER_VERSION >= 0x000e02 /* release 0.14.2 */
> > +    DeviceState *dev = DEVICE(object_property_get_link(OBJECT(con),
> > "device", &error_abort));
> > +    PCIDevice *pci = (PCIDevice *) object_dynamic_cast(OBJECT(dev),
> > TYPE_PCI_DEVICE);
> > +
> > +    if (pci == NULL) {
> > +        warn_report("Setting PCI path of a display device to SPICE: Not a
> > PCI device.");
> > +        return res;
> > +    }
> > +
> > +    char device_path[128] = "pci/0000";  // hardcoded PCI domain 0000
> > +    if (!append_pci_address_recursive(device_path, sizeof(device_path),
> > pci)) {
> > +        warn_report("Setting PCI path of a display device to SPICE: "
> > +            "Too many PCI devices in the chain.");
> > +        return res;
> > +    }
> > +
> > +    spice_qxl_set_device_address(qxlin, device_path);
> > +#endif
> > +
> > +    return res;
> >  }
> >  
> >  static int qemu_spice_set_ticket(bool fail_if_conn, bool disconnect_if_conn)
> > diff --git a/ui/spice-display.c b/ui/spice-display.c
> > index 2f8adb6b9f..f620750803 100644
> > --- a/ui/spice-display.c
> > +++ b/ui/spice-display.c
> > @@ -1129,6 +1129,11 @@ static void qemu_spice_display_init_one(QemuConsole
> > *con)
> >  
> >      ssd->qxl.base.sif = &dpy_interface.base;
> >      qemu_spice_add_display_interface(&ssd->qxl, con);
> > +
> > +#if SPICE_SERVER_VERSION >= 0x000e02 /* release 0.14.2 */
> 
> Surely OT: won't be better to have something like
> 
> #if SPICE_SERVER_VERSION >= SPICE_SERVER_VERSION_NUMBER(0,14,2)

It surely would be better :)

Cheers,
Lukas

> > +    spice_qxl_monitor_set_device_display_id(&ssd->qxl, 0,
> > qemu_console_get_head(con));
> > +#endif
> > +
> >      qemu_spice_create_host_memslot(ssd);
> >  
> >      register_displaychangelistener(&ssd->dcl);
> 
> Frediano

Re: [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest

[Lukáš Hrázký]

Hello,

On Mon, 2018-10-22 at 13:46 +0200, Lukáš Hrázký wrote:
> Hello,
> 
> On Thu, 2018-10-18 at 03:16 -0400, Frediano Ziglio wrote:
> > > 
> > > Adds two functions to let QEMU provide information to identify graphics
> > > devices and their monitors in the guest:
> > > 
> > > * device address - The path identifying the device on the system (e.g. PCI
> > >   path):
> > >   spice_qxl_set_device_address(...)
> > > 
> > > * device display ID - The index of the monitor on the graphics device:
> > >   spice_qxl_monitor_set_device_display_id(...)
> > 
> > This seems to indicate that this device is bound in some way to the previous
> > information but having 2 APIs make more fragile, potentially one could
> > call a function and not the other or in different order or mismatch them.
> 
> Not sure what you mean by "previous information".
> 
> Yes, you need to call both functions... The order doesn't matter and
> you can always send wrong data in a single function as well. I agree in
> this regard a single function would be better but don't find it a big
> issue. Note that the functions are called in different places in QEMU
> too.
> 
> ...
> 
> > > +SPICE_GNUC_VISIBLE
> > > +void spice_qxl_set_device_address(QXLInstance *instance, const char
> > > *device_address)
> > > +{
> > > 
> > > ...
> > > 
> > > + */
> > > +SPICE_GNUC_VISIBLE
> > > +void spice_qxl_monitor_set_device_display_id(QXLInstance *instance,
> > > +                                             uint32_t monitor_id,
> > > +                                             uint32_t device_display_id)
> > 
> > I still don't understand why, as suggested by Gerd, we need another function
> > instead of 2 additional parameters to the above API specifying start and
> > number, this API looks much more prone to errors.
> 
> Possibly. I (and I think Jonathon too?) didn't like the start and
> number API, this was proposed as an alternative.

this topic of the API functions should be resolved. I think the options
are:


1. Keep the two functions proposed in this patch:

void spice_qxl_set_device_address(QXLInstance *instance,
                                  const char *device_address);

void spice_qxl_monitor_set_device_display_id(QXLInstance *instance,
                                             uint32_t monitor_id,
                                             uint32_t device_display_id);


2. Have a single function as follows:

void spice_qxl_set_device_info(QXLInstance *instance,
                               const char *device_address,
                               uint32_t device_display_id_start,
                               uint32_t device_display_id_count);


The semantics of the single function in 2. (with a sort-of placeholder
name) would be setting the device_address for the QXLInstance as well
as setting a mapping of monitor IDs to device display IDs of
[0, count-1] -> [start, start+count-1].

The usage of 1. in QEMU is what's in patch 2/2. The usage of 2. would
mean calling the new single function in the two places where
spice_qxl_monitor_set_device_display_id() is now called in patch 2/2.
The code that composes the device_address would need to be put in a
function somewhere in QEMU (not sure where yet) that can then be called
from the two places where it's needed.

I'm still somewhat more inclined to keep the two functions, as their
API seems a bit cleaner to me and you can call/not call a single
function in a wrong way too. Frediano prefers a single function.
Jonathon, Gerd?

Thanks,
Lukas

Re: [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest

[Gerd Hoffmann]

> 2. Have a single function as follows:
> 
> void spice_qxl_set_device_info(QXLInstance *instance,
>                                const char *device_address,
>                                uint32_t device_display_id_start,
>                                uint32_t device_display_id_count);

How about:

void spice_qxl_set_device_info(QXLInstance *instance,
                               const char *device_address,
                               uint32_t device_display_id);

I don't think we need start+count:

 * For single-head devices device_display_id will be zero.
 * For one-channel-per-head multihead devices (i.e. virtio-gpu)
   device_display_id will enumerate the heads (so everybody can figure
   which channel is which head).
 * For one-channel-per-device multihead devices (i.e. qxl/linux)
   device_display_id will be zero too.  Number of heads is set via
   spice_qxl_set_max_monitors().

cheers,
  Gerd

Re: [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest

[Frediano Ziglio]

> 
> > 2. Have a single function as follows:
> > 
> > void spice_qxl_set_device_info(QXLInstance *instance,
> >                                const char *device_address,
> >                                uint32_t device_display_id_start,
> >                                uint32_t device_display_id_count);
> 
> How about:
> 
> void spice_qxl_set_device_info(QXLInstance *instance,
>                                const char *device_address,
>                                uint32_t device_display_id);
> 
> I don't think we need start+count:
> 
>  * For single-head devices device_display_id will be zero.
>  * For one-channel-per-head multihead devices (i.e. virtio-gpu)
>    device_display_id will enumerate the heads (so everybody can figure
>    which channel is which head).
>  * For one-channel-per-device multihead devices (i.e. qxl/linux)
>    device_display_id will be zero too.  Number of heads is set via
>    spice_qxl_set_max_monitors().
> 
> cheers,
>   Gerd
> 

What about "Console VNC" case? Passing a dummy (like -1) value for device_display_id ?
Kind of "I don't know which output is."

Frediano

Re: [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest

[Lukáš Hrázký]

On Mon, 2018-11-05 at 03:46 -0500, Frediano Ziglio wrote:
> > 
> > > 2. Have a single function as follows:
> > > 
> > > void spice_qxl_set_device_info(QXLInstance *instance,
> > >                                const char *device_address,
> > >                                uint32_t device_display_id_start,
> > >                                uint32_t device_display_id_count);
> > 
> > How about:
> > 
> > void spice_qxl_set_device_info(QXLInstance *instance,
> >                                const char *device_address,
> >                                uint32_t device_display_id);
> > 
> > I don't think we need start+count:
> > 
> >  * For single-head devices device_display_id will be zero.
> >  * For one-channel-per-head multihead devices (i.e. virtio-gpu)
> >    device_display_id will enumerate the heads (so everybody can figure
> >    which channel is which head).
> >  * For one-channel-per-device multihead devices (i.e. qxl/linux)
> >    device_display_id will be zero too.  Number of heads is set via
> >    spice_qxl_set_max_monitors().
> > 
> > cheers,
> >   Gerd
> > 
> 
> What about "Console VNC" case? Passing a dummy (like -1) value for device_display_id ?
> Kind of "I don't know which output is."

I'm not sure -1 will be practically helpful for anything? What will
happen if we use 0 for it? We still aren't sure how we want to handle
the VNC console anyway, should we wait untill we know?

Maybe I should use int32_t instead of uint32_t to have the option to
use -1 if we need it later?

Cheers,
Lukas

> Frediano

Re: [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest

[Gerd Hoffmann]

On Mon, Nov 05, 2018 at 03:46:37AM -0500, Frediano Ziglio wrote:
> > 
> > > 2. Have a single function as follows:
> > > 
> > > void spice_qxl_set_device_info(QXLInstance *instance,
> > >                                const char *device_address,
> > >                                uint32_t device_display_id_start,
> > >                                uint32_t device_display_id_count);
> > 
> > How about:
> > 
> > void spice_qxl_set_device_info(QXLInstance *instance,
> >                                const char *device_address,
> >                                uint32_t device_display_id);
> > 
> > I don't think we need start+count:
> > 
> >  * For single-head devices device_display_id will be zero.
> >  * For one-channel-per-head multihead devices (i.e. virtio-gpu)
> >    device_display_id will enumerate the heads (so everybody can figure
> >    which channel is which head).
> >  * For one-channel-per-device multihead devices (i.e. qxl/linux)
> >    device_display_id will be zero too.  Number of heads is set via
> >    spice_qxl_set_max_monitors().
> > 
> > cheers,
> >   Gerd
> > 
> 
> What about "Console VNC" case? Passing a dummy (like -1) value for device_display_id ?
> Kind of "I don't know which output is."

I'd use zero, assuming in case only one of multiple heads shows up as
vnc console it'll be the primary head.

But using -1 kind-of makes sense too.

cheers,
  Gerd

Re: [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest

[Lukáš Hrázký]

On Mon, 2018-11-05 at 07:52 +0100, Gerd Hoffmann wrote:
> > 2. Have a single function as follows:
> > 
> > void spice_qxl_set_device_info(QXLInstance *instance,
> >                                const char *device_address,
> >                                uint32_t device_display_id_start,
> >                                uint32_t device_display_id_count);
> 
> How about:
> 
> void spice_qxl_set_device_info(QXLInstance *instance,
>                                const char *device_address,
>                                uint32_t device_display_id);
> 
> I don't think we need start+count:
> 
>  * For single-head devices device_display_id will be zero.
>  * For one-channel-per-head multihead devices (i.e. virtio-gpu)
>    device_display_id will enumerate the heads (so everybody can figure
>    which channel is which head).
>  * For one-channel-per-device multihead devices (i.e. qxl/linux)
>    device_display_id will be zero too.  Number of heads is set via
>    spice_qxl_set_max_monitors().

That requires nontrivial and unexpected logic for the one-channel-per-
device multihead devices case. The API should be doing what it says and
 the dumber the better, this seems too smart to me...

That said, I don't find it significantly worse than the other options
(none of which seems great), so I'd just like we reached some consesus
and be done with it...

Cheers,
Lukas

> cheers,
>   Gerd
> 

Re: [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest

[Frediano Ziglio]

> On Mon, 2018-11-05 at 07:52 +0100, Gerd Hoffmann wrote:
> > > 2. Have a single function as follows:
> > > 
> > > void spice_qxl_set_device_info(QXLInstance *instance,
> > >                                const char *device_address,
> > >                                uint32_t device_display_id_start,
> > >                                uint32_t device_display_id_count);
> > 
> > How about:
> > 
> > void spice_qxl_set_device_info(QXLInstance *instance,
> >                                const char *device_address,
> >                                uint32_t device_display_id);
> > 
> > I don't think we need start+count:
> > 
> >  * For single-head devices device_display_id will be zero.
> >  * For one-channel-per-head multihead devices (i.e. virtio-gpu)
> >    device_display_id will enumerate the heads (so everybody can figure
> >    which channel is which head).
> >  * For one-channel-per-device multihead devices (i.e. qxl/linux)
> >    device_display_id will be zero too.  Number of heads is set via
> >    spice_qxl_set_max_monitors().
> 
> That requires nontrivial and unexpected logic for the one-channel-per-
> device multihead devices case. The API should be doing what it says and
>  the dumber the better, this seems too smart to me...
> 
> That said, I don't find it significantly worse than the other options
> (none of which seems great), so I'd just like we reached some consesus
> and be done with it...
> 
> Cheers,
> Lukas
> 
> > cheers,
> >   Gerd
> > 

Personally I prefer the single function API, either with or without count.

About the not trivial for compatibility you'll have to support Qemu versions
that either:
- does not call new APIs or spice_qxl_set_max_monitors. A QXL card will have
  the maximum possible number of monitors in the guest;
- does not call new APIs but call spice_qxl_set_max_monitors. A QXL card will
  have a maximum of monitor specified by spice_qxl_set_max_monitors in the
  guest;
- call new APIs (the choice in this case to call spice_qxl_set_max_monitors
  or not is yours).

Frediano

Re: [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest

[Gerd Hoffmann]

On Mon, Nov 05, 2018 at 01:18:57PM +0100, Lukáš Hrázký wrote:
> On Mon, 2018-11-05 at 07:52 +0100, Gerd Hoffmann wrote:
> > > 2. Have a single function as follows:
> > > 
> > > void spice_qxl_set_device_info(QXLInstance *instance,
> > >                                const char *device_address,
> > >                                uint32_t device_display_id_start,
> > >                                uint32_t device_display_id_count);
> > 
> > How about:
> > 
> > void spice_qxl_set_device_info(QXLInstance *instance,
> >                                const char *device_address,
> >                                uint32_t device_display_id);
> > 
> > I don't think we need start+count:
> > 
> >  * For single-head devices device_display_id will be zero.
> >  * For one-channel-per-head multihead devices (i.e. virtio-gpu)
> >    device_display_id will enumerate the heads (so everybody can figure
> >    which channel is which head).
> >  * For one-channel-per-device multihead devices (i.e. qxl/linux)
> >    device_display_id will be zero too.  Number of heads is set via
> >    spice_qxl_set_max_monitors().
> 
> That requires nontrivial and unexpected logic for the one-channel-per-
> device multihead devices case. The API should be doing what it says and
>  the dumber the better, this seems too smart to me...

Well, the device_display_id_count argument is redundant with
spice_qxl_set_max_monitors().  That isn't a great API either.

I can see that it simplifies the logic in spice-server if we have a
single function call instead of two.  So we could deprecate
spice_qxl_set_max_monitors() in favour of your
spice_qxl_set_device_info() variant.

spice_qxl_set_max_monitors() would then basically do this:

    spice_qxl_set_max_monitors(qxl, max)
    {
        spice_qxl_set_device_info(qxl, NULL, 0, max);
    }

cheers,
  Gerd

Re: [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest

[Lukáš Hrázký]

On Mon, 2018-11-05 at 14:08 +0100, Gerd Hoffmann wrote:
> On Mon, Nov 05, 2018 at 01:18:57PM +0100, Lukáš Hrázký wrote:
> > On Mon, 2018-11-05 at 07:52 +0100, Gerd Hoffmann wrote:
> > > > 2. Have a single function as follows:
> > > > 
> > > > void spice_qxl_set_device_info(QXLInstance *instance,
> > > >                                const char *device_address,
> > > >                                uint32_t device_display_id_start,
> > > >                                uint32_t device_display_id_count);
> > > 
> > > How about:
> > > 
> > > void spice_qxl_set_device_info(QXLInstance *instance,
> > >                                const char *device_address,
> > >                                uint32_t device_display_id);
> > > 
> > > I don't think we need start+count:
> > > 
> > >  * For single-head devices device_display_id will be zero.
> > >  * For one-channel-per-head multihead devices (i.e. virtio-gpu)
> > >    device_display_id will enumerate the heads (so everybody can figure
> > >    which channel is which head).
> > >  * For one-channel-per-device multihead devices (i.e. qxl/linux)
> > >    device_display_id will be zero too.  Number of heads is set via
> > >    spice_qxl_set_max_monitors().
> > 
> > That requires nontrivial and unexpected logic for the one-channel-per-
> > device multihead devices case. The API should be doing what it says and
> >  the dumber the better, this seems too smart to me...
> 
> Well, the device_display_id_count argument is redundant with
> spice_qxl_set_max_monitors().  That isn't a great API either.
> 
> I can see that it simplifies the logic in spice-server if we have a
> single function call instead of two.  So we could deprecate
> spice_qxl_set_max_monitors() in favour of your
> spice_qxl_set_device_info() variant.
> 
> spice_qxl_set_max_monitors() would then basically do this:
> 
>     spice_qxl_set_max_monitors(qxl, max)
>     {
>         spice_qxl_set_device_info(qxl, NULL, 0, max);
>     }

I can't actually do this, it does the wrong thing for the one-channel-
per-head (virtio-gpu) case. For that case it would send all
device_display_ids to 0 on all interfaces, but they need to be
different numbers.

For backwards compatibility when spice_qxl_set_device_info() is not
called I think we need to fallback to the old behaviour, as we are
missing the necessary information.

I'll send v3 with a single function using start + count and deprecate
the set_max_monitors one.

Cheers,
Lukas

> cheers,
>   Gerd
> 

Re: [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest

[Frediano Ziglio]

> 
> On Mon, 2018-11-05 at 14:08 +0100, Gerd Hoffmann wrote:
> > On Mon, Nov 05, 2018 at 01:18:57PM +0100, Lukáš Hrázký wrote:
> > > On Mon, 2018-11-05 at 07:52 +0100, Gerd Hoffmann wrote:
> > > > > 2. Have a single function as follows:
> > > > > 
> > > > > void spice_qxl_set_device_info(QXLInstance *instance,
> > > > >                                const char *device_address,
> > > > >                                uint32_t device_display_id_start,
> > > > >                                uint32_t device_display_id_count);
> > > > 
> > > > How about:
> > > > 
> > > > void spice_qxl_set_device_info(QXLInstance *instance,
> > > >                                const char *device_address,
> > > >                                uint32_t device_display_id);
> > > > 
> > > > I don't think we need start+count:
> > > > 
> > > >  * For single-head devices device_display_id will be zero.
> > > >  * For one-channel-per-head multihead devices (i.e. virtio-gpu)
> > > >    device_display_id will enumerate the heads (so everybody can figure
> > > >    which channel is which head).
> > > >  * For one-channel-per-device multihead devices (i.e. qxl/linux)
> > > >    device_display_id will be zero too.  Number of heads is set via
> > > >    spice_qxl_set_max_monitors().
> > > 
> > > That requires nontrivial and unexpected logic for the one-channel-per-
> > > device multihead devices case. The API should be doing what it says and
> > >  the dumber the better, this seems too smart to me...
> > 
> > Well, the device_display_id_count argument is redundant with
> > spice_qxl_set_max_monitors().  That isn't a great API either.
> > 
> > I can see that it simplifies the logic in spice-server if we have a
> > single function call instead of two.  So we could deprecate
> > spice_qxl_set_max_monitors() in favour of your
> > spice_qxl_set_device_info() variant.
> > 
> > spice_qxl_set_max_monitors() would then basically do this:
> > 
> >     spice_qxl_set_max_monitors(qxl, max)
> >     {
> >         spice_qxl_set_device_info(qxl, NULL, 0, max);
> >     }
> 
> I can't actually do this, it does the wrong thing for the one-channel-
> per-head (virtio-gpu) case. For that case it would send all
> device_display_ids to 0 on all interfaces, but they need to be
> different numbers.
> 

This function is only called for QXL cards devices so no virtio-gpu,
no cirrus, no svga or anything else.

> For backwards compatibility when spice_qxl_set_device_info() is not
> called I think we need to fallback to the old behaviour, as we are
> missing the necessary information.
> 
> I'll send v3 with a single function using start + count and deprecate
> the set_max_monitors one.
> 
> Cheers,
> Lukas
> 
> > cheers,
> >   Gerd
> > 
> 

Frediano

Re: [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest

[Gerd Hoffmann]

  Hi,

> > I can see that it simplifies the logic in spice-server if we have a
> > single function call instead of two.  So we could deprecate
> > spice_qxl_set_max_monitors() in favour of your
> > spice_qxl_set_device_info() variant.
> > 
> > spice_qxl_set_max_monitors() would then basically do this:
> > 
> >     spice_qxl_set_max_monitors(qxl, max)
> >     {
> >         spice_qxl_set_device_info(qxl, NULL, 0, max);
> >     }
> 
> I can't actually do this, it does the wrong thing for the one-channel-
> per-head (virtio-gpu) case. For that case it would send all
> device_display_ids to 0 on all interfaces, but they need to be
> different numbers.

Well, qemu calls spice_qxl_set_max_monitors() only in case there are
multiple monitors in one channel, i.e. qxl.

cheers,
  Gerd