Re: [qemu-s390x] [Qemu-devel] [PATCH] hw/s390x: fix clang compilation on 32bit machines

[Philippe Mathieu-Daudé]

Le lun. 18 mars 2019 11:34, Marcel Apfelbaum <address@hidden> a écrit :

Hi Christian,

On 3/18/19 11:27 AM, Christian Borntraeger wrote:
>
> On 16.03.19 12:09, Philippe Mathieu-Daudé wrote:
>> Hi Marcel,
>>
>> On 3/16/19 10:50 AM, Marcel Apfelbaum wrote:
>>> Configuring QEMU with:
>>>      configure --cc=clang --target-list=s390x-softmmu
>>> And compiling it using a 32 bit machine leads to:
>> Because there sizeof(ram_addr_t) = sizeof(uintptr_t) = 32.
>>
>>>      hw/s390x/s390-virtio-ccw.c:175:27: error: implicit conversion from
>>>        'unsigned long long' to 'ram_addr_t' (aka 'unsigned int') changes value
>>>        from 8796091973632 to 4293918720 [-Werror,-Wconstant-conversion]
>>>          chunk = MIN(size, KVM_SLOT_MAX_BYTES);
>>>                ~ ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
>> The comment 1 line earlier is:
>>
>>          /* KVM does not allow memslots >= 8 TB */
>>
>> Clang is correct, this KVM_SLOT_MAX_BYTES is incorrect on a 32bit s390,
>> you need a 64bit system.
> KVM is only supported on 64bit s390.
>   

So maybe the fix I proposed is enough.

Enough to silent a warning due to a bug, as confirmed Christian KVM code should be reachable on 32 bit hosts.

Safer would it be to fix the bug.

>
>
>> Per Hacking:
>>
>>    Use hwaddr for guest physical addresses except pcibus_t
>>    for PCI addresses.  In addition, ram_addr_t is a QEMU internal address
>>    space that maps guest RAM physical addresses into an intermediate
>>    address space that can map to host virtual address spaces.  Generally
>>    speaking, the size of guest memory can always fit into ram_addr_t but
>>    it would not be correct to store an actual guest physical address in a
>>    ram_addr_t.
>>
>> My understanding is we should not use ram_addr_t with KVM but rather
>> hwaddr, but I'm not sure.
>>
>> I don't know about s390, if 32bit host is supported or supports KVM.
>> If it is, maybe this could work:
>>
>> I don't think the following is clean:
>>
>> #if TARGET_LONG_BITS == 32
>> # define KVM_SLOT_MAX_BYTES RAM_ADDR_MAX
>> #else
>> # define KVM_SLOT_MAX_BYTES \
>>               ((KVM_MEM_MAX_NR_PAGES * TARGET_PAGE_SIZE) & SEG_MSK)
>> #endif
>>
>> But checking ifdef CONFIG_KVM might be clever:
>>
>> -- >8 --
>> @@ -161,7 +161,7 @@ static void virtio_ccw_register_hcalls(void)
>>   static void s390_memory_init(ram_addr_t mem_size)
>>   {
>>       MemoryRegion *sysmem = get_system_memory();
>> -    ram_addr_t chunk, offset = 0;
>> +    hwaddr offset = 0;
>>       unsigned int number = 0;
>>       gchar *name;
>>
>> @@ -169,14 +169,16 @@ static void s390_memory_init(ram_addr_t mem_size)
>>       name = g_strdup_printf("s390.ram");
>>       while (mem_size) {
>>           MemoryRegion *ram = g_new(MemoryRegion, 1);
>> -        uint64_t size = mem_size;
>> +        uint64_t chunk_size = mem_size;
>>
>> +#ifdef CONFIG_KVM
>>           /* KVM does not allow memslots >= 8 TB */
>> -        chunk = MIN(size, KVM_SLOT_MAX_BYTES);
>> -        memory_region_allocate_system_memory(ram, NULL, name, chunk);
>> +        chunk_size = MIN(mem_size, KVM_SLOT_MAX_BYTES);
>> +#endif
>> +        memory_region_allocate_system_memory(ram, NULL, name, chunk_size);
>>           memory_region_add_subregion(sysmem, offset, ram);
>> -        mem_size -= chunk;
>> -        offset += chunk;
>> +        mem_size -= chunk_size;
>> +        offset += chunk_size;
>>           g_free(name);
>>           name = g_strdup_printf("s390.ram.%u", ++number);
>>       }
>> ---
>>
>> Anyway s390x experts will figure that out ;)
>
> I will have a look.
>

Appreciated, I just need to be able to compile QEMU with clang on a
32bit machine.
Any fix would do.

Thanks,
Marcel