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

[Christian Borntraeger]

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.
 



> 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.