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

[Dr. David Alan Gilbert]

* Stefan Weil (address@hidden) wrote:
> Am 04.03.2015 um 13:44 schrieb Dr. David Alan Gilbert:
> >* Stefan Weil (address@hidden) 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 <address@hidden>
> >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 <address@hidden>

> 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 / address@hidden / Manchester, UK