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

[Marcel Apfelbaum]

Hi Philippe,

On 3/16/19 1:09 PM, 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.

Right

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

Looks fairly complicated, the chunk variable will be used by
memory_region_allocate_system_memory as 'ram_size' parameter, not an 
address.
Also it is initialized by a MIN macro with the first operand being uint64_t.

It just seems to me that if some kind of 'check' is needed, maybe is not 
related
to the variable type (in this case).

> Anyway s390x experts will figure that out ;)

Definitely!

Thanks for the review,
Marcel

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