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

[Stefan Weil <sw@weilnetz.de>]

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