Re: [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
>
>
>
>
>