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

[Marcel Apfelbaum]

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.

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