All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PULL 0/7] spice patch queue
@ 2015-03-04 14:30 Gerd Hoffmann
  2015-03-04 14:30 ` [Qemu-devel] [PULL 1/7] qxl: document minimal video memory for new modes Gerd Hoffmann
                   ` (7 more replies)
  0 siblings, 8 replies; 14+ messages in thread
From: Gerd Hoffmann @ 2015-03-04 14:30 UTC (permalink / raw)
  To: qemu-devel; +Cc: Gerd Hoffmann

  Hi,

Here comes the spice patch queue with a bunch of misc, small spice fixes
which arrived revently.  Nothing outstanding.

please pull,
  Gerd

The following changes since commit 0856579cac2f1dacecd847cfcd89680d26ff78f5:

  Revert "Merge remote-tracking branch 'remotes/ehabkost/tags/x86-pull-request' into staging" (2015-03-03 00:29:17 +0000)

are available in the git repository at:

  git://anongit.freedesktop.org/spice/qemu tags/pull-spice-20150304-1

for you to fetch changes up to 7c6044a94e52db8aef9a71d616c7a0914adb71ab:

  hmp: info spice: take out webdav (2015-03-04 14:47:52 +0100)

----------------------------------------------------------------
misc spice/qxl fixes.

----------------------------------------------------------------
Cole Robinson (1):
      hmp: info spice: Show string channel name

Gerd Hoffmann (2):
      qxl: drop update_displaychangelistener call for secondary qxl devices
      hmp: info spice: take out webdav

Radim Krčmář (4):
      qxl: document minimal video memory for new modes
      spice: fix invalid memory access to vga.vram
      qxl: refactor rounding up to a nearest power of 2
      vga: refactor vram_size clamping and rounding

 hmp.c                 | 32 ++++++++++++++++++++++++++++++++
 hw/display/qxl.c      | 35 ++++++++++++++++-------------------
 hw/display/vga.c      | 22 +++++++++++++++-------
 include/qemu-common.h |  3 +++
 util/cutils.c         | 14 ++++++++++++++
 5 files changed, 80 insertions(+), 26 deletions(-)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Qemu-devel] [PULL 1/7] qxl: document minimal video memory for new modes
  2015-03-04 14:30 [Qemu-devel] [PULL 0/7] spice patch queue Gerd Hoffmann
@ 2015-03-04 14:30 ` Gerd Hoffmann
  2015-03-04 14:30 ` [Qemu-devel] [PULL 2/7] spice: fix invalid memory access to vga.vram Gerd Hoffmann
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Gerd Hoffmann @ 2015-03-04 14:30 UTC (permalink / raw)
  To: qemu-devel; +Cc: Gerd Hoffmann, Radim Krčmář

From: Radim Krčmář <rkrcmar@redhat.com>

The alternative to removing existing comments.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/qxl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 61df477..6e90797 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -120,9 +120,12 @@ static QXLMode qxl_modes[] = {
     QXL_MODE_EX(2560, 2048),
     QXL_MODE_EX(2800, 2100),
     QXL_MODE_EX(3200, 2400),
+    /* these modes need more than 32 MB video memory */
     QXL_MODE_EX(3840, 2160), /* 4k mainstream */
     QXL_MODE_EX(4096, 2160), /* 4k            */
+    /* these modes need more than 64 MB video memory */
     QXL_MODE_EX(7680, 4320), /* 8k mainstream */
+    /* these modes need more than 128 MB video memory */
     QXL_MODE_EX(8192, 4320), /* 8k            */
 };
 
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Qemu-devel] [PULL 2/7] spice: fix invalid memory access to vga.vram
  2015-03-04 14:30 [Qemu-devel] [PULL 0/7] spice patch queue Gerd Hoffmann
  2015-03-04 14:30 ` [Qemu-devel] [PULL 1/7] qxl: document minimal video memory for new modes Gerd Hoffmann
@ 2015-03-04 14:30 ` Gerd Hoffmann
  2015-03-04 14:30 ` [Qemu-devel] [PULL 3/7] qxl: refactor rounding up to a nearest power of 2 Gerd Hoffmann
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Gerd Hoffmann @ 2015-03-04 14:30 UTC (permalink / raw)
  To: qemu-devel; +Cc: Gerd Hoffmann, Radim Krčmář

From: Radim Krčmář <rkrcmar@redhat.com>

vga_common_init() doesn't allow more than 256 MiB vram size and silently
shrinks any larger value.  qxl_dirty_surfaces() used the unshrinked size
via qxl->shadow_rom.surface0_area_size when accessing the memory, which
resulted in segfault.

Add a workaround for this case and an assert if it happens again.

We have to bump the vga memory limit too, because 256 MiB wouldn't have
allowed 8k (it requires more than 128 MiB).
1024 MiB doesn't work, but 512 MiB seems fine.

Proposed-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/qxl.c | 8 ++++++++
 hw/display/vga.c | 4 ++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 6e90797..92f2d50 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -370,6 +370,8 @@ static void init_qxl_rom(PCIQXLDevice *d)
     num_pages         -= surface0_area_size;
     num_pages          = num_pages / QXL_PAGE_SIZE;
 
+    assert(ram_header_size + surface0_area_size <= d->vga.vram_size);
+
     rom->draw_area_offset   = cpu_to_le32(0);
     rom->surface0_area_size = cpu_to_le32(surface0_area_size);
     rom->pages_offset       = cpu_to_le32(surface0_area_size);
@@ -1883,6 +1885,12 @@ static void qxl_init_ramsize(PCIQXLDevice *qxl)
     if (qxl->vgamem_size_mb < 8) {
         qxl->vgamem_size_mb = 8;
     }
+    /* XXX: we round vgamem_size_mb up to a nearest power of two and it must be
+     * less than vga_common_init()'s maximum on qxl->vga.vram_size (512 now).
+     */
+    if (qxl->vgamem_size_mb > 256) {
+        qxl->vgamem_size_mb = 256;
+    }
     qxl->vgamem_size = qxl->vgamem_size_mb * 1024 * 1024;
 
     /* vga ram (bar 0, total) */
diff --git a/hw/display/vga.c b/hw/display/vga.c
index c8c49ab..6e4ca7e 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2121,10 +2121,10 @@ void vga_common_init(VGACommonState *s, Object *obj, bool global_vmstate)
         expand4to8[i] = v;
     }
 
-    /* valid range: 1 MB -> 256 MB */
+    /* valid range: 1 MB -> 512 MB */
     s->vram_size = 1024 * 1024;
     while (s->vram_size < (s->vram_size_mb << 20) &&
-           s->vram_size < (256 << 20)) {
+           s->vram_size < (512 << 20)) {
         s->vram_size <<= 1;
     }
     s->vram_size_mb = s->vram_size >> 20;
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Qemu-devel] [PULL 3/7] qxl: refactor rounding up to a nearest power of 2
  2015-03-04 14:30 [Qemu-devel] [PULL 0/7] spice patch queue Gerd Hoffmann
  2015-03-04 14:30 ` [Qemu-devel] [PULL 1/7] qxl: document minimal video memory for new modes Gerd Hoffmann
  2015-03-04 14:30 ` [Qemu-devel] [PULL 2/7] spice: fix invalid memory access to vga.vram Gerd Hoffmann
@ 2015-03-04 14:30 ` Gerd Hoffmann
  2015-03-05  4:57   ` Dongsheng Song
  2015-03-05  9:52   ` Markus Armbruster
  2015-03-04 14:30 ` [Qemu-devel] [PULL 4/7] vga: refactor vram_size clamping and rounding Gerd Hoffmann
                   ` (4 subsequent siblings)
  7 siblings, 2 replies; 14+ messages in thread
From: Gerd Hoffmann @ 2015-03-04 14:30 UTC (permalink / raw)
  To: qemu-devel; +Cc: Gerd Hoffmann, Radim Krčmář

From: Radim Krčmář <rkrcmar@redhat.com>

We already have pow2floor, mirror it and use instead of a function with
similar results (same in used domain), to clarify our intent.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/qxl.c      | 23 +++++------------------
 include/qemu-common.h |  3 +++
 util/cutils.c         | 14 ++++++++++++++
 3 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 92f2d50..94ff52a 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -300,19 +300,6 @@ void qxl_spice_reset_cursor(PCIQXLDevice *qxl)
     qxl->ssd.cursor = cursor_builtin_hidden();
 }
 
-
-static inline uint32_t msb_mask(uint32_t val)
-{
-    uint32_t mask;
-
-    do {
-        mask = ~(val - 1) & val;
-        val &= ~mask;
-    } while (mask < val);
-
-    return mask;
-}
-
 static ram_addr_t qxl_rom_size(void)
 {
     uint32_t required_rom_size = sizeof(QXLRom) + sizeof(QXLModes) +
@@ -1921,10 +1908,10 @@ static void qxl_init_ramsize(PCIQXLDevice *qxl)
         qxl->vram32_size = 4096;
         qxl->vram_size = 4096;
     }
-    qxl->vgamem_size = msb_mask(qxl->vgamem_size * 2 - 1);
-    qxl->vga.vram_size = msb_mask(qxl->vga.vram_size * 2 - 1);
-    qxl->vram32_size = msb_mask(qxl->vram32_size * 2 - 1);
-    qxl->vram_size = msb_mask(qxl->vram_size * 2 - 1);
+    qxl->vgamem_size = pow2ceil(qxl->vgamem_size);
+    qxl->vga.vram_size = pow2ceil(qxl->vga.vram_size);
+    qxl->vram32_size = pow2ceil(qxl->vram32_size);
+    qxl->vram_size = pow2ceil(qxl->vram_size);
 }
 
 static int qxl_init_common(PCIQXLDevice *qxl)
@@ -1956,7 +1943,7 @@ static int qxl_init_common(PCIQXLDevice *qxl)
         break;
     case 4: /* qxl-4 */
         pci_device_rev = QXL_REVISION_STABLE_V12;
-        io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
+        io_size = pow2ceil(QXL_IO_RANGE_SIZE);
         break;
     default:
         error_report("Invalid revision %d for qxl device (max %d)",
diff --git a/include/qemu-common.h b/include/qemu-common.h
index 644b46d..1b5cffb 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -418,6 +418,9 @@ static inline bool is_power_of_2(uint64_t value)
 /* round down to the nearest power of 2*/
 int64_t pow2floor(int64_t value);
 
+/* round up to the nearest power of 2 (0 if overflow) */
+uint64_t pow2ceil(uint64_t value);
+
 #include "qemu/module.h"
 
 /*
diff --git a/util/cutils.c b/util/cutils.c
index dbe7412..c2250d1 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -483,6 +483,20 @@ int64_t pow2floor(int64_t value)
     return value;
 }
 
+/* round up to the nearest power of 2 (0 if overflow) */
+uint64_t pow2ceil(uint64_t value)
+{
+    uint8_t nlz = clz64(value);
+
+    if (is_power_of_2(value)) {
+        return value;
+    }
+    if (!nlz) {
+        return 0;
+    }
+    return 1ULL << (64 - nlz);
+}
+
 /*
  * Implementation of  ULEB128 (http://en.wikipedia.org/wiki/LEB128)
  * Input is limited to 14-bit numbers
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Qemu-devel] [PULL 4/7] vga: refactor vram_size clamping and rounding
  2015-03-04 14:30 [Qemu-devel] [PULL 0/7] spice patch queue Gerd Hoffmann
                   ` (2 preceding siblings ...)
  2015-03-04 14:30 ` [Qemu-devel] [PULL 3/7] qxl: refactor rounding up to a nearest power of 2 Gerd Hoffmann
@ 2015-03-04 14:30 ` Gerd Hoffmann
  2015-03-04 14:30 ` [Qemu-devel] [PULL 5/7] qxl: drop update_displaychangelistener call for secondary qxl devices Gerd Hoffmann
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Gerd Hoffmann @ 2015-03-04 14:30 UTC (permalink / raw)
  To: qemu-devel; +Cc: Gerd Hoffmann, Radim Krčmář

From: Radim Krčmář <rkrcmar@redhat.com>

Make the code a bit more obvious.

We don't have min/max, so a general helper for clamp probably isn't
acceptable either.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vga.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 6e4ca7e..c0f7b34 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2094,6 +2094,17 @@ static const GraphicHwOps vga_ops = {
     .text_update = vga_update_text,
 };
 
+static inline uint32_t uint_clamp(uint32_t val, uint32_t vmin, uint32_t vmax)
+{
+    if (val < vmin) {
+        return vmin;
+    }
+    if (val > vmax) {
+        return vmax;
+    }
+    return val;
+}
+
 void vga_common_init(VGACommonState *s, Object *obj, bool global_vmstate)
 {
     int i, j, v, b;
@@ -2121,13 +2132,10 @@ void vga_common_init(VGACommonState *s, Object *obj, bool global_vmstate)
         expand4to8[i] = v;
     }
 
-    /* valid range: 1 MB -> 512 MB */
-    s->vram_size = 1024 * 1024;
-    while (s->vram_size < (s->vram_size_mb << 20) &&
-           s->vram_size < (512 << 20)) {
-        s->vram_size <<= 1;
-    }
-    s->vram_size_mb = s->vram_size >> 20;
+    s->vram_size_mb = uint_clamp(s->vram_size_mb, 1, 512);
+    s->vram_size_mb = pow2ceil(s->vram_size_mb);
+    s->vram_size = s->vram_size_mb << 20;
+
     if (!s->vbe_size) {
         s->vbe_size = s->vram_size;
     }
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Qemu-devel] [PULL 5/7] qxl: drop update_displaychangelistener call for secondary qxl devices
  2015-03-04 14:30 [Qemu-devel] [PULL 0/7] spice patch queue Gerd Hoffmann
                   ` (3 preceding siblings ...)
  2015-03-04 14:30 ` [Qemu-devel] [PULL 4/7] vga: refactor vram_size clamping and rounding Gerd Hoffmann
@ 2015-03-04 14:30 ` Gerd Hoffmann
  2015-03-04 14:30 ` [Qemu-devel] [PULL 6/7] hmp: info spice: Show string channel name Gerd Hoffmann
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Gerd Hoffmann @ 2015-03-04 14:30 UTC (permalink / raw)
  To: qemu-devel; +Cc: Gerd Hoffmann

Commit 3dcadce5076d4b42fa395c39662d65e050b77784 added three
update_displaychangelistener call sites:

Two for primary qxl cards, when entering/leaving vga mode, which are
correct.

One for secondary qxl cards, which is wrong because we don't register
a displaychangelistener in the first place for secondary cards.

Remove it.

Reported-by: Brad Campbell <lists2009@fnarfbargle.com>
Tested-by: Brad Campbell <lists2009@fnarfbargle.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/qxl.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 94ff52a..762f75d 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -1147,7 +1147,6 @@ static void qxl_soft_reset(PCIQXLDevice *d)
         qxl_enter_vga_mode(d);
     } else {
         d->mode = QXL_MODE_UNDEFINED;
-        update_displaychangelistener(&d->ssd.dcl, GUI_REFRESH_INTERVAL_IDLE);
     }
 }
 
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Qemu-devel] [PULL 6/7] hmp: info spice: Show string channel name
  2015-03-04 14:30 [Qemu-devel] [PULL 0/7] spice patch queue Gerd Hoffmann
                   ` (4 preceding siblings ...)
  2015-03-04 14:30 ` [Qemu-devel] [PULL 5/7] qxl: drop update_displaychangelistener call for secondary qxl devices Gerd Hoffmann
@ 2015-03-04 14:30 ` Gerd Hoffmann
  2015-03-04 14:30 ` [Qemu-devel] [PULL 7/7] hmp: info spice: take out webdav Gerd Hoffmann
  2015-03-08 12:27 ` [Qemu-devel] [PULL 0/7] spice patch queue Peter Maydell
  7 siblings, 0 replies; 14+ messages in thread
From: Gerd Hoffmann @ 2015-03-04 14:30 UTC (permalink / raw)
  To: qemu-devel; +Cc: Luiz Capitulino, Gerd Hoffmann, Cole Robinson

From: Cole Robinson <crobinso@redhat.com>

Useful for debugging.

https://bugzilla.redhat.com/show_bug.cgi?id=822418
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hmp.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/hmp.c b/hmp.c
index 735097c..eacfb1b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -29,6 +29,10 @@
 #include "block/qapi.h"
 #include "qemu-io.h"
 
+#ifdef CONFIG_SPICE
+#include <spice/enums.h>
+#endif
+
 static void hmp_handle_error(Monitor *mon, Error **errp)
 {
     assert(errp);
@@ -545,6 +549,20 @@ void hmp_info_spice(Monitor *mon, const QDict *qdict)
 {
     SpiceChannelList *chan;
     SpiceInfo *info;
+    const char *channel_name;
+    const char * const channel_names[] = {
+        [SPICE_CHANNEL_MAIN] = "main",
+        [SPICE_CHANNEL_DISPLAY] = "display",
+        [SPICE_CHANNEL_INPUTS] = "inputs",
+        [SPICE_CHANNEL_CURSOR] = "cursor",
+        [SPICE_CHANNEL_PLAYBACK] = "playback",
+        [SPICE_CHANNEL_RECORD] = "record",
+        [SPICE_CHANNEL_TUNNEL] = "tunnel",
+        [SPICE_CHANNEL_SMARTCARD] = "smartcard",
+        [SPICE_CHANNEL_USBREDIR] = "usbredir",
+        [SPICE_CHANNEL_PORT] = "port",
+        [SPICE_CHANNEL_WEBDAV] = "webdav",
+    };
 
     info = qmp_query_spice(NULL);
 
@@ -581,6 +599,15 @@ void hmp_info_spice(Monitor *mon, const QDict *qdict)
                            chan->value->connection_id);
             monitor_printf(mon, "     channel: %" PRId64 ":%" PRId64 "\n",
                            chan->value->channel_type, chan->value->channel_id);
+
+            channel_name = "unknown";
+            if (chan->value->channel_type > 0 &&
+                chan->value->channel_type < ARRAY_SIZE(channel_names) &&
+                channel_names[chan->value->channel_type]) {
+                channel_name = channel_names[chan->value->channel_type];
+            }
+
+            monitor_printf(mon, "     channel name: %s\n", channel_name);
         }
     }
 
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Qemu-devel] [PULL 7/7] hmp: info spice: take out webdav
  2015-03-04 14:30 [Qemu-devel] [PULL 0/7] spice patch queue Gerd Hoffmann
                   ` (5 preceding siblings ...)
  2015-03-04 14:30 ` [Qemu-devel] [PULL 6/7] hmp: info spice: Show string channel name Gerd Hoffmann
@ 2015-03-04 14:30 ` Gerd Hoffmann
  2015-03-08 12:27 ` [Qemu-devel] [PULL 0/7] spice patch queue Peter Maydell
  7 siblings, 0 replies; 14+ messages in thread
From: Gerd Hoffmann @ 2015-03-04 14:30 UTC (permalink / raw)
  To: qemu-devel; +Cc: Gerd Hoffmann, Luiz Capitulino

Obvious suggestion for the next spice-protocol
release: Add some way to #ifdef new stuff.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
---
 hmp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hmp.c b/hmp.c
index eacfb1b..71c28bc 100644
--- a/hmp.c
+++ b/hmp.c
@@ -561,7 +561,12 @@ void hmp_info_spice(Monitor *mon, const QDict *qdict)
         [SPICE_CHANNEL_SMARTCARD] = "smartcard",
         [SPICE_CHANNEL_USBREDIR] = "usbredir",
         [SPICE_CHANNEL_PORT] = "port",
+#if 0
+        /* minimum spice-protocol is 0.12.3, webdav was added in 0.12.7,
+         * no easy way to #ifdef (SPICE_CHANNEL_* is a enum).  Disable
+         * as quick fix for build failures with older versions. */
         [SPICE_CHANNEL_WEBDAV] = "webdav",
+#endif
     };
 
     info = qmp_query_spice(NULL);
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PULL 3/7] qxl: refactor rounding up to a nearest power of 2
  2015-03-04 14:30 ` [Qemu-devel] [PULL 3/7] qxl: refactor rounding up to a nearest power of 2 Gerd Hoffmann
@ 2015-03-05  4:57   ` Dongsheng Song
  2015-03-05 15:35     ` Radim Krčmář
  2015-03-05  9:52   ` Markus Armbruster
  1 sibling, 1 reply; 14+ messages in thread
From: Dongsheng Song @ 2015-03-05  4:57 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: qemu-devel, Radim Krčmář

[-- Attachment #1: Type: text/plain, Size: 420 bytes --]

On Wed, Mar 4, 2015 at 10:30 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:

> +/* round up to the nearest power of 2 (0 if overflow) */
> +uint64_t pow2ceil(uint64_t value)
> +{
> +    uint8_t nlz = clz64(value);
> +
> +    if (is_power_of_2(value)) {
> +        return value;
> +    }
> +    if (!nlz) {
> +        return 0;
> +    }
> +    return 1ULL << (64 - nlz);
> +}
> +
>


please call clz64 after is_power_of_2.

[-- Attachment #2: Type: text/html, Size: 883 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PULL 3/7] qxl: refactor rounding up to a nearest power of 2
  2015-03-04 14:30 ` [Qemu-devel] [PULL 3/7] qxl: refactor rounding up to a nearest power of 2 Gerd Hoffmann
  2015-03-05  4:57   ` Dongsheng Song
@ 2015-03-05  9:52   ` Markus Armbruster
  2015-03-05 10:11     ` Gerd Hoffmann
  2015-03-05 15:12     ` Radim Krčmář
  1 sibling, 2 replies; 14+ messages in thread
From: Markus Armbruster @ 2015-03-05  9:52 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: qemu-devel, Radim Krčmář

Gerd Hoffmann <kraxel@redhat.com> writes:

> From: Radim Krčmář <rkrcmar@redhat.com>
>
> We already have pow2floor, mirror it and use instead of a function with
> similar results (same in used domain), to clarify our intent.
>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
[...]
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index 644b46d..1b5cffb 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -418,6 +418,9 @@ static inline bool is_power_of_2(uint64_t value)
>  /* round down to the nearest power of 2*/
>  int64_t pow2floor(int64_t value);
>  
> +/* round up to the nearest power of 2 (0 if overflow) */
> +uint64_t pow2ceil(uint64_t value);
> +
>  #include "qemu/module.h"
>  
>  /*
> diff --git a/util/cutils.c b/util/cutils.c
> index dbe7412..c2250d1 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -483,6 +483,20 @@ int64_t pow2floor(int64_t value)
>      return value;
>  }
>  
> +/* round up to the nearest power of 2 (0 if overflow) */

Callers need to check for overflow, but that's the callers' problem.

> +uint64_t pow2ceil(uint64_t value)
> +{
> +    uint8_t nlz = clz64(value);

You convert the value of clz64() from int to uint8_t only to promote it
right back to int in every single use.  Please don't muddy the waters
that way.

> +
> +    if (is_power_of_2(value)) {
> +        return value;
> +    }
> +    if (!nlz) {
> +        return 0;
> +    }
> +    return 1ULL << (64 - nlz);
> +}
> +
>  /*
>   * Implementation of  ULEB128 (http://en.wikipedia.org/wiki/LEB128)
>   * Input is limited to 14-bit numbers

Doesn't really mirror pow2floor() in master, because that one uses
int64_t.  Fine with me, because my "[PATCH 0/2] Proactive pow2floor()
fix, and dead code removal" changes pow2floor() to uint64_t.

Unfortunately, the two patches conflict.

This patch's implementation of pow2ceil() is needlessly complicated,
just like pow2floor() in master.  Simpler:

    uint64_t pow2ceil2(uint64_t value)
    {
        int n = clz64(value - 1);
        return n ? 1ull << (64 - n) : 0;
    }

I can rebase my patch on top of this one, and clean things up to my
taste.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PULL 3/7] qxl: refactor rounding up to a nearest power of 2
  2015-03-05  9:52   ` Markus Armbruster
@ 2015-03-05 10:11     ` Gerd Hoffmann
  2015-03-05 15:12     ` Radim Krčmář
  1 sibling, 0 replies; 14+ messages in thread
From: Gerd Hoffmann @ 2015-03-05 10:11 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: qemu-devel, Radim Krčmář

  Hi,

> This patch's implementation of pow2ceil() is needlessly complicated,
> just like pow2floor() in master.  Simpler:
> 
>     uint64_t pow2ceil2(uint64_t value)
>     {
>         int n = clz64(value - 1);
>         return n ? 1ull << (64 - n) : 0;
>     }
> 
> I can rebase my patch on top of this one, and clean things up to my
> taste.

I happily accept that offer (if that's ok with peter).

cheers,
  Gerd

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PULL 3/7] qxl: refactor rounding up to a nearest power of 2
  2015-03-05  9:52   ` Markus Armbruster
  2015-03-05 10:11     ` Gerd Hoffmann
@ 2015-03-05 15:12     ` Radim Krčmář
  1 sibling, 0 replies; 14+ messages in thread
From: Radim Krčmář @ 2015-03-05 15:12 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Gerd Hoffmann, qemu-devel

2015-03-05 10:52+0100, Markus Armbruster:
> Gerd Hoffmann <kraxel@redhat.com> writes:
> 
> > From: Radim Krčmář <rkrcmar@redhat.com>
> >
> > We already have pow2floor, mirror it and use instead of a function with
> > similar results (same in used domain), to clarify our intent.
> >
> > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> [...]
> > +/* round up to the nearest power of 2 (0 if overflow) */
> 
> Callers need to check for overflow, but that's the callers' problem.

It's obvious that the return value is 0 in this case -- the correct
result would have been a multiple of the modulo, but I wanted to be
explicit about it.

> > +uint64_t pow2ceil(uint64_t value)
> > +{
> > +    uint8_t nlz = clz64(value);
> 
> You convert the value of clz64() from int to uint8_t only to promote it
> right back to int in every single use.  Please don't muddy the waters
> that way.

Ok, the down-cast might cause some runtime overhead.
(I don't understand why the return value of clz64() is 'int' --
 using the smallest sufficient data type seems clearer to me.)

> > +    if (is_power_of_2(value)) {
> > +        return value;
> > +    }
> > +    if (!nlz) {
> > +        return 0;
> > +    }
> > +    return 1ULL << (64 - nlz);
> > +}
> 
> Doesn't really mirror pow2floor() in master, because that one uses
> int64_t.  Fine with me, because my "[PATCH 0/2] Proactive pow2floor()
> fix, and dead code removal" changes pow2floor() to uint64_t.

Yeah, I didn't understand why that returned 'int64_t' either.

I understood that pow2floor() used is_power_of_2() because it expected
higher probability of numbers that already are the power, so the clunky
code was balanced by a faster common case; and I duplicated this.
(clz64() without hardware support is slow.)

> Unfortunately, the two patches conflict.
> 
> This patch's implementation of pow2ceil() is needlessly complicated,
> just like pow2floor() in master.  Simpler:
> 
>     uint64_t pow2ceil2(uint64_t value)
>     {
>         int n = clz64(value - 1);
>         return n ? 1ull << (64 - n) : 0;

Mapping 0 to 0 is probably what we would want in most uses, but I'd name
the function differently then -- the closest power of 2 bigger than (or
equal to) 0 is 1, and 2^64 == 0, so the result was mathematically
correct.

We also lose the ability to detect overflow after the call, so callers
would have to do it before, with a code like
  'passed_value > DESTINATION_TYPE_MAX / 2 + 1'

> I can rebase my patch on top of this one, and clean things up to my
> taste.

Ok, thanks!

---
If eliminating is_power_of_2() and using the ternary operator is fine,
I'd write it like this:

    unsigned nlz = clz64(value - 1);
    return !value ? 1 : (nlz ? 1ULL << (64 - nlz) : 0);

If we don't believe that the compiler can optimize, and after applying
all I know about QEMU coding style:

    int nlz;

    if (!value) {
        return 1;
    }
    nlz = clz64(value - 1);
    return n ? 1ull << (64 - n) : 0;

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PULL 3/7] qxl: refactor rounding up to a nearest power of 2
  2015-03-05  4:57   ` Dongsheng Song
@ 2015-03-05 15:35     ` Radim Krčmář
  0 siblings, 0 replies; 14+ messages in thread
From: Radim Krčmář @ 2015-03-05 15:35 UTC (permalink / raw)
  To: Dongsheng Song; +Cc: Gerd Hoffmann, qemu-devel

2015-03-05 12:57+0800, Dongsheng Song:
> On Wed, Mar 4, 2015 at 10:30 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > +/* round up to the nearest power of 2 (0 if overflow) */
> > +uint64_t pow2ceil(uint64_t value)
> > +{
> > +    uint8_t nlz = clz64(value);
> > +
> > +    if (is_power_of_2(value)) {
> > +        return value;
> > +    }
> 
> please call clz64 after is_power_of_2.

All callers under clz64() are inline, so the optimizer can see that
there are no side effects and move the code if it would be faster;
I thought that separating it would only make the code uglier, because
QEMU doesn't allow lazy declarations ...

Markus had another design issue with this patch, so I'll send a v2 after
reaching the conclusion there.

Thanks.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PULL 0/7] spice patch queue
  2015-03-04 14:30 [Qemu-devel] [PULL 0/7] spice patch queue Gerd Hoffmann
                   ` (6 preceding siblings ...)
  2015-03-04 14:30 ` [Qemu-devel] [PULL 7/7] hmp: info spice: take out webdav Gerd Hoffmann
@ 2015-03-08 12:27 ` Peter Maydell
  7 siblings, 0 replies; 14+ messages in thread
From: Peter Maydell @ 2015-03-08 12:27 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: QEMU Developers

On 4 March 2015 at 23:30, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
> Here comes the spice patch queue with a bunch of misc, small spice fixes
> which arrived revently.  Nothing outstanding.
>
> please pull,
>   Gerd
>
> The following changes since commit 0856579cac2f1dacecd847cfcd89680d26ff78f5:
>
>   Revert "Merge remote-tracking branch 'remotes/ehabkost/tags/x86-pull-request' into staging" (2015-03-03 00:29:17 +0000)
>
> are available in the git repository at:
>
>   git://anongit.freedesktop.org/spice/qemu tags/pull-spice-20150304-1
>
> for you to fetch changes up to 7c6044a94e52db8aef9a71d616c7a0914adb71ab:
>
>   hmp: info spice: take out webdav (2015-03-04 14:47:52 +0100)
>
> ----------------------------------------------------------------
> misc spice/qxl fixes.
>
> ----------------------------------------------------------------

Applied, thanks.

-- PMM

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2015-03-08 12:27 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-04 14:30 [Qemu-devel] [PULL 0/7] spice patch queue Gerd Hoffmann
2015-03-04 14:30 ` [Qemu-devel] [PULL 1/7] qxl: document minimal video memory for new modes Gerd Hoffmann
2015-03-04 14:30 ` [Qemu-devel] [PULL 2/7] spice: fix invalid memory access to vga.vram Gerd Hoffmann
2015-03-04 14:30 ` [Qemu-devel] [PULL 3/7] qxl: refactor rounding up to a nearest power of 2 Gerd Hoffmann
2015-03-05  4:57   ` Dongsheng Song
2015-03-05 15:35     ` Radim Krčmář
2015-03-05  9:52   ` Markus Armbruster
2015-03-05 10:11     ` Gerd Hoffmann
2015-03-05 15:12     ` Radim Krčmář
2015-03-04 14:30 ` [Qemu-devel] [PULL 4/7] vga: refactor vram_size clamping and rounding Gerd Hoffmann
2015-03-04 14:30 ` [Qemu-devel] [PULL 5/7] qxl: drop update_displaychangelistener call for secondary qxl devices Gerd Hoffmann
2015-03-04 14:30 ` [Qemu-devel] [PULL 6/7] hmp: info spice: Show string channel name Gerd Hoffmann
2015-03-04 14:30 ` [Qemu-devel] [PULL 7/7] hmp: info spice: take out webdav Gerd Hoffmann
2015-03-08 12:27 ` [Qemu-devel] [PULL 0/7] spice patch queue Peter Maydell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.