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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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