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

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

[Lukáš Hrázký]

Hello,

this is the final version of the new QXL interface to identify the
graphics device monitors in the guest. This interface adds a function
that allows to set the device path (e.g. a PCI path) and the IDs of the
device's displays from QEMU to SPICE server. The server will forward
this information to the guest agent, which can use it to identify the
displays in the guest context (a follow-up series on the spice-devel
mailing list).

-- 
2.20.1

[Qemu-devel] [PATCH spice 1/3] QXL interface: add a function to identify monitors in the guest

[Lukáš Hrázký]

Adds a function to let QEMU provide information to identify graphics
devices and their monitors in the guest. The function
(spice_qxl_set_device_info) sets the device address (e.g. a PCI path)
and monitor ID -> device display ID mapping of displays exposed by given
QXL interface.

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

diff --git a/server/red-qxl.c b/server/red-qxl.c
index 97940611..0ea424cd 100644
--- a/server/red-qxl.c
+++ b/server/red-qxl.c
@@ -41,6 +41,9 @@
 #include "red-qxl.h"
 
 
+#define MAX_DEVICE_ADDRESS_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_DEVICE_ADDRESS_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,44 @@ void red_qxl_gl_draw_async_complete(QXLInstance *qxl)
     red_qxl_async_complete(qxl, cookie);
 }
 
+SPICE_GNUC_VISIBLE
+void spice_qxl_set_device_info(QXLInstance *instance,
+                               const char *device_address,
+                               uint32_t device_display_id_start,
+                               uint32_t device_display_id_count)
+{
+    g_return_if_fail(device_address != NULL);
+
+    size_t da_len = strnlen(device_address, MAX_DEVICE_ADDRESS_LEN);
+    if (da_len >= MAX_DEVICE_ADDRESS_LEN) {
+        spice_error("Device address too long: %lu > %u", da_len, MAX_DEVICE_ADDRESS_LEN);
+        return;
+    }
+
+    if (device_display_id_count > MAX_MONITORS_COUNT) {
+        spice_error("Device display ID count (%u) is greater than limit %u",
+                    device_display_id_count,
+                    MAX_MONITORS_COUNT);
+        return;
+    }
+
+    strncpy(instance->st->device_address, device_address, MAX_DEVICE_ADDRESS_LEN);
+
+    g_debug("QXL Instance %d setting device address \"%s\" and monitor -> device display mapping:",
+            instance->id,
+            device_address);
+
+    // store the mapping monitor_id -> device_display_id
+    for (uint32_t monitor_id = 0; monitor_id < device_display_id_count; ++monitor_id) {
+        uint32_t device_display_id = device_display_id_start + monitor_id;
+        instance->st->device_display_ids[monitor_id] = device_display_id;
+        g_debug("   monitor ID %u -> device display ID %u",
+                monitor_id, device_display_id);
+    }
+
+    instance->st->monitors_count = device_display_id_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..547d3d93 100644
--- a/server/spice-qxl.h
+++ b/server/spice-qxl.h
@@ -115,6 +115,52 @@ void spice_qxl_gl_draw_async(QXLInstance *instance,
                              uint32_t w, uint32_t h,
                              uint64_t cookie);
 
+/* since spice 0.14.2 */
+
+/**
+ * spice_qxl_set_device_info:
+ * @instance the QXL instance to set the path to
+ * @device_address the path of the device this QXL instance belongs to
+ * @device_display_id_start the starting device display ID of this interface,
+ *                          i.e. the one monitor ID 0 maps to
+ * @device_display_id_count the total number of device display IDs on this
+ *                          interface
+ *
+ * Sets the device information for this QXL interface, i.e. the hardware
+ * address (e.g. PCI) of the graphics device and the IDs of the displays of the
+ * graphics device that are exposed by this interface (device display IDs).
+ *
+ * The supported device address format is:
+ * "pci/<DOMAIN>/<SLOT>.<FUNCTION>/.../<SLOT>.<FUNCTION>"
+ *
+ * The "pci" identifies the rest of the string as a PCI address. 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. All of <DOMAIN>, <SLOT>, <FUNCTION> are hexadecimal numbers
+ * with the following number of digits:
+ *   <DOMAIN>: 4
+ *   <SLOT>: 2
+ *   <FUNCTION>: 1
+ *
+ * The device_display_id_{start,count} denotes the sequence of device display
+ * IDs that map to the zero-based sequence of monitor IDs provided by monitors
+ * config on this interface. For example with device_display_id_start = 2 and
+ * device_display_id_count = 3 you get the following mapping:
+ * monitor_id  ->  device_display_id
+ *          0  ->  2
+ *          1  ->  3
+ *          2  ->  4
+ *
+ * Note this example is unsupported in practice. The only supported cases are
+ * either a single device display ID (count = 1) or multiple device display IDs
+ * in a sequence starting from 0.
+ */
+void spice_qxl_set_device_info(QXLInstance *instance,
+                               const char *device_address,
+                               uint32_t device_display_id_start,
+                               uint32_t device_display_id_count);
+
 typedef struct QXLDevInitInfo {
     uint32_t num_memslots_groups;
     uint32_t num_memslots;
diff --git a/server/spice-server.syms b/server/spice-server.syms
index edf04a42..ac4d9b14 100644
--- a/server/spice-server.syms
+++ b/server/spice-server.syms
@@ -173,3 +173,8 @@ 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_info;
+} SPICE_SERVER_0.13.2;
-- 
2.20.1

[Qemu-devel] [PATCH spice 2/3] QXL interface: deprecate spice_qxl_set_max_monitors

[Lukáš Hrázký]

Replace it by spice_qxl_set_device_info. Note we can't use
monitors_count for what's stored in max_monitors, because monitors_count
denotes the length of the device_display_ids array, which
spice_qxl_set_max_monitors doesn't touch.

Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
---
 server/red-qxl.c   | 1 +
 server/spice-qxl.h | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/server/red-qxl.c b/server/red-qxl.c
index 0ea424cd..6ffd8286 100644
--- a/server/red-qxl.c
+++ b/server/red-qxl.c
@@ -888,6 +888,7 @@ void spice_qxl_set_device_info(QXLInstance *instance,
     }
 
     instance->st->monitors_count = device_display_id_count;
+    instance->st->max_monitors = device_display_id_count;
 }
 
 void red_qxl_init(RedsState *reds, QXLInstance *qxl)
diff --git a/server/spice-qxl.h b/server/spice-qxl.h
index 547d3d93..e7af5e5e 100644
--- a/server/spice-qxl.h
+++ b/server/spice-qxl.h
@@ -101,9 +101,9 @@ void spice_qxl_monitors_config_async(QXLInstance *instance, QXLPHYSICAL monitors
                                      int group_id, uint64_t cookie);
 /* since spice 0.12.3 */
 void spice_qxl_driver_unload(QXLInstance *instance);
-/* since spice 0.12.6 */
+/* since spice 0.12.6, deprecated since 0.14.2, spice_qxl_set_device_info replaces it */
 void spice_qxl_set_max_monitors(QXLInstance *instance,
-                                unsigned int max_monitors);
+                                unsigned int max_monitors) SPICE_GNUC_DEPRECATED;
 /* since spice 0.13.1 */
 void spice_qxl_gl_scanout(QXLInstance *instance,
                           int fd,
-- 
2.20.1

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

[Lukáš Hrázký]

Calls the new SPICE QXL interface function spice_qxl_set_device_info to
set the hardware address of the graphics device represented by the QXL
interface (e.g. a PCI path) and the device display IDs (the IDs of the
device's monitors that belong to this QXL interface).

Also stops using the deprecated spice_qxl_set_max_monitors, the new
interface function replaces it.

Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
---
 hw/display/qxl.c           | 14 ++++++++++++-
 include/ui/spice-display.h |  2 ++
 ui/spice-core.c            | 42 ++++++++++++++++++++++++++++++++++++++
 ui/spice-display.c         | 11 ++++++++++
 4 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 9087db5dee..b1c9e856f5 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -276,7 +276,8 @@ static void qxl_spice_monitors_config_async(PCIQXLDevice *qxl, int replay)
                     QXL_COOKIE_TYPE_POST_LOAD_MONITORS_CONFIG,
                     0));
     } else {
-#if SPICE_SERVER_VERSION >= 0x000c06 /* release 0.12.6 */
+#if SPICE_SERVER_VERSION >= 0x000c06 /* release 0.12.6 */ && \
+    SPICE_SERVER_VERSION < 0x000e02
         if (qxl->max_outputs) {
             spice_qxl_set_max_monitors(&qxl->ssd.qxl, qxl->max_outputs);
         }
@@ -2184,6 +2185,17 @@ 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 */
+    char device_address[256] = "";
+    if (qemu_spice_fill_device_address(qxl->vga.con, device_address, 256)) {
+        spice_qxl_set_device_info(&qxl->ssd.qxl,
+                                  device_address,
+                                  0,
+                                  qxl->max_outputs);
+    }
+#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/include/ui/spice-display.h b/include/ui/spice-display.h
index 87a84a59d4..7608fa7ebd 100644
--- a/include/ui/spice-display.h
+++ b/include/ui/spice-display.h
@@ -179,3 +179,5 @@ void qemu_spice_wakeup(SimpleSpiceDisplay *ssd);
 void qemu_spice_display_start(void);
 void qemu_spice_display_stop(void);
 int qemu_spice_display_is_running(SimpleSpiceDisplay *ssd);
+
+bool qemu_spice_fill_device_address(QemuConsole *con, char *device_address, size_t size);
diff --git a/ui/spice-core.c b/ui/spice-core.c
index ebaae24643..43a7435cd9 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 */
@@ -872,6 +874,46 @@ 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(char *buf, size_t buf_size, const PCIDevice *pci)
+{
+    PCIBus *bus = pci_get_bus(pci);
+    if (!pci_bus_is_root(bus)) {
+        append_pci_address(buf, buf_size, bus->parent_dev);
+    }
+
+    size_t len = strlen(buf);
+    ssize_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;
+}
+
+bool qemu_spice_fill_device_address(QemuConsole *con, char *device_address, size_t size)
+{
+    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 device address of a display device to SPICE: Not a PCI device.");
+        return false;
+    }
+
+    strncpy(device_address, "pci/0000", size);
+    if (!append_pci_address(device_address, size, pci)) {
+        warn_report("Setting device address of a display device to SPICE: "
+            "Too many PCI devices in the chain.");
+        return false;
+    }
+
+    return true;
+}
+
 int qemu_spice_add_display_interface(QXLInstance *qxlin, QemuConsole *con)
 {
     if (g_slist_find(spice_consoles, con)) {
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 52f8cb5ae1..c1605b3bc9 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -1147,6 +1147,17 @@ 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 */
+    char device_address[256] = "";
+    if (qemu_spice_fill_device_address(con, device_address, 256)) {
+        spice_qxl_set_device_info(&ssd->qxl,
+                                  device_address,
+                                  qemu_console_get_head(con),
+                                  1);
+    }
+#endif
+
     qemu_spice_create_host_memslot(ssd);
 
     register_displaychangelistener(&ssd->dcl);
-- 
2.20.1

Re: [Qemu-devel] [Spice-devel] [PATCH spice 1/3] QXL interface: add a function to identify monitors in the guest

[Jonathon Jongsma]

On Tue, 2019-01-08 at 16:26 +0100, Lukáš Hrázký wrote:
> Adds a function to let QEMU provide information to identify graphics
> devices and their monitors in the guest. The function
> (spice_qxl_set_device_info) sets the device address (e.g. a PCI path)
> and monitor ID -> device display ID mapping of displays exposed by
> given
> QXL interface.


In my previous review, I asked for a slightly more explicit explanation
of the phrase "monitor ID" in the commit log. In this case, "monitor
ID" is an ID specific to the QXL instance starting at 0 and represents
the displays that are associated with that particular QXL instance.

Perhaps it makes sense to rename this "Qxl monitor ID"?

> 
> Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
> ---
>  server/red-qxl.c         | 44 ++++++++++++++++++++++++++++++++++++++
>  server/spice-qxl.h       | 46
> ++++++++++++++++++++++++++++++++++++++++
>  server/spice-server.syms |  5 +++++
>  3 files changed, 95 insertions(+)
> 
> diff --git a/server/red-qxl.c b/server/red-qxl.c
> index 97940611..0ea424cd 100644
> --- a/server/red-qxl.c
> +++ b/server/red-qxl.c
> @@ -41,6 +41,9 @@
>  #include "red-qxl.h"
>  
>  
> +#define MAX_DEVICE_ADDRESS_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_DEVICE_ADDRESS_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,44 @@ void red_qxl_gl_draw_async_complete(QXLInstance
> *qxl)
>      red_qxl_async_complete(qxl, cookie);
>  }
>  
> +SPICE_GNUC_VISIBLE
> +void spice_qxl_set_device_info(QXLInstance *instance,
> +                               const char *device_address,
> +                               uint32_t device_display_id_start,
> +                               uint32_t device_display_id_count)
> +{
> +    g_return_if_fail(device_address != NULL);

Just a thought: what if qemu calls this function twice for the same
instance. In theory this shouldn't happen, but should we be defensive
and handle it somehow? Print a warning? Just overwrite the previous
mapping? Right now it looks like we would just overwrite.

> +
> +    size_t da_len = strnlen(device_address, MAX_DEVICE_ADDRESS_LEN);
> +    if (da_len >= MAX_DEVICE_ADDRESS_LEN) {
> +        spice_error("Device address too long: %lu > %u", da_len,
> MAX_DEVICE_ADDRESS_LEN);

Technically, I think the format string for a size_t variable is %zu,
not %lu

> +        return;
> +    }
> +
> +    if (device_display_id_count > MAX_MONITORS_COUNT) {
> +        spice_error("Device display ID count (%u) is greater than
> limit %u",
> +                    device_display_id_count,
> +                    MAX_MONITORS_COUNT);
> +        return;
> +    }
> +
> +    strncpy(instance->st->device_address, device_address,
> MAX_DEVICE_ADDRESS_LEN);
> +
> +    g_debug("QXL Instance %d setting device address \"%s\" and
> monitor -> device display mapping:",
> +            instance->id,
> +            device_address);
> +
> +    // store the mapping monitor_id -> device_display_id
> +    for (uint32_t monitor_id = 0; monitor_id <
> device_display_id_count; ++monitor_id) {
> +        uint32_t device_display_id = device_display_id_start +
> monitor_id;
> +        instance->st->device_display_ids[monitor_id] =
> device_display_id;
> +        g_debug("   monitor ID %u -> device display ID %u",
> +                monitor_id, device_display_id);
> +    }
> +
> +    instance->st->monitors_count = device_display_id_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..547d3d93 100644
> --- a/server/spice-qxl.h
> +++ b/server/spice-qxl.h
> @@ -115,6 +115,52 @@ void spice_qxl_gl_draw_async(QXLInstance
> *instance,
>                               uint32_t w, uint32_t h,
>                               uint64_t cookie);
>  
> +/* since spice 0.14.2 */
> +
> +/**
> + * spice_qxl_set_device_info:
> + * @instance the QXL instance to set the path to
> + * @device_address the path of the device this QXL instance belongs
> to
> + * @device_display_id_start the starting device display ID of this
> interface,
> + *                          i.e. the one monitor ID 0 maps to

perhaps reword this last line:
i.e. the device display ID of monitor ID 0

> + * @device_display_id_count the total number of device display IDs
> on this
> + *                          interface
> + *
> + * Sets the device information for this QXL interface, i.e. the
> hardware
> + * address (e.g. PCI) of the graphics device and the IDs of the
> displays of the
> + * graphics device that are exposed by this interface (device
> display IDs).
> + *
> + * The supported device address format is:
> + * "pci/<DOMAIN>/<SLOT>.<FUNCTION>/.../<SLOT>.<FUNCTION>"
> + *
> + * The "pci" identifies the rest of the string as a PCI address. 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. All of <DOMAIN>, <SLOT>, <FUNCTION> are
> hexadecimal numbers
> + * with the following number of digits:
> + *   <DOMAIN>: 4
> + *   <SLOT>: 2
> + *   <FUNCTION>: 1
> + *
> + * The device_display_id_{start,count} denotes the sequence of
> device display
> + * IDs that map to the zero-based sequence of monitor IDs provided
> by monitors
> + * config on this interface. For example with
> device_display_id_start = 2 and
> + * device_display_id_count = 3 you get the following mapping:
> + * monitor_id  ->  device_display_id
> + *          0  ->  2
> + *          1  ->  3
> + *          2  ->  4
> + *
> + * Note this example is unsupported in practice. The only supported
> cases are
> + * either a single device display ID (count = 1) or multiple device
> display IDs
> + * in a sequence starting from 0.
> + */

Gerd previously objected to using an example of an unsupported
scenario. But it doesn't seem that you changed anything here. I don't
care too much, myself.

> +void spice_qxl_set_device_info(QXLInstance *instance,
> +                               const char *device_address,
> +                               uint32_t device_display_id_start,
> +                               uint32_t device_display_id_count);
> +
>  typedef struct QXLDevInitInfo {
>      uint32_t num_memslots_groups;
>      uint32_t num_memslots;
> diff --git a/server/spice-server.syms b/server/spice-server.syms
> index edf04a42..ac4d9b14 100644
> --- a/server/spice-server.syms
> +++ b/server/spice-server.syms
> @@ -173,3 +173,8 @@ 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_info;
> +} SPICE_SERVER_0.13.2;

Re: [Qemu-devel] [Spice-devel] [PATCH spice 2/3] QXL interface: deprecate spice_qxl_set_max_monitors

[Jonathon Jongsma]

Acked-by: Jonathon Jongsma <jjongsma@redhat.com>


On Tue, 2019-01-08 at 16:26 +0100, Lukáš Hrázký wrote:
> Replace it by spice_qxl_set_device_info. Note we can't use
> monitors_count for what's stored in max_monitors, because
> monitors_count
> denotes the length of the device_display_ids array, which
> spice_qxl_set_max_monitors doesn't touch.
> 
> Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
> ---
>  server/red-qxl.c   | 1 +
>  server/spice-qxl.h | 4 ++--
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/server/red-qxl.c b/server/red-qxl.c
> index 0ea424cd..6ffd8286 100644
> --- a/server/red-qxl.c
> +++ b/server/red-qxl.c
> @@ -888,6 +888,7 @@ void spice_qxl_set_device_info(QXLInstance
> *instance,
>      }
>  
>      instance->st->monitors_count = device_display_id_count;
> +    instance->st->max_monitors = device_display_id_count;
>  }
>  
>  void red_qxl_init(RedsState *reds, QXLInstance *qxl)
> diff --git a/server/spice-qxl.h b/server/spice-qxl.h
> index 547d3d93..e7af5e5e 100644
> --- a/server/spice-qxl.h
> +++ b/server/spice-qxl.h
> @@ -101,9 +101,9 @@ void spice_qxl_monitors_config_async(QXLInstance
> *instance, QXLPHYSICAL monitors
>                                       int group_id, uint64_t cookie);
>  /* since spice 0.12.3 */
>  void spice_qxl_driver_unload(QXLInstance *instance);
> -/* since spice 0.12.6 */
> +/* since spice 0.12.6, deprecated since 0.14.2,
> spice_qxl_set_device_info replaces it */
>  void spice_qxl_set_max_monitors(QXLInstance *instance,
> -                                unsigned int max_monitors);
> +                                unsigned int max_monitors)
> SPICE_GNUC_DEPRECATED;
>  /* since spice 0.13.1 */
>  void spice_qxl_gl_scanout(QXLInstance *instance,
>                            int fd,

Re: [Qemu-devel] [Spice-devel] [PATCH spice 1/3] QXL interface: add a function to identify monitors in the guest

[Lukáš Hrázký]

On Wed, 2019-01-09 at 11:36 -0600, Jonathon Jongsma wrote:
> On Tue, 2019-01-08 at 16:26 +0100, Lukáš Hrázký wrote:
> > Adds a function to let QEMU provide information to identify graphics
> > devices and their monitors in the guest. The function
> > (spice_qxl_set_device_info) sets the device address (e.g. a PCI path)
> > and monitor ID -> device display ID mapping of displays exposed by
> > given
> > QXL interface.
> 
> 
> In my previous review, I asked for a slightly more explicit explanation
> of the phrase "monitor ID" in the commit log. In this case, "monitor
> ID" is an ID specific to the QXL instance starting at 0 and represents
> the displays that are associated with that particular QXL instance.
> 
> Perhaps it makes sense to rename this "Qxl monitor ID"?

Sorry, I might have forgotten to incorporate some comments on the old
series :/

To reiterate, though, what do you find unclear about the "monitor ID"?
I mean maybe it should be spelled "monitor_id" to make it clearer what
it refers to... Not sure if "Qxl monitor ID" helps, since it is not
strictly related to QXL, but more to the QXL interface (which is used
for all graphic cards, which makes it more confusing)...

> > Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
> > ---
> >  server/red-qxl.c         | 44 ++++++++++++++++++++++++++++++++++++++
> >  server/spice-qxl.h       | 46
> > ++++++++++++++++++++++++++++++++++++++++
> >  server/spice-server.syms |  5 +++++
> >  3 files changed, 95 insertions(+)
> > 
> > diff --git a/server/red-qxl.c b/server/red-qxl.c
> > index 97940611..0ea424cd 100644
> > --- a/server/red-qxl.c
> > +++ b/server/red-qxl.c
> > @@ -41,6 +41,9 @@
> >  #include "red-qxl.h"
> >  
> >  
> > +#define MAX_DEVICE_ADDRESS_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_DEVICE_ADDRESS_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,44 @@ void red_qxl_gl_draw_async_complete(QXLInstance
> > *qxl)
> >      red_qxl_async_complete(qxl, cookie);
> >  }
> >  
> > +SPICE_GNUC_VISIBLE
> > +void spice_qxl_set_device_info(QXLInstance *instance,
> > +                               const char *device_address,
> > +                               uint32_t device_display_id_start,
> > +                               uint32_t device_display_id_count)
> > +{
> > +    g_return_if_fail(device_address != NULL);
> 
> Just a thought: what if qemu calls this function twice for the same
> instance. In theory this shouldn't happen, but should we be defensive
> and handle it somehow? Print a warning? Just overwrite the previous
> mapping? Right now it looks like we would just overwrite.

I'm pretty sure overwriting is the correct behavior here, no logging
needed? There may be a situation where the info changes in the future,
for whatever reasons. At least this part of the code should be fine
with it (though I'm not sure atm. if e.g. the client would cope well).

> > +
> > +    size_t da_len = strnlen(device_address, MAX_DEVICE_ADDRESS_LEN);
> > +    if (da_len >= MAX_DEVICE_ADDRESS_LEN) {
> > +        spice_error("Device address too long: %lu > %u", da_len,
> > MAX_DEVICE_ADDRESS_LEN);
> 
> Technically, I think the format string for a size_t variable is %zu,
> not %lu

You're right, didn't know that. I think I have some fixing to do :)

> > +        return;
> > +    }
> > +
> > +    if (device_display_id_count > MAX_MONITORS_COUNT) {
> > +        spice_error("Device display ID count (%u) is greater than
> > limit %u",
> > +                    device_display_id_count,
> > +                    MAX_MONITORS_COUNT);
> > +        return;
> > +    }
> > +
> > +    strncpy(instance->st->device_address, device_address,
> > MAX_DEVICE_ADDRESS_LEN);
> > +
> > +    g_debug("QXL Instance %d setting device address \"%s\" and
> > monitor -> device display mapping:",
> > +            instance->id,
> > +            device_address);
> > +
> > +    // store the mapping monitor_id -> device_display_id
> > +    for (uint32_t monitor_id = 0; monitor_id <
> > device_display_id_count; ++monitor_id) {
> > +        uint32_t device_display_id = device_display_id_start +
> > monitor_id;
> > +        instance->st->device_display_ids[monitor_id] =
> > device_display_id;
> > +        g_debug("   monitor ID %u -> device display ID %u",
> > +                monitor_id, device_display_id);
> > +    }
> > +
> > +    instance->st->monitors_count = device_display_id_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..547d3d93 100644
> > --- a/server/spice-qxl.h
> > +++ b/server/spice-qxl.h
> > @@ -115,6 +115,52 @@ void spice_qxl_gl_draw_async(QXLInstance
> > *instance,
> >                               uint32_t w, uint32_t h,
> >                               uint64_t cookie);
> >  
> > +/* since spice 0.14.2 */
> > +
> > +/**
> > + * spice_qxl_set_device_info:
> > + * @instance the QXL instance to set the path to
> > + * @device_address the path of the device this QXL instance belongs
> > to
> > + * @device_display_id_start the starting device display ID of this
> > interface,
> > + *                          i.e. the one monitor ID 0 maps to
> 
> perhaps reword this last line:
> i.e. the device display ID of monitor ID 0

Will do.

> > + * @device_display_id_count the total number of device display IDs
> > on this
> > + *                          interface
> > + *
> > + * Sets the device information for this QXL interface, i.e. the
> > hardware
> > + * address (e.g. PCI) of the graphics device and the IDs of the
> > displays of the
> > + * graphics device that are exposed by this interface (device
> > display IDs).
> > + *
> > + * The supported device address format is:
> > + * "pci/<DOMAIN>/<SLOT>.<FUNCTION>/.../<SLOT>.<FUNCTION>"
> > + *
> > + * The "pci" identifies the rest of the string as a PCI address. 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. All of <DOMAIN>, <SLOT>, <FUNCTION> are
> > hexadecimal numbers
> > + * with the following number of digits:
> > + *   <DOMAIN>: 4
> > + *   <SLOT>: 2
> > + *   <FUNCTION>: 1
> > + *
> > + * The device_display_id_{start,count} denotes the sequence of
> > device display
> > + * IDs that map to the zero-based sequence of monitor IDs provided
> > by monitors
> > + * config on this interface. For example with
> > device_display_id_start = 2 and
> > + * device_display_id_count = 3 you get the following mapping:
> > + * monitor_id  ->  device_display_id
> > + *          0  ->  2
> > + *          1  ->  3
> > + *          2  ->  4
> > + *
> > + * Note this example is unsupported in practice. The only supported
> > cases are
> > + * either a single device display ID (count = 1) or multiple device
> > display IDs
> > + * in a sequence starting from 0.
> > + */
> 
> Gerd previously objected to using an example of an unsupported
> scenario. But it doesn't seem that you changed anything here. I don't
> care too much, myself.

Right, forgot about that.

Thanks,
Lukas

> > +void spice_qxl_set_device_info(QXLInstance *instance,
> > +                               const char *device_address,
> > +                               uint32_t device_display_id_start,
> > +                               uint32_t device_display_id_count);
> > +
> >  typedef struct QXLDevInitInfo {
> >      uint32_t num_memslots_groups;
> >      uint32_t num_memslots;
> > diff --git a/server/spice-server.syms b/server/spice-server.syms
> > index edf04a42..ac4d9b14 100644
> > --- a/server/spice-server.syms
> > +++ b/server/spice-server.syms
> > @@ -173,3 +173,8 @@ 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_info;
> > +} SPICE_SERVER_0.13.2;
> 
> 

Re: [Qemu-devel] [Spice-devel] [PATCH spice 1/3] QXL interface: add a function to identify monitors in the guest

[Jonathon Jongsma]

On Fri, 2019-01-11 at 11:15 +0100, Lukáš Hrázký wrote:
> On Wed, 2019-01-09 at 11:36 -0600, Jonathon Jongsma wrote:
> > On Tue, 2019-01-08 at 16:26 +0100, Lukáš Hrázký wrote:
> > > Adds a function to let QEMU provide information to identify
> > > graphics
> > > devices and their monitors in the guest. The function
> > > (spice_qxl_set_device_info) sets the device address (e.g. a PCI
> > > path)
> > > and monitor ID -> device display ID mapping of displays exposed
> > > by
> > > given
> > > QXL interface.
> > 
> > 
> > In my previous review, I asked for a slightly more explicit
> > explanation
> > of the phrase "monitor ID" in the commit log. In this case,
> > "monitor
> > ID" is an ID specific to the QXL instance starting at 0 and
> > represents
> > the displays that are associated with that particular QXL instance.
> > 
> > Perhaps it makes sense to rename this "Qxl monitor ID"?
> 
> Sorry, I might have forgotten to incorporate some comments on the old
> series :/
> 
> To reiterate, though, what do you find unclear about the "monitor
> ID"?
> I mean maybe it should be spelled "monitor_id" to make it clearer
> what
> it refers to... Not sure if "Qxl monitor ID" helps, since it is not
> strictly related to QXL, but more to the QXL interface (which is used
> for all graphic cards, which makes it more confusing)...

Well, there are so many somewhat similar phrases being used for
slightly different things here that I'm afraid that I'll get confused
in the future when I come back to look at this stuff. But as you say,
I'm not sure there's a good way to make it clearer. You can just ignore
this bit if you want

> 
> > > Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
> > > ---
> > >  server/red-qxl.c         | 44
> > > ++++++++++++++++++++++++++++++++++++++
> > >  server/spice-qxl.h       | 46
> > > ++++++++++++++++++++++++++++++++++++++++
> > >  server/spice-server.syms |  5 +++++
> > >  3 files changed, 95 insertions(+)
> > > 
> > > diff --git a/server/red-qxl.c b/server/red-qxl.c
> > > index 97940611..0ea424cd 100644
> > > --- a/server/red-qxl.c
> > > +++ b/server/red-qxl.c
> > > @@ -41,6 +41,9 @@
> > >  #include "red-qxl.h"
> > >  
> > >  
> > > +#define MAX_DEVICE_ADDRESS_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_DEVICE_ADDRESS_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,44 @@ void
> > > red_qxl_gl_draw_async_complete(QXLInstance
> > > *qxl)
> > >      red_qxl_async_complete(qxl, cookie);
> > >  }
> > >  
> > > +SPICE_GNUC_VISIBLE
> > > +void spice_qxl_set_device_info(QXLInstance *instance,
> > > +                               const char *device_address,
> > > +                               uint32_t device_display_id_start,
> > > +                               uint32_t device_display_id_count)
> > > +{
> > > +    g_return_if_fail(device_address != NULL);
> > 
> > Just a thought: what if qemu calls this function twice for the same
> > instance. In theory this shouldn't happen, but should we be
> > defensive
> > and handle it somehow? Print a warning? Just overwrite the previous
> > mapping? Right now it looks like we would just overwrite.
> 
> I'm pretty sure overwriting is the correct behavior here, no logging
> needed? There may be a situation where the info changes in the
> future,
> for whatever reasons. At least this part of the code should be fine
> with it (though I'm not sure atm. if e.g. the client would cope
> well).

At the moment, it seems more likely that if this function was called
twice, it would be due to a bug somewhere. Which is why I wondered
about printing a warning or something. I can't really imagine a
scenario where we'd want to change the device info after it was set.
Anyway, like I said, it's mostly a theoretical issue at this point.

Jonathon

> 
> > > +
> > > +    size_t da_len = strnlen(device_address,
> > > MAX_DEVICE_ADDRESS_LEN);
> > > +    if (da_len >= MAX_DEVICE_ADDRESS_LEN) {
> > > +        spice_error("Device address too long: %lu > %u", da_len,
> > > MAX_DEVICE_ADDRESS_LEN);
> > 
> > Technically, I think the format string for a size_t variable is
> > %zu,
> > not %lu
> 
> You're right, didn't know that. I think I have some fixing to do :)
> 
> > > +        return;
> > > +    }
> > > +
> > > +    if (device_display_id_count > MAX_MONITORS_COUNT) {
> > > +        spice_error("Device display ID count (%u) is greater
> > > than
> > > limit %u",
> > > +                    device_display_id_count,
> > > +                    MAX_MONITORS_COUNT);
> > > +        return;
> > > +    }
> > > +
> > > +    strncpy(instance->st->device_address, device_address,
> > > MAX_DEVICE_ADDRESS_LEN);
> > > +
> > > +    g_debug("QXL Instance %d setting device address \"%s\" and
> > > monitor -> device display mapping:",
> > > +            instance->id,
> > > +            device_address);
> > > +
> > > +    // store the mapping monitor_id -> device_display_id
> > > +    for (uint32_t monitor_id = 0; monitor_id <
> > > device_display_id_count; ++monitor_id) {
> > > +        uint32_t device_display_id = device_display_id_start +
> > > monitor_id;
> > > +        instance->st->device_display_ids[monitor_id] =
> > > device_display_id;
> > > +        g_debug("   monitor ID %u -> device display ID %u",
> > > +                monitor_id, device_display_id);
> > > +    }
> > > +
> > > +    instance->st->monitors_count = device_display_id_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..547d3d93 100644
> > > --- a/server/spice-qxl.h
> > > +++ b/server/spice-qxl.h
> > > @@ -115,6 +115,52 @@ void spice_qxl_gl_draw_async(QXLInstance
> > > *instance,
> > >                               uint32_t w, uint32_t h,
> > >                               uint64_t cookie);
> > >  
> > > +/* since spice 0.14.2 */
> > > +
> > > +/**
> > > + * spice_qxl_set_device_info:
> > > + * @instance the QXL instance to set the path to
> > > + * @device_address the path of the device this QXL instance
> > > belongs
> > > to
> > > + * @device_display_id_start the starting device display ID of
> > > this
> > > interface,
> > > + *                          i.e. the one monitor ID 0 maps to
> > 
> > perhaps reword this last line:
> > i.e. the device display ID of monitor ID 0
> 
> Will do.
> 
> > > + * @device_display_id_count the total number of device display
> > > IDs
> > > on this
> > > + *                          interface
> > > + *
> > > + * Sets the device information for this QXL interface, i.e. the
> > > hardware
> > > + * address (e.g. PCI) of the graphics device and the IDs of the
> > > displays of the
> > > + * graphics device that are exposed by this interface (device
> > > display IDs).
> > > + *
> > > + * The supported device address format is:
> > > + * "pci/<DOMAIN>/<SLOT>.<FUNCTION>/.../<SLOT>.<FUNCTION>"
> > > + *
> > > + * The "pci" identifies the rest of the string as a PCI address.
> > > 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. All of <DOMAIN>, <SLOT>, <FUNCTION> are
> > > hexadecimal numbers
> > > + * with the following number of digits:
> > > + *   <DOMAIN>: 4
> > > + *   <SLOT>: 2
> > > + *   <FUNCTION>: 1
> > > + *
> > > + * The device_display_id_{start,count} denotes the sequence of
> > > device display
> > > + * IDs that map to the zero-based sequence of monitor IDs
> > > provided
> > > by monitors
> > > + * config on this interface. For example with
> > > device_display_id_start = 2 and
> > > + * device_display_id_count = 3 you get the following mapping:
> > > + * monitor_id  ->  device_display_id
> > > + *          0  ->  2
> > > + *          1  ->  3
> > > + *          2  ->  4
> > > + *
> > > + * Note this example is unsupported in practice. The only
> > > supported
> > > cases are
> > > + * either a single device display ID (count = 1) or multiple
> > > device
> > > display IDs
> > > + * in a sequence starting from 0.
> > > + */
> > 
> > Gerd previously objected to using an example of an unsupported
> > scenario. But it doesn't seem that you changed anything here. I
> > don't
> > care too much, myself.
> 
> Right, forgot about that.
> 
> Thanks,
> Lukas
> 
> > > +void spice_qxl_set_device_info(QXLInstance *instance,
> > > +                               const char *device_address,
> > > +                               uint32_t device_display_id_start,
> > > +                               uint32_t
> > > device_display_id_count);
> > > +
> > >  typedef struct QXLDevInitInfo {
> > >      uint32_t num_memslots_groups;
> > >      uint32_t num_memslots;
> > > diff --git a/server/spice-server.syms b/server/spice-server.syms
> > > index edf04a42..ac4d9b14 100644
> > > --- a/server/spice-server.syms
> > > +++ b/server/spice-server.syms
> > > @@ -173,3 +173,8 @@ 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_info;
> > > +} SPICE_SERVER_0.13.2;
> > 
> >