[Qemu-devel] migration: Fix 32 bit compiler errors

[Qemu-devel] migration: Fix 32 bit compiler errors

[Stefan Weil]

Obviously that code was never before compiled on 32 bit hosts.
The RDMA API uses lots of uint64_t values and the code casts them
to and from pointers.

I tried to fix the compilation but did not run any runtime tests.

I think the first two patches are trivial, but the third one might
not be trivial, so please review it carefully.

Regards
Stefan

[PATCH 1/3] migration: Fix coding style (whitespace issues)
[PATCH 2/3] migration: Fix some 32 bit compiler errors
[PATCH 3/3] migration: Fix remaining 32 bit compiler errors

[Qemu-devel] [PATCH 1/3] migration: Fix coding style (whitespace issues)

[Stefan Weil]

* Remove trailing whitespace (fixes 9 errors from checkpatch.pl).
  One comment line was longer than 80 characters, so wrap it
  and fix a typo, too.
* Replace tabs by blanks (fixes 1 error).

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---

My editor automatically removes trailing whitespace, so before fixing code,
I had to fix the coding style.

 migration/rdma.c |   22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index 6bee30c..67c5701 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -703,7 +703,7 @@ static void qemu_rdma_dump_id(const char *who, struct ibv_context *verbs)
                 verbs->device->ibdev_path,
                 port.link_layer,
                 (port.link_layer == IBV_LINK_LAYER_INFINIBAND) ? "Infiniband" :
-                 ((port.link_layer == IBV_LINK_LAYER_ETHERNET) 
+                 ((port.link_layer == IBV_LINK_LAYER_ETHERNET)
                     ? "Ethernet" : "Unknown"));
 }
 
@@ -738,7 +738,7 @@ static void qemu_rdma_dump_gid(const char *who, struct rdma_cm_id *id)
  * and validate what time of hardware it is.
  *
  * Unfortunately, this puts the user in a fix:
- * 
+ *
  *  If the source VM connects with an IPv4 address without knowing that the
  *  destination has bound to '[::]' the migration will unconditionally fail
  *  unless the management software is explicitly listening on the the IPv4
@@ -746,13 +746,13 @@ static void qemu_rdma_dump_gid(const char *who, struct rdma_cm_id *id)
  *
  *  If the source VM connects with an IPv6 address, then we're OK because we can
  *  throw an error on the source (and similarly on the destination).
- * 
+ *
  *  But in mixed environments, this will be broken for a while until it is fixed
  *  inside linux.
  *
  * We do provide a *tiny* bit of help in this function: We can list all of the
  * devices in the system and check to see if all the devices are RoCE or
- * Infiniband. 
+ * Infiniband.
  *
  * If we detect that we have a *pure* RoCE environment, then we can safely
  * thrown an error even if the management software has specified '[::]' as the
@@ -771,17 +771,17 @@ static int qemu_rdma_broken_ipv6_kernel(Error **errp, struct ibv_context *verbs)
     /* This bug only exists in linux, to our knowledge. */
 #ifdef CONFIG_LINUX
 
-    /* 
+    /*
      * Verbs are only NULL if management has bound to '[::]'.
-     * 
+     *
      * Let's iterate through all the devices and see if there any pure IB
      * devices (non-ethernet).
-     * 
+     *
      * If not, then we can safely proceed with the migration.
      * Otherwise, there are no guarantees until the bug is fixed in linux.
      */
     if (!verbs) {
-	    int num_devices, x;
+        int num_devices, x;
         struct ibv_device ** dev_list = ibv_get_device_list(&num_devices);
         bool roce_found = false;
         bool ib_found = false;
@@ -826,8 +826,8 @@ static int qemu_rdma_broken_ipv6_kernel(Error **errp, struct ibv_context *verbs)
 
     /*
      * If we have a verbs context, that means that some other than '[::]' was
-     * used by the management software for binding. In which case we can actually 
-     * warn the user about a potential broken kernel;
+     * used by the management software for binding. In which case we can
+     * actually warn the user about a potentially broken kernel.
      */
 
     /* IB ports start with 1, not 0 */
@@ -2421,7 +2421,7 @@ static int qemu_rdma_dest_init(RDMAContext *rdma, Error **errp)
                         continue;
                     }
                 }
-                    
+
                 goto listen;
             }
         }
-- 
1.7.10.4

[Qemu-devel] [PATCH 2/3] migration: Fix some 32 bit compiler errors

[Stefan Weil]

The current code won't compile on 32 bit hosts because there are lots
of type casts between pointers and 64 bit integers.

Fix some of them.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 migration/rdma.c |   23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index 67c5701..1512460 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -1104,7 +1104,7 @@ static int qemu_rdma_search_ram_block(RDMAContext *rdma,
  * to perform the actual RDMA operation.
  */
 static int qemu_rdma_register_and_get_keys(RDMAContext *rdma,
-        RDMALocalBlock *block, uint8_t *host_addr,
+        RDMALocalBlock *block, uintptr_t host_addr,
         uint32_t *lkey, uint32_t *rkey, int chunk,
         uint8_t *chunk_start, uint8_t *chunk_end)
 {
@@ -1141,11 +1141,12 @@ static int qemu_rdma_register_and_get_keys(RDMAContext *rdma,
         if (!block->pmr[chunk]) {
             perror("Failed to register chunk!");
             fprintf(stderr, "Chunk details: block: %d chunk index %d"
-                            " start %" PRIu64 " end %" PRIu64 " host %" PRIu64
-                            " local %" PRIu64 " registrations: %d\n",
-                            block->index, chunk, (uint64_t) chunk_start,
-                            (uint64_t) chunk_end, (uint64_t) host_addr,
-                            (uint64_t) block->local_host_addr,
+                            " start %" PRIuPTR " end %" PRIuPTR
+                            " host %" PRIuPTR
+                            " local %" PRIuPTR " registrations: %d\n",
+                            block->index, chunk, (uintptr_t)chunk_start,
+                            (uintptr_t)chunk_end, host_addr,
+                            (uintptr_t)block->local_host_addr,
                             rdma->total_registrations);
             return -1;
         }
@@ -1932,8 +1933,7 @@ retry:
             }
 
             /* try to overlap this single registration with the one we sent. */
-            if (qemu_rdma_register_and_get_keys(rdma, block,
-                                                (uint8_t *) sge.addr,
+            if (qemu_rdma_register_and_get_keys(rdma, block, sge.addr,
                                                 &sge.lkey, NULL, chunk,
                                                 chunk_start, chunk_end)) {
                 error_report("cannot get lkey");
@@ -1952,8 +1952,7 @@ retry:
             block->remote_host_addr = reg_result->host_addr;
         } else {
             /* already registered before */
-            if (qemu_rdma_register_and_get_keys(rdma, block,
-                                                (uint8_t *)sge.addr,
+            if (qemu_rdma_register_and_get_keys(rdma, block, sge.addr,
                                                 &sge.lkey, NULL, chunk,
                                                 chunk_start, chunk_end)) {
                 error_report("cannot get lkey!");
@@ -1965,7 +1964,7 @@ retry:
     } else {
         send_wr.wr.rdma.rkey = block->remote_rkey;
 
-        if (qemu_rdma_register_and_get_keys(rdma, block, (uint8_t *)sge.addr,
+        if (qemu_rdma_register_and_get_keys(rdma, block, sge.addr,
                                                      &sge.lkey, NULL, chunk,
                                                      chunk_start, chunk_end)) {
             error_report("cannot get lkey!");
@@ -3036,7 +3035,7 @@ static int qemu_rdma_registration_handle(QEMUFile *f, void *opaque,
                 chunk_start = ram_chunk_start(block, chunk);
                 chunk_end = ram_chunk_end(block, chunk + reg->chunks);
                 if (qemu_rdma_register_and_get_keys(rdma, block,
-                            (uint8_t *)host_addr, NULL, &reg_result->rkey,
+                            (uintptr_t)host_addr, NULL, &reg_result->rkey,
                             chunk, chunk_start, chunk_end)) {
                     error_report("cannot get rkey");
                     ret = -EINVAL;
-- 
1.7.10.4

[Qemu-devel] [PATCH 3/3] migration: Fix remaining 32 bit compiler errors

[Stefan Weil]

Fix type casts between pointers and 64 bit integers.
Now 32 bit builds are possible again.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 migration/rdma.c |   57 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 31 insertions(+), 26 deletions(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index 1512460..a68f1f1 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -493,8 +493,8 @@ static inline uint64_t ram_chunk_index(const uint8_t *start,
 static inline uint8_t *ram_chunk_start(const RDMALocalBlock *rdma_ram_block,
                                        uint64_t i)
 {
-    return (uint8_t *) (((uintptr_t) rdma_ram_block->local_host_addr)
-                                    + (i << RDMA_REG_CHUNK_SHIFT));
+    return (uint8_t *)(uintptr_t)(rdma_ram_block->local_host_addr +
+                                  (i << RDMA_REG_CHUNK_SHIFT));
 }
 
 static inline uint8_t *ram_chunk_end(const RDMALocalBlock *rdma_ram_block,
@@ -515,7 +515,7 @@ static int __qemu_rdma_add_block(RDMAContext *rdma, void *host_addr,
 {
     RDMALocalBlocks *local = &rdma->local_ram_blocks;
     RDMALocalBlock *block = g_hash_table_lookup(rdma->blockmap,
-        (void *) block_offset);
+        (void *)(uintptr_t)block_offset);
     RDMALocalBlock *old = local->block;
 
     assert(block == NULL);
@@ -526,9 +526,11 @@ static int __qemu_rdma_add_block(RDMAContext *rdma, void *host_addr,
         int x;
 
         for (x = 0; x < local->nb_blocks; x++) {
-            g_hash_table_remove(rdma->blockmap, (void *)old[x].offset);
-            g_hash_table_insert(rdma->blockmap, (void *)old[x].offset,
-                                                &local->block[x]);
+            g_hash_table_remove(rdma->blockmap,
+                                (void *)(uintptr_t)old[x].offset);
+            g_hash_table_insert(rdma->blockmap,
+                                (void *)(uintptr_t)old[x].offset,
+                                &local->block[x]);
         }
         memcpy(local->block, old, sizeof(RDMALocalBlock) * local->nb_blocks);
         g_free(old);
@@ -549,12 +551,12 @@ static int __qemu_rdma_add_block(RDMAContext *rdma, void *host_addr,
 
     block->is_ram_block = local->init ? false : true;
 
-    g_hash_table_insert(rdma->blockmap, (void *) block_offset, block);
+    g_hash_table_insert(rdma->blockmap, (void *)(uintptr_t)block_offset, block);
 
     trace___qemu_rdma_add_block(local->nb_blocks,
-                           (uint64_t) block->local_host_addr, block->offset,
+                           (uintptr_t)block->local_host_addr, block->offset,
                            block->length,
-                           (uint64_t) (block->local_host_addr + block->length),
+                           (uintptr_t)(block->local_host_addr + block->length),
                            BITS_TO_LONGS(block->nb_chunks) *
                                sizeof(unsigned long) * 8,
                            block->nb_chunks);
@@ -595,7 +597,7 @@ static int qemu_rdma_init_ram_blocks(RDMAContext *rdma)
     return 0;
 }
 
-static int __qemu_rdma_delete_block(RDMAContext *rdma, ram_addr_t block_offset)
+static int __qemu_rdma_delete_block(RDMAContext *rdma, uintptr_t block_offset)
 {
     RDMALocalBlocks *local = &rdma->local_ram_blocks;
     RDMALocalBlock *block = g_hash_table_lookup(rdma->blockmap,
@@ -635,7 +637,7 @@ static int __qemu_rdma_delete_block(RDMAContext *rdma, ram_addr_t block_offset)
     block->remote_keys = NULL;
 
     for (x = 0; x < local->nb_blocks; x++) {
-        g_hash_table_remove(rdma->blockmap, (void *)old[x].offset);
+        g_hash_table_remove(rdma->blockmap, (void *)(uintptr_t)old[x].offset);
     }
 
     if (local->nb_blocks > 1) {
@@ -658,9 +660,9 @@ static int __qemu_rdma_delete_block(RDMAContext *rdma, ram_addr_t block_offset)
     }
 
     trace___qemu_rdma_delete_block(local->nb_blocks,
-                           (uint64_t)block->local_host_addr,
+                           (uintptr_t)block->local_host_addr,
                            block->offset, block->length,
-                           (uint64_t)(block->local_host_addr + block->length),
+                           (uintptr_t)(block->local_host_addr + block->length),
                            BITS_TO_LONGS(block->nb_chunks) *
                                sizeof(unsigned long) * 8, block->nb_chunks);
 
@@ -670,8 +672,9 @@ static int __qemu_rdma_delete_block(RDMAContext *rdma, ram_addr_t block_offset)
 
     if (local->nb_blocks) {
         for (x = 0; x < local->nb_blocks; x++) {
-            g_hash_table_insert(rdma->blockmap, (void *)local->block[x].offset,
-                                                &local->block[x]);
+            g_hash_table_insert(rdma->blockmap,
+                                (void *)(uintptr_t)local->block[x].offset,
+                                &local->block[x]);
         }
     }
 
@@ -1076,7 +1079,7 @@ static int qemu_rdma_reg_whole_ram_blocks(RDMAContext *rdma)
  * This search cannot fail or the migration will fail.
  */
 static int qemu_rdma_search_ram_block(RDMAContext *rdma,
-                                      uint64_t block_offset,
+                                      uintptr_t block_offset,
                                       uint64_t offset,
                                       uint64_t length,
                                       uint64_t *block_index,
@@ -1380,8 +1383,8 @@ static uint64_t qemu_rdma_poll(RDMAContext *rdma, uint64_t *wr_id_out,
         RDMALocalBlock *block = &(rdma->local_ram_blocks.block[index]);
 
         trace_qemu_rdma_poll_write(print_wrid(wr_id), wr_id, rdma->nb_sent,
-                 index, chunk,
-                 block->local_host_addr, (void *)block->remote_host_addr);
+                                   index, chunk, block->local_host_addr,
+                                   (void *)(uintptr_t)block->remote_host_addr);
 
         clear_bit(chunk, block->transit_bitmap);
 
@@ -1524,7 +1527,7 @@ static int qemu_rdma_post_send_control(RDMAContext *rdma, uint8_t *buf,
     RDMAWorkRequestData *wr = &rdma->wr_data[RDMA_WRID_CONTROL];
     struct ibv_send_wr *bad_wr;
     struct ibv_sge sge = {
-                           .addr = (uint64_t)(wr->control),
+                           .addr = (uintptr_t)(wr->control),
                            .length = head->len + sizeof(RDMAControlHeader),
                            .lkey = wr->control_mr->lkey,
                          };
@@ -1578,7 +1581,7 @@ static int qemu_rdma_post_recv_control(RDMAContext *rdma, int idx)
 {
     struct ibv_recv_wr *bad_wr;
     struct ibv_sge sge = {
-                            .addr = (uint64_t)(rdma->wr_data[idx].control),
+                            .addr = (uintptr_t)(rdma->wr_data[idx].control),
                             .length = RDMA_CONTROL_MAX_BUFFER,
                             .lkey = rdma->wr_data[idx].control_mr->lkey,
                          };
@@ -1825,11 +1828,12 @@ static int qemu_rdma_write_one(QEMUFile *f, RDMAContext *rdma,
                              };
 
 retry:
-    sge.addr = (uint64_t)(block->local_host_addr +
+    sge.addr = (uintptr_t)(block->local_host_addr +
                             (current_addr - block->offset));
     sge.length = length;
 
-    chunk = ram_chunk_index(block->local_host_addr, (uint8_t *) sge.addr);
+    chunk = ram_chunk_index(block->local_host_addr,
+                            (uint8_t *)(uintptr_t)sge.addr);
     chunk_start = ram_chunk_start(block, chunk);
 
     if (block->is_ram_block) {
@@ -1882,8 +1886,9 @@ retry:
              * memset() + madvise() the entire chunk without RDMA.
              */
 
-            if (can_use_buffer_find_nonzero_offset((void *)sge.addr, length)
-                   && buffer_find_nonzero_offset((void *)sge.addr,
+            if (can_use_buffer_find_nonzero_offset((void *)(uintptr_t)sge.addr,
+                                                   length)
+                   && buffer_find_nonzero_offset((void *)(uintptr_t)sge.addr,
                                                     length) == length) {
                 RDMACompress comp = {
                                         .offset = current_addr,
@@ -2978,7 +2983,7 @@ static int qemu_rdma_registration_handle(QEMUFile *f, void *opaque,
              */
             for (i = 0; i < local->nb_blocks; i++) {
                 rdma->block[i].remote_host_addr =
-                    (uint64_t)(local->block[i].local_host_addr);
+                    (uintptr_t)(local->block[i].local_host_addr);
 
                 if (rdma->pin_all) {
                     rdma->block[i].remote_rkey = local->block[i].mr->rkey;
@@ -3042,7 +3047,7 @@ static int qemu_rdma_registration_handle(QEMUFile *f, void *opaque,
                     goto out;
                 }
 
-                reg_result->host_addr = (uint64_t) block->local_host_addr;
+                reg_result->host_addr = (uintptr_t)block->local_host_addr;
 
                 trace_qemu_rdma_registration_handle_register_rkey(
                                                            reg_result->rkey);
-- 
1.7.10.4

Re: [Qemu-devel] migration: Fix 32 bit compiler errors

[Amit Shah]

On (Sat) 28 Feb 2015 [19:09:40], Stefan Weil wrote:
> Obviously that code was never before compiled on 32 bit hosts.
> The RDMA API uses lots of uint64_t values and the code casts them
> to and from pointers.
> 
> I tried to fix the compilation but did not run any runtime tests.
> 
> I think the first two patches are trivial, but the third one might
> not be trivial, so please review it carefully.

Interestingly Juan found this out recently as well, and Dave was
taking a look.  CC'ing Dave.

Thanks,

		Amit

Re: [Qemu-devel] [PATCH 1/3] migration: Fix coding style (whitespace issues)

[Michael Tokarev]

28.02.2015 21:09, Stefan Weil wrote:
> * Remove trailing whitespace (fixes 9 errors from checkpatch.pl).
>   One comment line was longer than 80 characters, so wrap it
>   and fix a typo, too.
> * Replace tabs by blanks (fixes 1 error).

This hunk:

> @@ -2421,7 +2421,7 @@ static int qemu_rdma_dest_init(RDMAContext *rdma, Error **errp)
>                          continue;
>                      }
>                  }
> -                    
> +
>                  goto listen;
>              }


clashes with my earlier attemt to clean up this code, I rewrote
this loop a bit to avoid this "twisted" usage of gotos.
I'll apply this patch without this change.

Thanks,

/mjt

Re: [Qemu-devel] [PATCH 1/3] migration: Fix coding style (whitespace issues)

[Dr. David Alan Gilbert]

* Stefan Weil (sw@weilnetz.de) wrote:
> * Remove trailing whitespace (fixes 9 errors from checkpatch.pl).
>   One comment line was longer than 80 characters, so wrap it
>   and fix a typo, too.
> * Replace tabs by blanks (fixes 1 error).
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>

Needs merging with Michael's fixes, but otherwise:

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
> 
> My editor automatically removes trailing whitespace, so before fixing code,
> I had to fix the coding style.
> 
>  migration/rdma.c |   22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 6bee30c..67c5701 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -703,7 +703,7 @@ static void qemu_rdma_dump_id(const char *who, struct ibv_context *verbs)
>                  verbs->device->ibdev_path,
>                  port.link_layer,
>                  (port.link_layer == IBV_LINK_LAYER_INFINIBAND) ? "Infiniband" :
> -                 ((port.link_layer == IBV_LINK_LAYER_ETHERNET) 
> +                 ((port.link_layer == IBV_LINK_LAYER_ETHERNET)
>                      ? "Ethernet" : "Unknown"));
>  }
>  
> @@ -738,7 +738,7 @@ static void qemu_rdma_dump_gid(const char *who, struct rdma_cm_id *id)
>   * and validate what time of hardware it is.
>   *
>   * Unfortunately, this puts the user in a fix:
> - * 
> + *
>   *  If the source VM connects with an IPv4 address without knowing that the
>   *  destination has bound to '[::]' the migration will unconditionally fail
>   *  unless the management software is explicitly listening on the the IPv4
> @@ -746,13 +746,13 @@ static void qemu_rdma_dump_gid(const char *who, struct rdma_cm_id *id)
>   *
>   *  If the source VM connects with an IPv6 address, then we're OK because we can
>   *  throw an error on the source (and similarly on the destination).
> - * 
> + *
>   *  But in mixed environments, this will be broken for a while until it is fixed
>   *  inside linux.
>   *
>   * We do provide a *tiny* bit of help in this function: We can list all of the
>   * devices in the system and check to see if all the devices are RoCE or
> - * Infiniband. 
> + * Infiniband.
>   *
>   * If we detect that we have a *pure* RoCE environment, then we can safely
>   * thrown an error even if the management software has specified '[::]' as the
> @@ -771,17 +771,17 @@ static int qemu_rdma_broken_ipv6_kernel(Error **errp, struct ibv_context *verbs)
>      /* This bug only exists in linux, to our knowledge. */
>  #ifdef CONFIG_LINUX
>  
> -    /* 
> +    /*
>       * Verbs are only NULL if management has bound to '[::]'.
> -     * 
> +     *
>       * Let's iterate through all the devices and see if there any pure IB
>       * devices (non-ethernet).
> -     * 
> +     *
>       * If not, then we can safely proceed with the migration.
>       * Otherwise, there are no guarantees until the bug is fixed in linux.
>       */
>      if (!verbs) {
> -	    int num_devices, x;
> +        int num_devices, x;
>          struct ibv_device ** dev_list = ibv_get_device_list(&num_devices);
>          bool roce_found = false;
>          bool ib_found = false;
> @@ -826,8 +826,8 @@ static int qemu_rdma_broken_ipv6_kernel(Error **errp, struct ibv_context *verbs)
>  
>      /*
>       * If we have a verbs context, that means that some other than '[::]' was
> -     * used by the management software for binding. In which case we can actually 
> -     * warn the user about a potential broken kernel;
> +     * used by the management software for binding. In which case we can
> +     * actually warn the user about a potentially broken kernel.
>       */
>  
>      /* IB ports start with 1, not 0 */
> @@ -2421,7 +2421,7 @@ static int qemu_rdma_dest_init(RDMAContext *rdma, Error **errp)
>                          continue;
>                      }
>                  }
> -                    
> +
>                  goto listen;
>              }
>          }
> -- 
> 1.7.10.4
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/3] migration: Fix some 32 bit compiler errors

[Dr. David Alan Gilbert]

* Stefan Weil (sw@weilnetz.de) wrote:
> The current code won't compile on 32 bit hosts because there are lots
> of type casts between pointers and 64 bit integers.
> 
> Fix some of them.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>

Please route rdma stuff through migration, not -trivial; it's never
trivial to read this code.

> ---
>  migration/rdma.c |   23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 67c5701..1512460 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -1104,7 +1104,7 @@ static int qemu_rdma_search_ram_block(RDMAContext *rdma,
>   * to perform the actual RDMA operation.
>   */
>  static int qemu_rdma_register_and_get_keys(RDMAContext *rdma,
> -        RDMALocalBlock *block, uint8_t *host_addr,
> +        RDMALocalBlock *block, uintptr_t host_addr,
>          uint32_t *lkey, uint32_t *rkey, int chunk,
>          uint8_t *chunk_start, uint8_t *chunk_end)

OK, so 'host_addr' seems to only be used in this function to print debug,
so that should be harmless.

>  {
> @@ -1141,11 +1141,12 @@ static int qemu_rdma_register_and_get_keys(RDMAContext *rdma,
>          if (!block->pmr[chunk]) {
>              perror("Failed to register chunk!");
>              fprintf(stderr, "Chunk details: block: %d chunk index %d"
> -                            " start %" PRIu64 " end %" PRIu64 " host %" PRIu64
> -                            " local %" PRIu64 " registrations: %d\n",
> -                            block->index, chunk, (uint64_t) chunk_start,
> -                            (uint64_t) chunk_end, (uint64_t) host_addr,
> -                            (uint64_t) block->local_host_addr,
> +                            " start %" PRIuPTR " end %" PRIuPTR
> +                            " host %" PRIuPTR
> +                            " local %" PRIuPTR " registrations: %d\n",
> +                            block->index, chunk, (uintptr_t)chunk_start,
> +                            (uintptr_t)chunk_end, host_addr,
> +                            (uintptr_t)block->local_host_addr,

OK, although is there any reason not to use %p for most of those?

>                              rdma->total_registrations);
>              return -1;
>          }
> @@ -1932,8 +1933,7 @@ retry:
>              }
>  
>              /* try to overlap this single registration with the one we sent. */
> -            if (qemu_rdma_register_and_get_keys(rdma, block,
> -                                                (uint8_t *) sge.addr,
> +            if (qemu_rdma_register_and_get_keys(rdma, block, sge.addr,
>                                                  &sge.lkey, NULL, chunk,
>                                                  chunk_start, chunk_end)) {

sge.addr comes from /usr/include/infiniband/verbs.h for me:

struct ibv_sge {
        uint64_t                addr;
        uint32_t                length;
        uint32_t                lkey;
};

and that's the same on both 32 bit and 64 bit hosts (Fedora 21).
I'm confused about why this helps you build 32 bit, since that uint64_t gets
passed to your host_addr that's now a unitptr_t that will be 32bit.

Dave

>                  error_report("cannot get lkey");
> @@ -1952,8 +1952,7 @@ retry:
>              block->remote_host_addr = reg_result->host_addr;
>          } else {
>              /* already registered before */
> -            if (qemu_rdma_register_and_get_keys(rdma, block,
> -                                                (uint8_t *)sge.addr,
> +            if (qemu_rdma_register_and_get_keys(rdma, block, sge.addr,
>                                                  &sge.lkey, NULL, chunk,
>                                                  chunk_start, chunk_end)) {
>                  error_report("cannot get lkey!");
> @@ -1965,7 +1964,7 @@ retry:
>      } else {
>          send_wr.wr.rdma.rkey = block->remote_rkey;
>  
> -        if (qemu_rdma_register_and_get_keys(rdma, block, (uint8_t *)sge.addr,
> +        if (qemu_rdma_register_and_get_keys(rdma, block, sge.addr,
>                                                       &sge.lkey, NULL, chunk,
>                                                       chunk_start, chunk_end)) {
>              error_report("cannot get lkey!");
> @@ -3036,7 +3035,7 @@ static int qemu_rdma_registration_handle(QEMUFile *f, void *opaque,
>                  chunk_start = ram_chunk_start(block, chunk);
>                  chunk_end = ram_chunk_end(block, chunk + reg->chunks);
>                  if (qemu_rdma_register_and_get_keys(rdma, block,
> -                            (uint8_t *)host_addr, NULL, &reg_result->rkey,
> +                            (uintptr_t)host_addr, NULL, &reg_result->rkey,
>                              chunk, chunk_start, chunk_end)) {
>                      error_report("cannot get rkey");
>                      ret = -EINVAL;
> -- 
> 1.7.10.4
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 3/3] migration: Fix remaining 32 bit compiler errors

[Dr. David Alan Gilbert]

* Stefan Weil (sw@weilnetz.de) wrote:
> Fix type casts between pointers and 64 bit integers.
> Now 32 bit builds are possible again.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  migration/rdma.c |   57 +++++++++++++++++++++++++++++-------------------------
>  1 file changed, 31 insertions(+), 26 deletions(-)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 1512460..a68f1f1 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -493,8 +493,8 @@ static inline uint64_t ram_chunk_index(const uint8_t *start,
>  static inline uint8_t *ram_chunk_start(const RDMALocalBlock *rdma_ram_block,
>                                         uint64_t i)
>  {
> -    return (uint8_t *) (((uintptr_t) rdma_ram_block->local_host_addr)
> -                                    + (i << RDMA_REG_CHUNK_SHIFT));
> +    return (uint8_t *)(uintptr_t)(rdma_ram_block->local_host_addr +
> +                                  (i << RDMA_REG_CHUNK_SHIFT));
>  }
>  
>  static inline uint8_t *ram_chunk_end(const RDMALocalBlock *rdma_ram_block,
> @@ -515,7 +515,7 @@ static int __qemu_rdma_add_block(RDMAContext *rdma, void *host_addr,
>  {

This will clash with my 'unbreak dtrace tracing due to double _ in rdma names' that's
going through Stefan's tree.

>      RDMALocalBlocks *local = &rdma->local_ram_blocks;
>      RDMALocalBlock *block = g_hash_table_lookup(rdma->blockmap,
> -        (void *) block_offset);
> +        (void *)(uintptr_t)block_offset);
>      RDMALocalBlock *old = local->block;


>  
>      assert(block == NULL);
> @@ -526,9 +526,11 @@ static int __qemu_rdma_add_block(RDMAContext *rdma, void *host_addr,
>          int x;
>  
>          for (x = 0; x < local->nb_blocks; x++) {
> -            g_hash_table_remove(rdma->blockmap, (void *)old[x].offset);
> -            g_hash_table_insert(rdma->blockmap, (void *)old[x].offset,
> -                                                &local->block[x]);
> +            g_hash_table_remove(rdma->blockmap,
> +                                (void *)(uintptr_t)old[x].offset);
> +            g_hash_table_insert(rdma->blockmap,
> +                                (void *)(uintptr_t)old[x].offset,
> +                                &local->block[x]);
>          }
>          memcpy(local->block, old, sizeof(RDMALocalBlock) * local->nb_blocks);
>          g_free(old);
> @@ -549,12 +551,12 @@ static int __qemu_rdma_add_block(RDMAContext *rdma, void *host_addr,
>  
>      block->is_ram_block = local->init ? false : true;
>  
> -    g_hash_table_insert(rdma->blockmap, (void *) block_offset, block);
> +    g_hash_table_insert(rdma->blockmap, (void *)(uintptr_t)block_offset, block);
>  
>      trace___qemu_rdma_add_block(local->nb_blocks,
> -                           (uint64_t) block->local_host_addr, block->offset,
> +                           (uintptr_t)block->local_host_addr, block->offset,
>                             block->length,
> -                           (uint64_t) (block->local_host_addr + block->length),
> +                           (uintptr_t)(block->local_host_addr + block->length),
>                             BITS_TO_LONGS(block->nb_chunks) *
>                                 sizeof(unsigned long) * 8,
>                             block->nb_chunks);
> @@ -595,7 +597,7 @@ static int qemu_rdma_init_ram_blocks(RDMAContext *rdma)
>      return 0;
>  }
>  
> -static int __qemu_rdma_delete_block(RDMAContext *rdma, ram_addr_t block_offset)
> +static int __qemu_rdma_delete_block(RDMAContext *rdma, uintptr_t block_offset)
>  {
>      RDMALocalBlocks *local = &rdma->local_ram_blocks;
>      RDMALocalBlock *block = g_hash_table_lookup(rdma->blockmap,
> @@ -635,7 +637,7 @@ static int __qemu_rdma_delete_block(RDMAContext *rdma, ram_addr_t block_offset)
>      block->remote_keys = NULL;
>  
>      for (x = 0; x < local->nb_blocks; x++) {
> -        g_hash_table_remove(rdma->blockmap, (void *)old[x].offset);
> +        g_hash_table_remove(rdma->blockmap, (void *)(uintptr_t)old[x].offset);
>      }
>  
>      if (local->nb_blocks > 1) {
> @@ -658,9 +660,9 @@ static int __qemu_rdma_delete_block(RDMAContext *rdma, ram_addr_t block_offset)
>      }
>  
>      trace___qemu_rdma_delete_block(local->nb_blocks,
> -                           (uint64_t)block->local_host_addr,
> +                           (uintptr_t)block->local_host_addr,
>                             block->offset, block->length,
> -                           (uint64_t)(block->local_host_addr + block->length),
> +                           (uintptr_t)(block->local_host_addr + block->length),
>                             BITS_TO_LONGS(block->nb_chunks) *
>                                 sizeof(unsigned long) * 8, block->nb_chunks);
>  
> @@ -670,8 +672,9 @@ static int __qemu_rdma_delete_block(RDMAContext *rdma, ram_addr_t block_offset)
>  
>      if (local->nb_blocks) {
>          for (x = 0; x < local->nb_blocks; x++) {
> -            g_hash_table_insert(rdma->blockmap, (void *)local->block[x].offset,
> -                                                &local->block[x]);
> +            g_hash_table_insert(rdma->blockmap,
> +                                (void *)(uintptr_t)local->block[x].offset,
> +                                &local->block[x]);
>          }
>      }
>  
> @@ -1076,7 +1079,7 @@ static int qemu_rdma_reg_whole_ram_blocks(RDMAContext *rdma)
>   * This search cannot fail or the migration will fail.
>   */
>  static int qemu_rdma_search_ram_block(RDMAContext *rdma,
> -                                      uint64_t block_offset,
> +                                      uintptr_t block_offset,
>                                        uint64_t offset,
>                                        uint64_t length,
>                                        uint64_t *block_index,
> @@ -1380,8 +1383,8 @@ static uint64_t qemu_rdma_poll(RDMAContext *rdma, uint64_t *wr_id_out,
>          RDMALocalBlock *block = &(rdma->local_ram_blocks.block[index]);
>  
>          trace_qemu_rdma_poll_write(print_wrid(wr_id), wr_id, rdma->nb_sent,
> -                 index, chunk,
> -                 block->local_host_addr, (void *)block->remote_host_addr);
> +                                   index, chunk, block->local_host_addr,
> +                                   (void *)(uintptr_t)block->remote_host_addr);

I think that's the wrong fix there; remote_host_addr is actually 64bit because
even if this host is 32bit the other end might be 64bit; I think the right
fix is to change the trace to use a uint64_t for the remote_host_addr.

>          clear_bit(chunk, block->transit_bitmap);
>  
> @@ -1524,7 +1527,7 @@ static int qemu_rdma_post_send_control(RDMAContext *rdma, uint8_t *buf,
>      RDMAWorkRequestData *wr = &rdma->wr_data[RDMA_WRID_CONTROL];
>      struct ibv_send_wr *bad_wr;
>      struct ibv_sge sge = {
> -                           .addr = (uint64_t)(wr->control),
> +                           .addr = (uintptr_t)(wr->control),

No, as before, my belief is that the .addr is a uint64_t.

(Although curiously I did a test and found that gcc lets me pass a uint64_t
to a function that has a uintptr_t parameter on a 32bit machine, but I don't
understand why.)

Dave

>                             .length = head->len + sizeof(RDMAControlHeader),
>                             .lkey = wr->control_mr->lkey,
>                           };
> @@ -1578,7 +1581,7 @@ static int qemu_rdma_post_recv_control(RDMAContext *rdma, int idx)
>  {
>      struct ibv_recv_wr *bad_wr;
>      struct ibv_sge sge = {
> -                            .addr = (uint64_t)(rdma->wr_data[idx].control),
> +                            .addr = (uintptr_t)(rdma->wr_data[idx].control),
>                              .length = RDMA_CONTROL_MAX_BUFFER,
>                              .lkey = rdma->wr_data[idx].control_mr->lkey,
>                           };
> @@ -1825,11 +1828,12 @@ static int qemu_rdma_write_one(QEMUFile *f, RDMAContext *rdma,
>                               };
>  
>  retry:
> -    sge.addr = (uint64_t)(block->local_host_addr +
> +    sge.addr = (uintptr_t)(block->local_host_addr +
>                              (current_addr - block->offset));
>      sge.length = length;
>  
> -    chunk = ram_chunk_index(block->local_host_addr, (uint8_t *) sge.addr);
> +    chunk = ram_chunk_index(block->local_host_addr,
> +                            (uint8_t *)(uintptr_t)sge.addr);
>      chunk_start = ram_chunk_start(block, chunk);
>  
>      if (block->is_ram_block) {
> @@ -1882,8 +1886,9 @@ retry:
>               * memset() + madvise() the entire chunk without RDMA.
>               */
>  
> -            if (can_use_buffer_find_nonzero_offset((void *)sge.addr, length)
> -                   && buffer_find_nonzero_offset((void *)sge.addr,
> +            if (can_use_buffer_find_nonzero_offset((void *)(uintptr_t)sge.addr,
> +                                                   length)
> +                   && buffer_find_nonzero_offset((void *)(uintptr_t)sge.addr,
>                                                      length) == length) {
>                  RDMACompress comp = {
>                                          .offset = current_addr,
> @@ -2978,7 +2983,7 @@ static int qemu_rdma_registration_handle(QEMUFile *f, void *opaque,
>               */
>              for (i = 0; i < local->nb_blocks; i++) {
>                  rdma->block[i].remote_host_addr =
> -                    (uint64_t)(local->block[i].local_host_addr);
> +                    (uintptr_t)(local->block[i].local_host_addr);
>  
>                  if (rdma->pin_all) {
>                      rdma->block[i].remote_rkey = local->block[i].mr->rkey;
> @@ -3042,7 +3047,7 @@ static int qemu_rdma_registration_handle(QEMUFile *f, void *opaque,
>                      goto out;
>                  }
>  
> -                reg_result->host_addr = (uint64_t) block->local_host_addr;
> +                reg_result->host_addr = (uintptr_t)block->local_host_addr;
>  
>                  trace_qemu_rdma_registration_handle_register_rkey(
>                                                             reg_result->rkey);
> -- 
> 1.7.10.4
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 3/3] migration: Fix remaining 32 bit compiler errors

[Stefan Weil]

Please see my comments below.

Am 04.03.2015 um 14:31 schrieb Dr. David Alan Gilbert:
> * Stefan Weil (sw@weilnetz.de) wrote:
>> Fix type casts between pointers and 64 bit integers.
>> Now 32 bit builds are possible again.
>>
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>> ---
>>   migration/rdma.c |   57 +++++++++++++++++++++++++++++-------------------------
>>   1 file changed, 31 insertions(+), 26 deletions(-)
>>
>> diff --git a/migration/rdma.c b/migration/rdma.c
>> index 1512460..a68f1f1 100644
>> --- a/migration/rdma.c
>> +++ b/migration/rdma.c
>> @@ -493,8 +493,8 @@ static inline uint64_t ram_chunk_index(const uint8_t *start,
>>   static inline uint8_t *ram_chunk_start(const RDMALocalBlock *rdma_ram_block,
>>                                          uint64_t i)
>>   {
>> -    return (uint8_t *) (((uintptr_t) rdma_ram_block->local_host_addr)
>> -                                    + (i << RDMA_REG_CHUNK_SHIFT));
>> +    return (uint8_t *)(uintptr_t)(rdma_ram_block->local_host_addr +
>> +                                  (i << RDMA_REG_CHUNK_SHIFT));
>>   }
>>   
>>   static inline uint8_t *ram_chunk_end(const RDMALocalBlock *rdma_ram_block,
>> @@ -515,7 +515,7 @@ static int __qemu_rdma_add_block(RDMAContext *rdma, void *host_addr,
>>   {
> This will clash with my 'unbreak dtrace tracing due to double _ in rdma names' that's
> going through Stefan's tree.

Then this part will have to wait until Stefan's tree was applied.

>
>>       RDMALocalBlocks *local = &rdma->local_ram_blocks;
>>       RDMALocalBlock *block = g_hash_table_lookup(rdma->blockmap,
>> -        (void *) block_offset);
>> +        (void *)(uintptr_t)block_offset);
>>       RDMALocalBlock *old = local->block;
>
>>   
>>       assert(block == NULL);
>> @@ -526,9 +526,11 @@ static int __qemu_rdma_add_block(RDMAContext *rdma, void *host_addr,
>>           int x;
>>   
>>           for (x = 0; x < local->nb_blocks; x++) {
>> -            g_hash_table_remove(rdma->blockmap, (void *)old[x].offset);
>> -            g_hash_table_insert(rdma->blockmap, (void *)old[x].offset,
>> -                                                &local->block[x]);
>> +            g_hash_table_remove(rdma->blockmap,
>> +                                (void *)(uintptr_t)old[x].offset);
>> +            g_hash_table_insert(rdma->blockmap,
>> +                                (void *)(uintptr_t)old[x].offset,
>> +                                &local->block[x]);
>>           }
>>           memcpy(local->block, old, sizeof(RDMALocalBlock) * local->nb_blocks);
>>           g_free(old);
>> @@ -549,12 +551,12 @@ static int __qemu_rdma_add_block(RDMAContext *rdma, void *host_addr,
>>   
>>       block->is_ram_block = local->init ? false : true;
>>   
>> -    g_hash_table_insert(rdma->blockmap, (void *) block_offset, block);
>> +    g_hash_table_insert(rdma->blockmap, (void *)(uintptr_t)block_offset, block);
>>   
>>       trace___qemu_rdma_add_block(local->nb_blocks,
>> -                           (uint64_t) block->local_host_addr, block->offset,
>> +                           (uintptr_t)block->local_host_addr, block->offset,
>>                              block->length,
>> -                           (uint64_t) (block->local_host_addr + block->length),
>> +                           (uintptr_t)(block->local_host_addr + block->length),
>>                              BITS_TO_LONGS(block->nb_chunks) *
>>                                  sizeof(unsigned long) * 8,
>>                              block->nb_chunks);
>> @@ -595,7 +597,7 @@ static int qemu_rdma_init_ram_blocks(RDMAContext *rdma)
>>       return 0;
>>   }
>>   
>> -static int __qemu_rdma_delete_block(RDMAContext *rdma, ram_addr_t block_offset)
>> +static int __qemu_rdma_delete_block(RDMAContext *rdma, uintptr_t block_offset)
>>   {
>>       RDMALocalBlocks *local = &rdma->local_ram_blocks;
>>       RDMALocalBlock *block = g_hash_table_lookup(rdma->blockmap,
>> @@ -635,7 +637,7 @@ static int __qemu_rdma_delete_block(RDMAContext *rdma, ram_addr_t block_offset)
>>       block->remote_keys = NULL;
>>   
>>       for (x = 0; x < local->nb_blocks; x++) {
>> -        g_hash_table_remove(rdma->blockmap, (void *)old[x].offset);
>> +        g_hash_table_remove(rdma->blockmap, (void *)(uintptr_t)old[x].offset);
>>       }
>>   
>>       if (local->nb_blocks > 1) {
>> @@ -658,9 +660,9 @@ static int __qemu_rdma_delete_block(RDMAContext *rdma, ram_addr_t block_offset)
>>       }
>>   
>>       trace___qemu_rdma_delete_block(local->nb_blocks,
>> -                           (uint64_t)block->local_host_addr,
>> +                           (uintptr_t)block->local_host_addr,
>>                              block->offset, block->length,
>> -                           (uint64_t)(block->local_host_addr + block->length),
>> +                           (uintptr_t)(block->local_host_addr + block->length),
>>                              BITS_TO_LONGS(block->nb_chunks) *
>>                                  sizeof(unsigned long) * 8, block->nb_chunks);
>>   
>> @@ -670,8 +672,9 @@ static int __qemu_rdma_delete_block(RDMAContext *rdma, ram_addr_t block_offset)
>>   
>>       if (local->nb_blocks) {
>>           for (x = 0; x < local->nb_blocks; x++) {
>> -            g_hash_table_insert(rdma->blockmap, (void *)local->block[x].offset,
>> -                                                &local->block[x]);
>> +            g_hash_table_insert(rdma->blockmap,
>> +                                (void *)(uintptr_t)local->block[x].offset,
>> +                                &local->block[x]);
>>           }
>>       }
>>   
>> @@ -1076,7 +1079,7 @@ static int qemu_rdma_reg_whole_ram_blocks(RDMAContext *rdma)
>>    * This search cannot fail or the migration will fail.
>>    */
>>   static int qemu_rdma_search_ram_block(RDMAContext *rdma,
>> -                                      uint64_t block_offset,
>> +                                      uintptr_t block_offset,
>>                                         uint64_t offset,
>>                                         uint64_t length,
>>                                         uint64_t *block_index,
>> @@ -1380,8 +1383,8 @@ static uint64_t qemu_rdma_poll(RDMAContext *rdma, uint64_t *wr_id_out,
>>           RDMALocalBlock *block = &(rdma->local_ram_blocks.block[index]);
>>   
>>           trace_qemu_rdma_poll_write(print_wrid(wr_id), wr_id, rdma->nb_sent,
>> -                 index, chunk,
>> -                 block->local_host_addr, (void *)block->remote_host_addr);
>> +                                   index, chunk, block->local_host_addr,
>> +                                   (void *)(uintptr_t)block->remote_host_addr);
> I think that's the wrong fix there; remote_host_addr is actually 64bit because
> even if this host is 32bit the other end might be 64bit; I think the right
> fix is to change the trace to use a uint64_t for the remote_host_addr.

That depends on the kind of output which is wanted.

We can either use %p for pointers, then the output will depend on the 
host's pointer size.
Or we can use %16 PRIu64, then the output will always show 16 digits 
(with at least 8
leading zeros on 32 bit hosts).

>
>>           clear_bit(chunk, block->transit_bitmap);
>>   
>> @@ -1524,7 +1527,7 @@ static int qemu_rdma_post_send_control(RDMAContext *rdma, uint8_t *buf,
>>       RDMAWorkRequestData *wr = &rdma->wr_data[RDMA_WRID_CONTROL];
>>       struct ibv_send_wr *bad_wr;
>>       struct ibv_sge sge = {
>> -                           .addr = (uint64_t)(wr->control),
>> +                           .addr = (uintptr_t)(wr->control),
> No, as before, my belief is that the .addr is a uint64_t.

Yes, because wr->control is a pointer which you want to convert to an 
integer value.
C allows assignments from 32 bit integers to 64 bit integers - the 
compiler will add the
necessary 0 bits.

>
> (Although curiously I did a test and found that gcc lets me pass a uint64_t
> to a function that has a uintptr_t parameter on a 32bit machine, but I don't
> understand why.)
>
> Dave
>
>>                              .length = head->len + sizeof(RDMAControlHeader),
>>                              .lkey = wr->control_mr->lkey,
>>                            };
>> @@ -1578,7 +1581,7 @@ static int qemu_rdma_post_recv_control(RDMAContext *rdma, int idx)
>>   {
>>       struct ibv_recv_wr *bad_wr;
>>       struct ibv_sge sge = {
>> -                            .addr = (uint64_t)(rdma->wr_data[idx].control),
>> +                            .addr = (uintptr_t)(rdma->wr_data[idx].control),
>>                               .length = RDMA_CONTROL_MAX_BUFFER,
>>                               .lkey = rdma->wr_data[idx].control_mr->lkey,
>>                            };
>> @@ -1825,11 +1828,12 @@ static int qemu_rdma_write_one(QEMUFile *f, RDMAContext *rdma,
>>                                };
>>   
>>   retry:
>> -    sge.addr = (uint64_t)(block->local_host_addr +
>> +    sge.addr = (uintptr_t)(block->local_host_addr +
>>                               (current_addr - block->offset));
>>       sge.length = length;
>>   
>> -    chunk = ram_chunk_index(block->local_host_addr, (uint8_t *) sge.addr);
>> +    chunk = ram_chunk_index(block->local_host_addr,
>> +                            (uint8_t *)(uintptr_t)sge.addr);
>>       chunk_start = ram_chunk_start(block, chunk);
>>   
>>       if (block->is_ram_block) {
>> @@ -1882,8 +1886,9 @@ retry:
>>                * memset() + madvise() the entire chunk without RDMA.
>>                */
>>   
>> -            if (can_use_buffer_find_nonzero_offset((void *)sge.addr, length)
>> -                   && buffer_find_nonzero_offset((void *)sge.addr,
>> +            if (can_use_buffer_find_nonzero_offset((void *)(uintptr_t)sge.addr,
>> +                                                   length)
>> +                   && buffer_find_nonzero_offset((void *)(uintptr_t)sge.addr,
>>                                                       length) == length) {
>>                   RDMACompress comp = {
>>                                           .offset = current_addr,
>> @@ -2978,7 +2983,7 @@ static int qemu_rdma_registration_handle(QEMUFile *f, void *opaque,
>>                */
>>               for (i = 0; i < local->nb_blocks; i++) {
>>                   rdma->block[i].remote_host_addr =
>> -                    (uint64_t)(local->block[i].local_host_addr);
>> +                    (uintptr_t)(local->block[i].local_host_addr);
>>   
>>                   if (rdma->pin_all) {
>>                       rdma->block[i].remote_rkey = local->block[i].mr->rkey;
>> @@ -3042,7 +3047,7 @@ static int qemu_rdma_registration_handle(QEMUFile *f, void *opaque,
>>                       goto out;
>>                   }
>>   
>> -                reg_result->host_addr = (uint64_t) block->local_host_addr;
>> +                reg_result->host_addr = (uintptr_t)block->local_host_addr;
>>   
>>                   trace_qemu_rdma_registration_handle_register_rkey(
>>                                                              reg_result->rkey);
>> -- 
>> 1.7.10.4
>>
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/3] migration: Fix some 32 bit compiler errors

[Stefan Weil]

Am 04.03.2015 um 13:44 schrieb Dr. David Alan Gilbert:
> * Stefan Weil (sw@weilnetz.de) wrote:
>> The current code won't compile on 32 bit hosts because there are lots
>> of type casts between pointers and 64 bit integers.
>>
>> Fix some of them.
>>
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> Please route rdma stuff through migration, not -trivial; it's never
> trivial to read this code.

IMHO the modifications here are trivial transformations, but I agree 
that other people might have a different view. Patch 3 is less trivial 
(as I wrote in my initial mail).

>
>> ---
>>   migration/rdma.c |   23 +++++++++++------------
>>   1 file changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/migration/rdma.c b/migration/rdma.c
>> index 67c5701..1512460 100644
>> --- a/migration/rdma.c
>> +++ b/migration/rdma.c
>> @@ -1104,7 +1104,7 @@ static int qemu_rdma_search_ram_block(RDMAContext *rdma,
>>    * to perform the actual RDMA operation.
>>    */
>>   static int qemu_rdma_register_and_get_keys(RDMAContext *rdma,
>> -        RDMALocalBlock *block, uint8_t *host_addr,
>> +        RDMALocalBlock *block, uintptr_t host_addr,
>>           uint32_t *lkey, uint32_t *rkey, int chunk,
>>           uint8_t *chunk_start, uint8_t *chunk_end)
> OK, so 'host_addr' seems to only be used in this function to print debug,
> so that should be harmless.
>
>>   {
>> @@ -1141,11 +1141,12 @@ static int qemu_rdma_register_and_get_keys(RDMAContext *rdma,
>>           if (!block->pmr[chunk]) {
>>               perror("Failed to register chunk!");
>>               fprintf(stderr, "Chunk details: block: %d chunk index %d"
>> -                            " start %" PRIu64 " end %" PRIu64 " host %" PRIu64
>> -                            " local %" PRIu64 " registrations: %d\n",
>> -                            block->index, chunk, (uint64_t) chunk_start,
>> -                            (uint64_t) chunk_end, (uint64_t) host_addr,
>> -                            (uint64_t) block->local_host_addr,
>> +                            " start %" PRIuPTR " end %" PRIuPTR
>> +                            " host %" PRIuPTR
>> +                            " local %" PRIuPTR " registrations: %d\n",
>> +                            block->index, chunk, (uintptr_t)chunk_start,
>> +                            (uintptr_t)chunk_end, host_addr,
>> +                            (uintptr_t)block->local_host_addr,
> OK, although is there any reason not to use %p for most of those?

The output of %p depends on the host's pointer size and is in hex. I 
don't know why the original author had chosen to show these values as 
integers.

>
>>                               rdma->total_registrations);
>>               return -1;
>>           }
>> @@ -1932,8 +1933,7 @@ retry:
>>               }
>>   
>>               /* try to overlap this single registration with the one we sent. */
>> -            if (qemu_rdma_register_and_get_keys(rdma, block,
>> -                                                (uint8_t *) sge.addr,
>> +            if (qemu_rdma_register_and_get_keys(rdma, block, sge.addr,
>>                                                   &sge.lkey, NULL, chunk,
>>                                                   chunk_start, chunk_end)) {
> sge.addr comes from /usr/include/infiniband/verbs.h for me:
>
> struct ibv_sge {
>          uint64_t                addr;
>          uint32_t                length;
>          uint32_t                lkey;
> };
>
> and that's the same on both 32 bit and 64 bit hosts (Fedora 21).
> I'm confused about why this helps you build 32 bit, since that uint64_t gets
> passed to your host_addr that's now a unitptr_t that will be 32bit.

That works because conversions between 32 and 64 bit values are no 
problem for the compiler (but maybe for the user when precision gets 
lost). IMHO it's surprising that the API in verbs.h uses uint64_t 
instead of uintptr_t for pointer values, but that's a different question.

Regards
Stefan

Re: [Qemu-devel] [PATCH 3/3] migration: Fix remaining 32 bit compiler errors

[Dr. David Alan Gilbert]

* Stefan Weil (sw@weilnetz.de) wrote:
> Please see my comments below.
> 
> Am 04.03.2015 um 14:31 schrieb Dr. David Alan Gilbert:
> >* Stefan Weil (sw@weilnetz.de) wrote:
> >>Fix type casts between pointers and 64 bit integers.
> >>Now 32 bit builds are possible again.
> >>
> >>Signed-off-by: Stefan Weil <sw@weilnetz.de>
> >>---
> >>  migration/rdma.c |   57 +++++++++++++++++++++++++++++-------------------------
> >>  1 file changed, 31 insertions(+), 26 deletions(-)
> >>
> >>diff --git a/migration/rdma.c b/migration/rdma.c
> >>index 1512460..a68f1f1 100644
> >>--- a/migration/rdma.c
> >>+++ b/migration/rdma.c
> >>@@ -493,8 +493,8 @@ static inline uint64_t ram_chunk_index(const uint8_t *start,
> >>  static inline uint8_t *ram_chunk_start(const RDMALocalBlock *rdma_ram_block,
> >>                                         uint64_t i)
> >>  {
> >>-    return (uint8_t *) (((uintptr_t) rdma_ram_block->local_host_addr)
> >>-                                    + (i << RDMA_REG_CHUNK_SHIFT));
> >>+    return (uint8_t *)(uintptr_t)(rdma_ram_block->local_host_addr +
> >>+                                  (i << RDMA_REG_CHUNK_SHIFT));
> >>  }
> >>  static inline uint8_t *ram_chunk_end(const RDMALocalBlock *rdma_ram_block,
> >>@@ -515,7 +515,7 @@ static int __qemu_rdma_add_block(RDMAContext *rdma, void *host_addr,
> >>  {
> >This will clash with my 'unbreak dtrace tracing due to double _ in rdma names' that's
> >going through Stefan's tree.
> 
> Then this part will have to wait until Stefan's tree was applied.
> 
> >
> >>      RDMALocalBlocks *local = &rdma->local_ram_blocks;
> >>      RDMALocalBlock *block = g_hash_table_lookup(rdma->blockmap,
> >>-        (void *) block_offset);
> >>+        (void *)(uintptr_t)block_offset);
> >>      RDMALocalBlock *old = local->block;
> >
> >>      assert(block == NULL);
> >>@@ -526,9 +526,11 @@ static int __qemu_rdma_add_block(RDMAContext *rdma, void *host_addr,
> >>          int x;
> >>          for (x = 0; x < local->nb_blocks; x++) {
> >>-            g_hash_table_remove(rdma->blockmap, (void *)old[x].offset);
> >>-            g_hash_table_insert(rdma->blockmap, (void *)old[x].offset,
> >>-                                                &local->block[x]);
> >>+            g_hash_table_remove(rdma->blockmap,
> >>+                                (void *)(uintptr_t)old[x].offset);
> >>+            g_hash_table_insert(rdma->blockmap,
> >>+                                (void *)(uintptr_t)old[x].offset,
> >>+                                &local->block[x]);
> >>          }
> >>          memcpy(local->block, old, sizeof(RDMALocalBlock) * local->nb_blocks);
> >>          g_free(old);
> >>@@ -549,12 +551,12 @@ static int __qemu_rdma_add_block(RDMAContext *rdma, void *host_addr,
> >>      block->is_ram_block = local->init ? false : true;
> >>-    g_hash_table_insert(rdma->blockmap, (void *) block_offset, block);
> >>+    g_hash_table_insert(rdma->blockmap, (void *)(uintptr_t)block_offset, block);
> >>      trace___qemu_rdma_add_block(local->nb_blocks,
> >>-                           (uint64_t) block->local_host_addr, block->offset,
> >>+                           (uintptr_t)block->local_host_addr, block->offset,
> >>                             block->length,
> >>-                           (uint64_t) (block->local_host_addr + block->length),
> >>+                           (uintptr_t)(block->local_host_addr + block->length),
> >>                             BITS_TO_LONGS(block->nb_chunks) *
> >>                                 sizeof(unsigned long) * 8,
> >>                             block->nb_chunks);
> >>@@ -595,7 +597,7 @@ static int qemu_rdma_init_ram_blocks(RDMAContext *rdma)
> >>      return 0;
> >>  }
> >>-static int __qemu_rdma_delete_block(RDMAContext *rdma, ram_addr_t block_offset)
> >>+static int __qemu_rdma_delete_block(RDMAContext *rdma, uintptr_t block_offset)
> >>  {
> >>      RDMALocalBlocks *local = &rdma->local_ram_blocks;
> >>      RDMALocalBlock *block = g_hash_table_lookup(rdma->blockmap,
> >>@@ -635,7 +637,7 @@ static int __qemu_rdma_delete_block(RDMAContext *rdma, ram_addr_t block_offset)
> >>      block->remote_keys = NULL;
> >>      for (x = 0; x < local->nb_blocks; x++) {
> >>-        g_hash_table_remove(rdma->blockmap, (void *)old[x].offset);
> >>+        g_hash_table_remove(rdma->blockmap, (void *)(uintptr_t)old[x].offset);
> >>      }
> >>      if (local->nb_blocks > 1) {
> >>@@ -658,9 +660,9 @@ static int __qemu_rdma_delete_block(RDMAContext *rdma, ram_addr_t block_offset)
> >>      }
> >>      trace___qemu_rdma_delete_block(local->nb_blocks,
> >>-                           (uint64_t)block->local_host_addr,
> >>+                           (uintptr_t)block->local_host_addr,
> >>                             block->offset, block->length,
> >>-                           (uint64_t)(block->local_host_addr + block->length),
> >>+                           (uintptr_t)(block->local_host_addr + block->length),
> >>                             BITS_TO_LONGS(block->nb_chunks) *
> >>                                 sizeof(unsigned long) * 8, block->nb_chunks);
> >>@@ -670,8 +672,9 @@ static int __qemu_rdma_delete_block(RDMAContext *rdma, ram_addr_t block_offset)
> >>      if (local->nb_blocks) {
> >>          for (x = 0; x < local->nb_blocks; x++) {
> >>-            g_hash_table_insert(rdma->blockmap, (void *)local->block[x].offset,
> >>-                                                &local->block[x]);
> >>+            g_hash_table_insert(rdma->blockmap,
> >>+                                (void *)(uintptr_t)local->block[x].offset,
> >>+                                &local->block[x]);
> >>          }
> >>      }
> >>@@ -1076,7 +1079,7 @@ static int qemu_rdma_reg_whole_ram_blocks(RDMAContext *rdma)
> >>   * This search cannot fail or the migration will fail.
> >>   */
> >>  static int qemu_rdma_search_ram_block(RDMAContext *rdma,
> >>-                                      uint64_t block_offset,
> >>+                                      uintptr_t block_offset,
> >>                                        uint64_t offset,
> >>                                        uint64_t length,
> >>                                        uint64_t *block_index,
> >>@@ -1380,8 +1383,8 @@ static uint64_t qemu_rdma_poll(RDMAContext *rdma, uint64_t *wr_id_out,
> >>          RDMALocalBlock *block = &(rdma->local_ram_blocks.block[index]);
> >>          trace_qemu_rdma_poll_write(print_wrid(wr_id), wr_id, rdma->nb_sent,
> >>-                 index, chunk,
> >>-                 block->local_host_addr, (void *)block->remote_host_addr);
> >>+                                   index, chunk, block->local_host_addr,
> >>+                                   (void *)(uintptr_t)block->remote_host_addr);
> >I think that's the wrong fix there; remote_host_addr is actually 64bit because
> >even if this host is 32bit the other end might be 64bit; I think the right
> >fix is to change the trace to use a uint64_t for the remote_host_addr.
> 
> That depends on the kind of output which is wanted.
> 
> We can either use %p for pointers, then the output will depend on the host's
> pointer size.
> Or we can use %16 PRIu64, then the output will always show 16 digits (with
> at least 8
> leading zeros on 32 bit hosts).

Since it's trying to print the remote_host_addr and that really can be 64bit
it doesn't seem right to use a %p since that's dependent on the local pointer
size.  I'd use a % PRIx64.

> >>          clear_bit(chunk, block->transit_bitmap);
> >>@@ -1524,7 +1527,7 @@ static int qemu_rdma_post_send_control(RDMAContext *rdma, uint8_t *buf,
> >>      RDMAWorkRequestData *wr = &rdma->wr_data[RDMA_WRID_CONTROL];
> >>      struct ibv_send_wr *bad_wr;
> >>      struct ibv_sge sge = {
> >>-                           .addr = (uint64_t)(wr->control),
> >>+                           .addr = (uintptr_t)(wr->control),
> >No, as before, my belief is that the .addr is a uint64_t.
> 
> Yes, because wr->control is a pointer which you want to convert to an
> integer value.
> C allows assignments from 32 bit integers to 64 bit integers - the compiler
> will add the
> necessary 0 bits.

Yes, fair enough.

Dave

> >(Although curiously I did a test and found that gcc lets me pass a uint64_t
> >to a function that has a uintptr_t parameter on a 32bit machine, but I don't
> >understand why.)
> >
> >Dave
> >
> >>                             .length = head->len + sizeof(RDMAControlHeader),
> >>                             .lkey = wr->control_mr->lkey,
> >>                           };
> >>@@ -1578,7 +1581,7 @@ static int qemu_rdma_post_recv_control(RDMAContext *rdma, int idx)
> >>  {
> >>      struct ibv_recv_wr *bad_wr;
> >>      struct ibv_sge sge = {
> >>-                            .addr = (uint64_t)(rdma->wr_data[idx].control),
> >>+                            .addr = (uintptr_t)(rdma->wr_data[idx].control),
> >>                              .length = RDMA_CONTROL_MAX_BUFFER,
> >>                              .lkey = rdma->wr_data[idx].control_mr->lkey,
> >>                           };
> >>@@ -1825,11 +1828,12 @@ static int qemu_rdma_write_one(QEMUFile *f, RDMAContext *rdma,
> >>                               };
> >>  retry:
> >>-    sge.addr = (uint64_t)(block->local_host_addr +
> >>+    sge.addr = (uintptr_t)(block->local_host_addr +
> >>                              (current_addr - block->offset));
> >>      sge.length = length;
> >>-    chunk = ram_chunk_index(block->local_host_addr, (uint8_t *) sge.addr);
> >>+    chunk = ram_chunk_index(block->local_host_addr,
> >>+                            (uint8_t *)(uintptr_t)sge.addr);
> >>      chunk_start = ram_chunk_start(block, chunk);
> >>      if (block->is_ram_block) {
> >>@@ -1882,8 +1886,9 @@ retry:
> >>               * memset() + madvise() the entire chunk without RDMA.
> >>               */
> >>-            if (can_use_buffer_find_nonzero_offset((void *)sge.addr, length)
> >>-                   && buffer_find_nonzero_offset((void *)sge.addr,
> >>+            if (can_use_buffer_find_nonzero_offset((void *)(uintptr_t)sge.addr,
> >>+                                                   length)
> >>+                   && buffer_find_nonzero_offset((void *)(uintptr_t)sge.addr,
> >>                                                      length) == length) {
> >>                  RDMACompress comp = {
> >>                                          .offset = current_addr,
> >>@@ -2978,7 +2983,7 @@ static int qemu_rdma_registration_handle(QEMUFile *f, void *opaque,
> >>               */
> >>              for (i = 0; i < local->nb_blocks; i++) {
> >>                  rdma->block[i].remote_host_addr =
> >>-                    (uint64_t)(local->block[i].local_host_addr);
> >>+                    (uintptr_t)(local->block[i].local_host_addr);
> >>                  if (rdma->pin_all) {
> >>                      rdma->block[i].remote_rkey = local->block[i].mr->rkey;
> >>@@ -3042,7 +3047,7 @@ static int qemu_rdma_registration_handle(QEMUFile *f, void *opaque,
> >>                      goto out;
> >>                  }
> >>-                reg_result->host_addr = (uint64_t) block->local_host_addr;
> >>+                reg_result->host_addr = (uintptr_t)block->local_host_addr;
> >>                  trace_qemu_rdma_registration_handle_register_rkey(
> >>                                                             reg_result->rkey);
> >>-- 
> >>1.7.10.4
> >>
> >>
> >--
> >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/3] migration: Fix some 32 bit compiler errors

[Dr. David Alan Gilbert]

* Stefan Weil (sw@weilnetz.de) wrote:
> Am 04.03.2015 um 13:44 schrieb Dr. David Alan Gilbert:
> >* Stefan Weil (sw@weilnetz.de) wrote:
> >>The current code won't compile on 32 bit hosts because there are lots
> >>of type casts between pointers and 64 bit integers.
> >>
> >>Fix some of them.
> >>
> >>Signed-off-by: Stefan Weil <sw@weilnetz.de>
> >Please route rdma stuff through migration, not -trivial; it's never
> >trivial to read this code.
> 
> IMHO the modifications here are trivial transformations, but I agree that
> other people might have a different view. Patch 3 is less trivial (as I
> wrote in my initial mail).
> 
> >
> >>---
> >>  migration/rdma.c |   23 +++++++++++------------
> >>  1 file changed, 11 insertions(+), 12 deletions(-)
> >>
> >>diff --git a/migration/rdma.c b/migration/rdma.c
> >>index 67c5701..1512460 100644
> >>--- a/migration/rdma.c
> >>+++ b/migration/rdma.c
> >>@@ -1104,7 +1104,7 @@ static int qemu_rdma_search_ram_block(RDMAContext *rdma,
> >>   * to perform the actual RDMA operation.
> >>   */
> >>  static int qemu_rdma_register_and_get_keys(RDMAContext *rdma,
> >>-        RDMALocalBlock *block, uint8_t *host_addr,
> >>+        RDMALocalBlock *block, uintptr_t host_addr,
> >>          uint32_t *lkey, uint32_t *rkey, int chunk,
> >>          uint8_t *chunk_start, uint8_t *chunk_end)
> >OK, so 'host_addr' seems to only be used in this function to print debug,
> >so that should be harmless.
> >
> >>  {
> >>@@ -1141,11 +1141,12 @@ static int qemu_rdma_register_and_get_keys(RDMAContext *rdma,
> >>          if (!block->pmr[chunk]) {
> >>              perror("Failed to register chunk!");
> >>              fprintf(stderr, "Chunk details: block: %d chunk index %d"
> >>-                            " start %" PRIu64 " end %" PRIu64 " host %" PRIu64
> >>-                            " local %" PRIu64 " registrations: %d\n",
> >>-                            block->index, chunk, (uint64_t) chunk_start,
> >>-                            (uint64_t) chunk_end, (uint64_t) host_addr,
> >>-                            (uint64_t) block->local_host_addr,
> >>+                            " start %" PRIuPTR " end %" PRIuPTR
> >>+                            " host %" PRIuPTR
> >>+                            " local %" PRIuPTR " registrations: %d\n",
> >>+                            block->index, chunk, (uintptr_t)chunk_start,
> >>+                            (uintptr_t)chunk_end, host_addr,
> >>+                            (uintptr_t)block->local_host_addr,
> >OK, although is there any reason not to use %p for most of those?
> 
> The output of %p depends on the host's pointer size and is in hex. I don't
> know why the original author had chosen to show these values as integers.

Yes, that's fair enough; if you recut it for any reason then %p would be nice
for the local pointers.

> >>                              rdma->total_registrations);
> >>              return -1;
> >>          }
> >>@@ -1932,8 +1933,7 @@ retry:
> >>              }
> >>              /* try to overlap this single registration with the one we sent. */
> >>-            if (qemu_rdma_register_and_get_keys(rdma, block,
> >>-                                                (uint8_t *) sge.addr,
> >>+            if (qemu_rdma_register_and_get_keys(rdma, block, sge.addr,
> >>                                                  &sge.lkey, NULL, chunk,
> >>                                                  chunk_start, chunk_end)) {
> >sge.addr comes from /usr/include/infiniband/verbs.h for me:
> >
> >struct ibv_sge {
> >         uint64_t                addr;
> >         uint32_t                length;
> >         uint32_t                lkey;
> >};
> >
> >and that's the same on both 32 bit and 64 bit hosts (Fedora 21).
> >I'm confused about why this helps you build 32 bit, since that uint64_t gets
> >passed to your host_addr that's now a unitptr_t that will be 32bit.
> 
> That works because conversions between 32 and 64 bit values are no problem
> for the compiler (but maybe for the user when precision gets lost).

Yes, I was surprised you don't get a warning for truncation there, but since
you don't:

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> IMHO
> it's surprising that the API in verbs.h uses uint64_t instead of uintptr_t
> for pointer values, but that's a different question.

Many things about the IB API surprise me.

Dave

> 
> Regards
> Stefan
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/3] migration: Fix coding style (whitespace issues)

[Juan Quintela]

Stefan Weil <sw@weilnetz.de> wrote:
> * Remove trailing whitespace (fixes 9 errors from checkpatch.pl).
>   One comment line was longer than 80 characters, so wrap it
>   and fix a typo, too.
> * Replace tabs by blanks (fixes 1 error).
>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>

Applied, thanks.

Re: [Qemu-devel] [PATCH 2/3] migration: Fix some 32 bit compiler errors

[Juan Quintela]

Stefan Weil <sw@weilnetz.de> wrote:
> The current code won't compile on 32 bit hosts because there are lots
> of type casts between pointers and 64 bit integers.
>
> Fix some of them.
>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>

Applied, thanks.

Re: [Qemu-devel] [PATCH 3/3] migration: Fix remaining 32 bit compiler errors

[Juan Quintela]

Stefan Weil <sw@weilnetz.de> wrote:
> Fix type casts between pointers and 64 bit integers.
> Now 32 bit builds are possible again.
>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>

Applied, thanks.

I fixed by hand the changes in the name of the dtrace functions.

Later, Juan.