[Qemu-devel] [PULL 0/2] Vga 20181029 patches

[Qemu-devel] [PULL 0/2] Vga 20181029 patches

[Gerd Hoffmann]

The following changes since commit b312532fd03413d0e6ae6767ec793a3e30f487b8:

  Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging (2018-10-19 19:01:07 +0100)

are available in the git repository at:

  git://git.kraxel.org/qemu tags/vga-20181029-pull-request

for you to fetch changes up to e69a10f468d8f6aa6c00a4308f5a8e1f2fd6b3a1:

  vga_int: remove unused function protype (2018-10-29 10:43:48 +0100)

----------------------------------------------------------------
vga: two fixes.

----------------------------------------------------------------

Gerd Hoffmann (1):
  qxl: store channel id in qxl->id

yuchenlin (1):
  vga_int: remove unused function protype

 hw/display/qxl.h     |  1 +
 hw/display/vga_int.h |  1 -
 hw/display/qxl.c     | 19 ++++++++++++-------
 3 files changed, 13 insertions(+), 8 deletions(-)

-- 
2.9.3

[Qemu-devel] [PULL 1/2] qxl: store channel id in qxl->id

[Gerd Hoffmann]

See qemu_spice_add_display_interface(), the console index is also used
as channel id.  So put that into the qxl->id field too.

In typical use cases (one primary qxl-vga device, optionally one or more
secondary qxl devices, no non-qxl display devices) this doesn't change
anything.

With this in place the qxl->id can not be used any more to figure
whenever a given device is primary (with vga compat mode) or secondary.
So add a bool to track this.

Cc: spice-devel@lists.freedesktop.org
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 20181012114540.27829-1-kraxel@redhat.com
---
 hw/display/qxl.h |  1 +
 hw/display/qxl.c | 19 ++++++++++++-------
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/hw/display/qxl.h b/hw/display/qxl.h
index dd9c0522b7..6f9d1f21fa 100644
--- a/hw/display/qxl.h
+++ b/hw/display/qxl.h
@@ -34,6 +34,7 @@ typedef struct PCIQXLDevice {
     PortioList         vga_port_list;
     SimpleSpiceDisplay ssd;
     int                id;
+    bool               have_vga;
     uint32_t           debug;
     uint32_t           guestdebug;
     uint32_t           cmdlog;
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index f608abc769..9087db5dee 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -848,7 +848,7 @@ static int interface_get_cursor_command(QXLInstance *sin, struct QXLCommandExt *
         qxl->guest_primary.commands++;
         qxl_track_command(qxl, ext);
         qxl_log_command(qxl, "csr", ext);
-        if (qxl->id == 0) {
+        if (qxl->have_vga) {
             qxl_render_cursor(qxl, ext);
         }
         trace_qxl_ring_cursor_get(qxl->id, qxl_mode_to_string(qxl->mode));
@@ -1255,7 +1255,7 @@ static void qxl_soft_reset(PCIQXLDevice *d)
     d->current_async = QXL_UNDEFINED_IO;
     qemu_mutex_unlock(&d->async_lock);
 
-    if (d->id == 0) {
+    if (d->have_vga) {
         qxl_enter_vga_mode(d);
     } else {
         d->mode = QXL_MODE_UNDEFINED;
@@ -2139,7 +2139,7 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error **errp)
 
     memory_region_init_io(&qxl->io_bar, OBJECT(qxl), &qxl_io_ops, qxl,
                           "qxl-ioports", io_size);
-    if (qxl->id == 0) {
+    if (qxl->have_vga) {
         vga_dirty_log_start(&qxl->vga);
     }
     memory_region_set_flush_coalesced(&qxl->io_bar);
@@ -2171,7 +2171,7 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error **errp)
 
     /* print pci bar details */
     dprint(qxl, 1, "ram/%s: %" PRId64 " MB [region 0]\n",
-           qxl->id == 0 ? "pri" : "sec", qxl->vga.vram_size / MiB);
+           qxl->have_vga ? "pri" : "sec", qxl->vga.vram_size / MiB);
     dprint(qxl, 1, "vram/32: %" PRIx64 " MB [region 1]\n",
            qxl->vram32_size / MiB);
     dprint(qxl, 1, "vram/64: %" PRIx64 " MB %s\n",
@@ -2199,7 +2199,6 @@ static void qxl_realize_primary(PCIDevice *dev, Error **errp)
     VGACommonState *vga = &qxl->vga;
     Error *local_err = NULL;
 
-    qxl->id = 0;
     qxl_init_ramsize(qxl);
     vga->vbe_size = qxl->vgamem_size;
     vga->vram_size_mb = qxl->vga.vram_size / MiB;
@@ -2210,8 +2209,15 @@ static void qxl_realize_primary(PCIDevice *dev, Error **errp)
                      vga, "vga");
     portio_list_set_flush_coalesced(&qxl->vga_port_list);
     portio_list_add(&qxl->vga_port_list, pci_address_space_io(dev), 0x3b0);
+    qxl->have_vga = true;
 
     vga->con = graphic_console_init(DEVICE(dev), 0, &qxl_ops, qxl);
+    qxl->id = qemu_console_get_index(vga->con); /* == channel_id */
+    if (qxl->id != 0) {
+        error_setg(errp, "primary qxl-vga device must be console 0 "
+                   "(first display device on the command line)");
+        return;
+    }
 
     qxl_realize_common(qxl, &local_err);
     if (local_err) {
@@ -2226,15 +2232,14 @@ static void qxl_realize_primary(PCIDevice *dev, Error **errp)
 
 static void qxl_realize_secondary(PCIDevice *dev, Error **errp)
 {
-    static int device_id = 1;
     PCIQXLDevice *qxl = PCI_QXL(dev);
 
-    qxl->id = device_id++;
     qxl_init_ramsize(qxl);
     memory_region_init_ram(&qxl->vga.vram, OBJECT(dev), "qxl.vgavram",
                            qxl->vga.vram_size, &error_fatal);
     qxl->vga.vram_ptr = memory_region_get_ram_ptr(&qxl->vga.vram);
     qxl->vga.con = graphic_console_init(DEVICE(dev), 0, &qxl_ops, qxl);
+    qxl->id = qemu_console_get_index(qxl->vga.con); /* == channel_id */
 
     qxl_realize_common(qxl, errp);
 }
-- 
2.9.3

Re: [Qemu-devel] [PULL 1/2] qxl: store channel id in qxl->id

[Frediano Ziglio]

> 
> See qemu_spice_add_display_interface(), the console index is also used
> as channel id.  So put that into the qxl->id field too.
> 
> In typical use cases (one primary qxl-vga device, optionally one or more
> secondary qxl devices, no non-qxl display devices) this doesn't change
> anything.
> 
> With this in place the qxl->id can not be used any more to figure
> whenever a given device is primary (with vga compat mode) or secondary.
> So add a bool to track this.
> 
> Cc: spice-devel@lists.freedesktop.org
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Message-id: 20181012114540.27829-1-kraxel@redhat.com
> ---
>  hw/display/qxl.h |  1 +
>  hw/display/qxl.c | 19 ++++++++++++-------
>  2 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/display/qxl.h b/hw/display/qxl.h
> index dd9c0522b7..6f9d1f21fa 100644
> --- a/hw/display/qxl.h
> +++ b/hw/display/qxl.h
> @@ -34,6 +34,7 @@ typedef struct PCIQXLDevice {
>      PortioList         vga_port_list;
>      SimpleSpiceDisplay ssd;
>      int                id;
> +    bool               have_vga;
>      uint32_t           debug;
>      uint32_t           guestdebug;
>      uint32_t           cmdlog;
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index f608abc769..9087db5dee 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -848,7 +848,7 @@ static int interface_get_cursor_command(QXLInstance *sin,
> struct QXLCommandExt *
>          qxl->guest_primary.commands++;
>          qxl_track_command(qxl, ext);
>          qxl_log_command(qxl, "csr", ext);
> -        if (qxl->id == 0) {
> +        if (qxl->have_vga) {
>              qxl_render_cursor(qxl, ext);
>          }
>          trace_qxl_ring_cursor_get(qxl->id, qxl_mode_to_string(qxl->mode));
> @@ -1255,7 +1255,7 @@ static void qxl_soft_reset(PCIQXLDevice *d)
>      d->current_async = QXL_UNDEFINED_IO;
>      qemu_mutex_unlock(&d->async_lock);
>  
> -    if (d->id == 0) {
> +    if (d->have_vga) {
>          qxl_enter_vga_mode(d);
>      } else {
>          d->mode = QXL_MODE_UNDEFINED;
> @@ -2139,7 +2139,7 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error
> **errp)
>  
>      memory_region_init_io(&qxl->io_bar, OBJECT(qxl), &qxl_io_ops, qxl,
>                            "qxl-ioports", io_size);
> -    if (qxl->id == 0) {
> +    if (qxl->have_vga) {
>          vga_dirty_log_start(&qxl->vga);
>      }
>      memory_region_set_flush_coalesced(&qxl->io_bar);
> @@ -2171,7 +2171,7 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error
> **errp)
>  
>      /* print pci bar details */
>      dprint(qxl, 1, "ram/%s: %" PRId64 " MB [region 0]\n",
> -           qxl->id == 0 ? "pri" : "sec", qxl->vga.vram_size / MiB);
> +           qxl->have_vga ? "pri" : "sec", qxl->vga.vram_size / MiB);
>      dprint(qxl, 1, "vram/32: %" PRIx64 " MB [region 1]\n",
>             qxl->vram32_size / MiB);
>      dprint(qxl, 1, "vram/64: %" PRIx64 " MB %s\n",
> @@ -2199,7 +2199,6 @@ static void qxl_realize_primary(PCIDevice *dev, Error
> **errp)
>      VGACommonState *vga = &qxl->vga;
>      Error *local_err = NULL;
>  
> -    qxl->id = 0;
>      qxl_init_ramsize(qxl);
>      vga->vbe_size = qxl->vgamem_size;
>      vga->vram_size_mb = qxl->vga.vram_size / MiB;
> @@ -2210,8 +2209,15 @@ static void qxl_realize_primary(PCIDevice *dev, Error
> **errp)
>                       vga, "vga");
>      portio_list_set_flush_coalesced(&qxl->vga_port_list);
>      portio_list_add(&qxl->vga_port_list, pci_address_space_io(dev), 0x3b0);
> +    qxl->have_vga = true;
>  
>      vga->con = graphic_console_init(DEVICE(dev), 0, &qxl_ops, qxl);
> +    qxl->id = qemu_console_get_index(vga->con); /* == channel_id */
> +    if (qxl->id != 0) {
> +        error_setg(errp, "primary qxl-vga device must be console 0 "
> +                   "(first display device on the command line)");
> +        return;
> +    }

In the comment this seems no more required so why testing it?

>  
>      qxl_realize_common(qxl, &local_err);
>      if (local_err) {
> @@ -2226,15 +2232,14 @@ static void qxl_realize_primary(PCIDevice *dev, Error
> **errp)
>  
>  static void qxl_realize_secondary(PCIDevice *dev, Error **errp)
>  {
> -    static int device_id = 1;
>      PCIQXLDevice *qxl = PCI_QXL(dev);
>  
> -    qxl->id = device_id++;
>      qxl_init_ramsize(qxl);
>      memory_region_init_ram(&qxl->vga.vram, OBJECT(dev), "qxl.vgavram",
>                             qxl->vga.vram_size, &error_fatal);
>      qxl->vga.vram_ptr = memory_region_get_ram_ptr(&qxl->vga.vram);
>      qxl->vga.con = graphic_console_init(DEVICE(dev), 0, &qxl_ops, qxl);
> +    qxl->id = qemu_console_get_index(qxl->vga.con); /* == channel_id */
>  

As these IDs must be contiguous this means that there must be the requirement
that if there is a qxl interface only qxl interfaces are used and no other
console which seems to me wrong.

>      qxl_realize_common(qxl, errp);
>  }

Frediano

Re: [Qemu-devel] [PULL 1/2] qxl: store channel id in qxl->id

[Gerd Hoffmann]

  Hi,

> >      vga->con = graphic_console_init(DEVICE(dev), 0, &qxl_ops, qxl);
> > +    qxl->id = qemu_console_get_index(vga->con); /* == channel_id */
> > +    if (qxl->id != 0) {
> > +        error_setg(errp, "primary qxl-vga device must be console 0 "
> > +                   "(first display device on the command line)");
> > +        return;
> > +    }
> 
> In the comment this seems no more required so why testing it?

I'd expect it'll confuse guests.

If you are sure this is not the case feel free to send a patch dropping
this check.

> >  static void qxl_realize_secondary(PCIDevice *dev, Error **errp)
> >  {
> > -    static int device_id = 1;
> >      PCIQXLDevice *qxl = PCI_QXL(dev);
> >  
> > -    qxl->id = device_id++;
> >      qxl_init_ramsize(qxl);
> >      memory_region_init_ram(&qxl->vga.vram, OBJECT(dev), "qxl.vgavram",
> >                             qxl->vga.vram_size, &error_fatal);
> >      qxl->vga.vram_ptr = memory_region_get_ram_ptr(&qxl->vga.vram);
> >      qxl->vga.con = graphic_console_init(DEVICE(dev), 0, &qxl_ops, qxl);
> > +    qxl->id = qemu_console_get_index(qxl->vga.con); /* == channel_id */
> >  
> 
> As these IDs must be contiguous this means that there must be the requirement
> that if there is a qxl interface only qxl interfaces are used and no other
> console which seems to me wrong.

Hmm.  Didn't know this is a requirement.  But as far I know qxl->id is
also expected to be the channel_id.  So with qxl and non-qxl mixed we
are in trouble no matter what:

  * with the patch we break the contiguous id requirement.
  * without the patch we break the qxl->id == channel_id requirement.

So, what now?

cheers,
  Gerd

Re: [Qemu-devel] [PULL 1/2] qxl: store channel id in qxl->id

[Frediano Ziglio]

> 
> Hi,
> 
> > >      vga->con = graphic_console_init(DEVICE(dev), 0, &qxl_ops, qxl);
> > > +    qxl->id = qemu_console_get_index(vga->con); /* == channel_id */
> > > +    if (qxl->id != 0) {
> > > +        error_setg(errp, "primary qxl-vga device must be console 0 "
> > > +                   "(first display device on the command line)");
> > > +        return;
> > > +    }
> > 
> > In the comment this seems no more required so why testing it?
> 
> I'd expect it'll confuse guests.
> 
> If you are sure this is not the case feel free to send a patch dropping
> this check.
> 
> > >  static void qxl_realize_secondary(PCIDevice *dev, Error **errp)
> > >  {
> > > -    static int device_id = 1;
> > >      PCIQXLDevice *qxl = PCI_QXL(dev);
> > >  
> > > -    qxl->id = device_id++;
> > >      qxl_init_ramsize(qxl);
> > >      memory_region_init_ram(&qxl->vga.vram, OBJECT(dev), "qxl.vgavram",
> > >                             qxl->vga.vram_size, &error_fatal);
> > >      qxl->vga.vram_ptr = memory_region_get_ram_ptr(&qxl->vga.vram);
> > >      qxl->vga.con = graphic_console_init(DEVICE(dev), 0, &qxl_ops, qxl);
> > > +    qxl->id = qemu_console_get_index(qxl->vga.con); /* == channel_id */
> > >  
> > 
> > As these IDs must be contiguous this means that there must be the
> > requirement
> > that if there is a qxl interface only qxl interfaces are used and no other
> > console which seems to me wrong.
> 
> Hmm.  Didn't know this is a requirement.  But as far I know qxl->id is
> also expected to be the channel_id.  So with qxl and non-qxl mixed we
> are in trouble no matter what:
> 
>   * with the patch we break the contiguous id requirement.

yes

>   * without the patch we break the qxl->id == channel_id requirement.

no, why? qxl->id is not Qemu index.

> 
> So, what now?
> 
> cheers,
>   Gerd
> 

Frediano

Re: [Qemu-devel] [PULL 1/2] qxl: store channel id in qxl->id

[Frediano Ziglio]

> Hi,
> 
> > >      vga->con = graphic_console_init(DEVICE(dev), 0, &qxl_ops, qxl);
> > > +    qxl->id = qemu_console_get_index(vga->con); /* == channel_id */
> > > +    if (qxl->id != 0) {
> > > +        error_setg(errp, "primary qxl-vga device must be console 0 "
> > > +                   "(first display device on the command line)");
> > > +        return;
> > > +    }
> > 
> > In the comment this seems no more required so why testing it?
> 
> I'd expect it'll confuse guests.
> 
> If you are sure this is not the case feel free to send a patch dropping
> this check.
> 

Had a look more deep into Qemu code. Both qxl_realize_primary and
qxl_realize_secondary calls qxl_realize_common which calls 
qemu_spice_add_display_interface to register the interface to spice.
Both qxl_realize_primary and qxl_realize_secondary set qxl->id
(first with 0 second from 1 as code below was doing).
There are 2 fields "id": PCIQXLDevice->id and PCIQXLDevice->ssd.qxl.id,
having PCIQXLDevice->ssd.qxl.id == qemu_console_get_index(PCIQXLDevice->vga->con)
== channel_id (PCIQXLDevice->ssd.qxl.id is set in qemu_spice_add_display_interface).
Looks like PCIQXLDevice->id, beside to distinguish if primary or not is
only used for tracing. Would not be better to remove PCIQXLDevice->id
at all after adding the have_vga flag?

> > >  static void qxl_realize_secondary(PCIDevice *dev, Error **errp)
> > >  {
> > > -    static int device_id = 1;
> > >      PCIQXLDevice *qxl = PCI_QXL(dev);
> > >  
> > > -    qxl->id = device_id++;
> > >      qxl_init_ramsize(qxl);
> > >      memory_region_init_ram(&qxl->vga.vram, OBJECT(dev), "qxl.vgavram",
> > >                             qxl->vga.vram_size, &error_fatal);
> > >      qxl->vga.vram_ptr = memory_region_get_ram_ptr(&qxl->vga.vram);
> > >      qxl->vga.con = graphic_console_init(DEVICE(dev), 0, &qxl_ops, qxl);
> > > +    qxl->id = qemu_console_get_index(qxl->vga.con); /* == channel_id */
> > >  
> > 
> > As these IDs must be contiguous this means that there must be the
> > requirement
> > that if there is a qxl interface only qxl interfaces are used and no other
> > console which seems to me wrong.
> 
> Hmm.  Didn't know this is a requirement.  But as far I know qxl->id is
> also expected to be the channel_id.  So with qxl and non-qxl mixed we
> are in trouble no matter what:
> 
>   * with the patch we break the contiguous id requirement.

PCIQXLDevice->id? No reason actually to be contiguous.
PCIQXLDevice->ssd.qxl.id is already qemu_console_get_index so potentially
not contiguous.

>   * without the patch we break the qxl->id == channel_id requirement.

Yes, can be broken although I suspect for other reasons this is true.

> 
> So, what now?
> 
> cheers,
>   Gerd
> 

I would personally add the "have_vga" field and use a single id field,
this will make sure to have the same consistent value.

Frediano

[Qemu-devel] [PULL 2/2] vga_int: remove unused function protype

[Gerd Hoffmann]

From: yuchenlin <yuchenlin@synology.com>

Signed-off-by: yuchenlin <yuchenlin@synology.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-id: 20181022080053.9379-1-yuchenlin@synology.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vga_int.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
index 6e4fa48a79..55c418eab5 100644
--- a/hw/display/vga_int.h
+++ b/hw/display/vga_int.h
@@ -166,7 +166,6 @@ MemoryRegion *vga_init_io(VGACommonState *s, Object *obj,
                           const MemoryRegionPortio **vbe_ports);
 void vga_common_reset(VGACommonState *s);
 
-void vga_sync_dirty_bitmap(VGACommonState *s);
 void vga_dirty_log_start(VGACommonState *s);
 void vga_dirty_log_stop(VGACommonState *s);
 
-- 
2.9.3

Re: [Qemu-devel] [PULL 0/2] Vga 20181029 patches

[Peter Maydell]

On 29 October 2018 at 12:47, Gerd Hoffmann <kraxel@redhat.com> wrote:
> The following changes since commit b312532fd03413d0e6ae6767ec793a3e30f487b8:
>
>   Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging (2018-10-19 19:01:07 +0100)
>
> are available in the git repository at:
>
>   git://git.kraxel.org/qemu tags/vga-20181029-pull-request
>
> for you to fetch changes up to e69a10f468d8f6aa6c00a4308f5a8e1f2fd6b3a1:
>
>   vga_int: remove unused function protype (2018-10-29 10:43:48 +0100)
>
> ----------------------------------------------------------------
> vga: two fixes.
>
> ----------------------------------------------------------------
>
> Gerd Hoffmann (1):
>   qxl: store channel id in qxl->id
>
> yuchenlin (1):
>   vga_int: remove unused function protype
>
>  hw/display/qxl.h     |  1 +
>  hw/display/vga_int.h |  1 -
>  hw/display/qxl.c     | 19 ++++++++++++-------
>  3 files changed, 13 insertions(+), 8 deletions(-)

Applied, thanks.

-- PMM