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

[Philippe Mathieu-Daudé]

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.

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 ;)

Regards,

Phil.

> 
> Fix the missmatch by declaring the 'chunk' variable as uint64_t.
> 
> Signed-off-by: Marcel Apfelbaum <address@hidden>
> ---
>  hw/s390x/s390-virtio-ccw.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index d11069b860..2efa47bc3e 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -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;
> +    uint64_t chunk, offset = 0;
>      unsigned int number = 0;
>      gchar *name;
>  
>