[Qemu-devel] [PATCH] qxl: fix -Waddress-of-packed-member

Marc-André Lureau posted 1 patch 4 years, 11 months ago
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test checkpatch passed
Test asan passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190408201203.28924-1-marcandre.lureau@redhat.com
Maintainers: Gerd Hoffmann <kraxel@redhat.com>
hw/display/qxl.c | 83 +++++++++++++++++++++++++++++-------------------
1 file changed, 50 insertions(+), 33 deletions(-)
[Qemu-devel] [PATCH] qxl: fix -Waddress-of-packed-member
Posted by Marc-André Lureau 4 years, 11 months ago
The GCC9 compiler complains about QXL code that takes the address of
members of the 'struct QXLReleaseRing' which is marked packed:

  CC      hw/display/qxl.o
/home/elmarco/src/qemu/hw/display/qxl.c: In function ‘init_qxl_ram’:
/home/elmarco/src/qemu/hw/display/qxl.c:50:19: warning: taking address of packed member of ‘struct QXLReleaseRing_ring_el’ may result in an unaligned pointer value [-Waddress-of-packed-member]
   50 |             ret = &(r)->items[prod].el;                                 \
      |                   ^~~~~~~~~~~~~~~~~~~~
/home/elmarco/src/qemu/hw/display/qxl.c:429:5: note: in expansion of macro ‘SPICE_RING_PROD_ITEM’
  429 |     SPICE_RING_PROD_ITEM(d, &d->ram->release_ring, item);
      |     ^~~~~~~~~~~~~~~~~~~~
/home/elmarco/src/qemu/hw/display/qxl.c: In function ‘qxl_push_free_res’:
/home/elmarco/src/qemu/hw/display/qxl.c:50:19: warning: taking address of packed member of ‘struct QXLReleaseRing_ring_el’ may result in an unaligned pointer value [-Waddress-of-packed-member]
   50 |             ret = &(r)->items[prod].el;                                 \
      |                   ^~~~~~~~~~~~~~~~~~~~
/home/elmarco/src/qemu/hw/display/qxl.c:762:5: note: in expansion of macro ‘SPICE_RING_PROD_ITEM’
  762 |     SPICE_RING_PROD_ITEM(d, ring, item);
      |     ^~~~~~~~~~~~~~~~~~~~
/home/elmarco/src/qemu/hw/display/qxl.c: In function ‘interface_release_resource’:
/home/elmarco/src/qemu/hw/display/qxl.c:50:19: warning: taking address of packed member of ‘struct QXLReleaseRing_ring_el’ may result in an unaligned pointer value [-Waddress-of-packed-member]
   50 |             ret = &(r)->items[prod].el;                                 \
      |                   ^~~~~~~~~~~~~~~~~~~~
/home/elmarco/src/qemu/hw/display/qxl.c:795:5: note: in expansion of macro ‘SPICE_RING_PROD_ITEM’
  795 |     SPICE_RING_PROD_ITEM(qxl, ring, item);
      |     ^~~~~~~~~~~~~~~~~~~~

Replace pointer usage by direct structure/array access instead.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/display/qxl.c | 83 +++++++++++++++++++++++++++++-------------------
 1 file changed, 50 insertions(+), 33 deletions(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index c8ce5781e0..12d83dd6f1 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -39,29 +39,49 @@
  * abort we just qxl_set_guest_bug and set the return to NULL. Still
  * it may happen as a result of emulator bug as well.
  */
-#undef SPICE_RING_PROD_ITEM
-#define SPICE_RING_PROD_ITEM(qxl, r, ret) {                             \
-        uint32_t prod = (r)->prod & SPICE_RING_INDEX_MASK(r);           \
-        if (prod >= ARRAY_SIZE((r)->items)) {                           \
-            qxl_set_guest_bug(qxl, "SPICE_RING_PROD_ITEM indices mismatch " \
-                          "%u >= %zu", prod, ARRAY_SIZE((r)->items));   \
-            ret = NULL;                                                 \
-        } else {                                                        \
-            ret = &(r)->items[prod].el;                                 \
-        }                                                               \
+#define SPICE_RING_GET_CHECK(qxl, r, field) ({                          \
+    field = (r)->field & SPICE_RING_INDEX_MASK(r);                      \
+    bool mismatch = field >= ARRAY_SIZE((r)->items);                    \
+    if (mismatch) {                                                     \
+        qxl_set_guest_bug(qxl, "SPICE_RING_GET %s indices mismatch "    \
+                          "%u >= %zu", stringify(field), field,         \
+                          ARRAY_SIZE((r)->items));                      \
+    }                                                                   \
+    !mismatch;                                                          \
+})
+
+static inline uint64_t
+qxl_release_ring_get_prod(PCIQXLDevice *qxl)
+{
+    struct QXLReleaseRing *ring = &qxl->ram->release_ring;
+    uint32_t prod;
+    bool ok = SPICE_RING_GET_CHECK(qxl, ring, prod);
+    assert(ok);
+
+    return ring->items[prod].el;
+}
+
+static inline bool
+qxl_release_ring_set_prod(PCIQXLDevice *qxl, uint64_t val)
+{
+    struct QXLReleaseRing *ring = &qxl->ram->release_ring;
+    uint32_t prod;
+    bool ok = SPICE_RING_GET_CHECK(qxl, ring, prod);
+    if (ok) {
+        ring->items[prod].el = val;
     }
+    return ok;
+}
 
 #undef SPICE_RING_CONS_ITEM
-#define SPICE_RING_CONS_ITEM(qxl, r, ret) {                             \
-        uint32_t cons = (r)->cons & SPICE_RING_INDEX_MASK(r);           \
-        if (cons >= ARRAY_SIZE((r)->items)) {                           \
-            qxl_set_guest_bug(qxl, "SPICE_RING_CONS_ITEM indices mismatch " \
-                          "%u >= %zu", cons, ARRAY_SIZE((r)->items));   \
-            ret = NULL;                                                 \
-        } else {                                                        \
-            ret = &(r)->items[cons].el;                                 \
-        }                                                               \
-    }
+#define SPICE_RING_CONS_ITEM(qxl, r, ret) {     \
+    uint32_t cons;                              \
+    if (!SPICE_RING_GET_CHECK(qxl, r, cons)) {  \
+        ret = NULL;                             \
+    } else {                                    \
+        ret = &(r)->items[cons].el;             \
+    }                                           \
+}
 
 #undef ALIGN
 #define ALIGN(a, b) (((a) + ((b) - 1)) & ~((b) - 1))
@@ -414,7 +434,6 @@ static void init_qxl_rom(PCIQXLDevice *d)
 static void init_qxl_ram(PCIQXLDevice *d)
 {
     uint8_t *buf;
-    uint64_t *item;
 
     buf = d->vga.vram_ptr;
     d->ram = (QXLRam *)(buf + le32_to_cpu(d->shadow_rom.ram_header_offset));
@@ -426,9 +445,9 @@ static void init_qxl_ram(PCIQXLDevice *d)
     SPICE_RING_INIT(&d->ram->cmd_ring);
     SPICE_RING_INIT(&d->ram->cursor_ring);
     SPICE_RING_INIT(&d->ram->release_ring);
-    SPICE_RING_PROD_ITEM(d, &d->ram->release_ring, item);
-    assert(item);
-    *item = 0;
+    if (!qxl_release_ring_set_prod(d, 0)) {
+        g_assert_not_reached();
+    }
     qxl_ring_set_dirty(d);
 }
 
@@ -732,7 +751,6 @@ static int interface_req_cmd_notification(QXLInstance *sin)
 static inline void qxl_push_free_res(PCIQXLDevice *d, int flush)
 {
     QXLReleaseRing *ring = &d->ram->release_ring;
-    uint64_t *item;
     int notify;
 
 #define QXL_FREE_BUNCH_SIZE 32
@@ -759,11 +777,9 @@ static inline void qxl_push_free_res(PCIQXLDevice *d, int flush)
     if (notify) {
         qxl_send_events(d, QXL_INTERRUPT_DISPLAY);
     }
-    SPICE_RING_PROD_ITEM(d, ring, item);
-    if (!item) {
+    if (!qxl_release_ring_set_prod(d, 0)) {
         return;
     }
-    *item = 0;
     d->num_free_res = 0;
     d->last_release = NULL;
     qxl_ring_set_dirty(d);
@@ -775,7 +791,8 @@ static void interface_release_resource(QXLInstance *sin,
 {
     PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
     QXLReleaseRing *ring;
-    uint64_t *item, id;
+    uint32_t prod;
+    uint64_t id;
 
     if (ext.group_id == MEMSLOT_GROUP_HOST) {
         /* host group -> vga mode update request */
@@ -792,16 +809,16 @@ static void interface_release_resource(QXLInstance *sin,
      * pci bar 0, $command.release_info
      */
     ring = &qxl->ram->release_ring;
-    SPICE_RING_PROD_ITEM(qxl, ring, item);
-    if (!item) {
+
+    if (!SPICE_RING_GET_CHECK(qxl, ring, prod)) {
         return;
     }
-    if (*item == 0) {
+    if (qxl_release_ring_get_prod(qxl) == 0) {
         /* stick head into the ring */
         id = ext.info->id;
         ext.info->next = 0;
         qxl_ram_set_dirty(qxl, &ext.info->next);
-        *item = id;
+        qxl_release_ring_set_prod(qxl, id);
         qxl_ring_set_dirty(qxl);
     } else {
         /* append item to the list */
-- 
2.21.0.196.g041f5ea1cf


Re: [Qemu-devel] [PATCH] qxl: fix -Waddress-of-packed-member
Posted by Philippe Mathieu-Daudé 4 years, 10 months ago
On 4/8/19 10:12 PM, Marc-André Lureau wrote:
> The GCC9 compiler complains about QXL code that takes the address of
> members of the 'struct QXLReleaseRing' which is marked packed:
> 
>   CC      hw/display/qxl.o
> /home/elmarco/src/qemu/hw/display/qxl.c: In function ‘init_qxl_ram’:
> /home/elmarco/src/qemu/hw/display/qxl.c:50:19: warning: taking address of packed member of ‘struct QXLReleaseRing_ring_el’ may result in an unaligned pointer value [-Waddress-of-packed-member]
>    50 |             ret = &(r)->items[prod].el;                                 \
>       |                   ^~~~~~~~~~~~~~~~~~~~
> /home/elmarco/src/qemu/hw/display/qxl.c:429:5: note: in expansion of macro ‘SPICE_RING_PROD_ITEM’
>   429 |     SPICE_RING_PROD_ITEM(d, &d->ram->release_ring, item);
>       |     ^~~~~~~~~~~~~~~~~~~~
> /home/elmarco/src/qemu/hw/display/qxl.c: In function ‘qxl_push_free_res’:
> /home/elmarco/src/qemu/hw/display/qxl.c:50:19: warning: taking address of packed member of ‘struct QXLReleaseRing_ring_el’ may result in an unaligned pointer value [-Waddress-of-packed-member]
>    50 |             ret = &(r)->items[prod].el;                                 \
>       |                   ^~~~~~~~~~~~~~~~~~~~
> /home/elmarco/src/qemu/hw/display/qxl.c:762:5: note: in expansion of macro ‘SPICE_RING_PROD_ITEM’
>   762 |     SPICE_RING_PROD_ITEM(d, ring, item);
>       |     ^~~~~~~~~~~~~~~~~~~~
> /home/elmarco/src/qemu/hw/display/qxl.c: In function ‘interface_release_resource’:
> /home/elmarco/src/qemu/hw/display/qxl.c:50:19: warning: taking address of packed member of ‘struct QXLReleaseRing_ring_el’ may result in an unaligned pointer value [-Waddress-of-packed-member]
>    50 |             ret = &(r)->items[prod].el;                                 \
>       |                   ^~~~~~~~~~~~~~~~~~~~
> /home/elmarco/src/qemu/hw/display/qxl.c:795:5: note: in expansion of macro ‘SPICE_RING_PROD_ITEM’
>   795 |     SPICE_RING_PROD_ITEM(qxl, ring, item);
>       |     ^~~~~~~~~~~~~~~~~~~~
> 
> Replace pointer usage by direct structure/array access instead.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  hw/display/qxl.c | 83 +++++++++++++++++++++++++++++-------------------
>  1 file changed, 50 insertions(+), 33 deletions(-)
> 
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index c8ce5781e0..12d83dd6f1 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -39,29 +39,49 @@
>   * abort we just qxl_set_guest_bug and set the return to NULL. Still
>   * it may happen as a result of emulator bug as well.
>   */
> -#undef SPICE_RING_PROD_ITEM
> -#define SPICE_RING_PROD_ITEM(qxl, r, ret) {                             \
> -        uint32_t prod = (r)->prod & SPICE_RING_INDEX_MASK(r);           \
> -        if (prod >= ARRAY_SIZE((r)->items)) {                           \
> -            qxl_set_guest_bug(qxl, "SPICE_RING_PROD_ITEM indices mismatch " \
> -                          "%u >= %zu", prod, ARRAY_SIZE((r)->items));   \
> -            ret = NULL;                                                 \
> -        } else {                                                        \
> -            ret = &(r)->items[prod].el;                                 \
> -        }                                                               \
> +#define SPICE_RING_GET_CHECK(qxl, r, field) ({                          \
> +    field = (r)->field & SPICE_RING_INDEX_MASK(r);                      \
> +    bool mismatch = field >= ARRAY_SIZE((r)->items);                    \
> +    if (mismatch) {                                                     \
> +        qxl_set_guest_bug(qxl, "SPICE_RING_GET %s indices mismatch "    \
> +                          "%u >= %zu", stringify(field), field,         \
> +                          ARRAY_SIZE((r)->items));                      \
> +    }                                                                   \
> +    !mismatch;                                                          \
> +})
> +
> +static inline uint64_t
> +qxl_release_ring_get_prod(PCIQXLDevice *qxl)
> +{
> +    struct QXLReleaseRing *ring = &qxl->ram->release_ring;
> +    uint32_t prod;
> +    bool ok = SPICE_RING_GET_CHECK(qxl, ring, prod);
> +    assert(ok);
> +
> +    return ring->items[prod].el;
> +}
> +
> +static inline bool
> +qxl_release_ring_set_prod(PCIQXLDevice *qxl, uint64_t val)
> +{
> +    struct QXLReleaseRing *ring = &qxl->ram->release_ring;
> +    uint32_t prod;
> +    bool ok = SPICE_RING_GET_CHECK(qxl, ring, prod);
> +    if (ok) {
> +        ring->items[prod].el = val;
>      }
> +    return ok;
> +}
>  
>  #undef SPICE_RING_CONS_ITEM
> -#define SPICE_RING_CONS_ITEM(qxl, r, ret) {                             \
> -        uint32_t cons = (r)->cons & SPICE_RING_INDEX_MASK(r);           \
> -        if (cons >= ARRAY_SIZE((r)->items)) {                           \
> -            qxl_set_guest_bug(qxl, "SPICE_RING_CONS_ITEM indices mismatch " \
> -                          "%u >= %zu", cons, ARRAY_SIZE((r)->items));   \
> -            ret = NULL;                                                 \
> -        } else {                                                        \
> -            ret = &(r)->items[cons].el;                                 \
> -        }                                                               \
> -    }
> +#define SPICE_RING_CONS_ITEM(qxl, r, ret) {     \
> +    uint32_t cons;                              \
> +    if (!SPICE_RING_GET_CHECK(qxl, r, cons)) {  \
> +        ret = NULL;                             \
> +    } else {                                    \
> +        ret = &(r)->items[cons].el;             \
> +    }                                           \
> +}
>  
>  #undef ALIGN
>  #define ALIGN(a, b) (((a) + ((b) - 1)) & ~((b) - 1))
> @@ -414,7 +434,6 @@ static void init_qxl_rom(PCIQXLDevice *d)
>  static void init_qxl_ram(PCIQXLDevice *d)
>  {
>      uint8_t *buf;
> -    uint64_t *item;
>  
>      buf = d->vga.vram_ptr;
>      d->ram = (QXLRam *)(buf + le32_to_cpu(d->shadow_rom.ram_header_offset));
> @@ -426,9 +445,9 @@ static void init_qxl_ram(PCIQXLDevice *d)
>      SPICE_RING_INIT(&d->ram->cmd_ring);
>      SPICE_RING_INIT(&d->ram->cursor_ring);
>      SPICE_RING_INIT(&d->ram->release_ring);
> -    SPICE_RING_PROD_ITEM(d, &d->ram->release_ring, item);
> -    assert(item);
> -    *item = 0;
> +    if (!qxl_release_ring_set_prod(d, 0)) {
> +        g_assert_not_reached();
> +    }
>      qxl_ring_set_dirty(d);
>  }
>  
> @@ -732,7 +751,6 @@ static int interface_req_cmd_notification(QXLInstance *sin)
>  static inline void qxl_push_free_res(PCIQXLDevice *d, int flush)
>  {
>      QXLReleaseRing *ring = &d->ram->release_ring;
> -    uint64_t *item;
>      int notify;
>  
>  #define QXL_FREE_BUNCH_SIZE 32
> @@ -759,11 +777,9 @@ static inline void qxl_push_free_res(PCIQXLDevice *d, int flush)
>      if (notify) {
>          qxl_send_events(d, QXL_INTERRUPT_DISPLAY);
>      }
> -    SPICE_RING_PROD_ITEM(d, ring, item);
> -    if (!item) {
> +    if (!qxl_release_ring_set_prod(d, 0)) {
>          return;
>      }
> -    *item = 0;
>      d->num_free_res = 0;
>      d->last_release = NULL;
>      qxl_ring_set_dirty(d);
> @@ -775,7 +791,8 @@ static void interface_release_resource(QXLInstance *sin,
>  {
>      PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
>      QXLReleaseRing *ring;
> -    uint64_t *item, id;
> +    uint32_t prod;
> +    uint64_t id;
>  
>      if (ext.group_id == MEMSLOT_GROUP_HOST) {
>          /* host group -> vga mode update request */
> @@ -792,16 +809,16 @@ static void interface_release_resource(QXLInstance *sin,
>       * pci bar 0, $command.release_info
>       */
>      ring = &qxl->ram->release_ring;
> -    SPICE_RING_PROD_ITEM(qxl, ring, item);
> -    if (!item) {
> +
> +    if (!SPICE_RING_GET_CHECK(qxl, ring, prod)) {
>          return;
>      }
> -    if (*item == 0) {
> +    if (qxl_release_ring_get_prod(qxl) == 0) {
>          /* stick head into the ring */
>          id = ext.info->id;
>          ext.info->next = 0;
>          qxl_ram_set_dirty(qxl, &ext.info->next);
> -        *item = id;
> +        qxl_release_ring_set_prod(qxl, id);
>          qxl_ring_set_dirty(qxl);
>      } else {
>          /* append item to the list */
>