* Stefan Weil (s...@weilnetz.de) wrote:
> Am 04.03.2015 um 13:44 schrieb Dr. David Alan Gilbert:
> >* Stefan Weil (s...@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 <s...@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 <dgilb...@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 / dgilb...@redhat.com / Manchester, UK
