qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Halil Pasic
Subject: Re: [Qemu-devel] [qemu-s390x] [PATCH] hw/s390x: fix clang compilation on 32bit machines
Date: Fri, 22 Mar 2019 16:52:13 +0100

On Mon, 18 Mar 2019 22:08:50 +0100
Philippe Mathieu-Daudé <address@hidden> wrote:

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

Sorry guys for the long wait. We are decimated by flue at the moment.

IMHO Clang is wrong about this. The value put in chunk is guaranteed to
fit unsigned int.

Namely


static void s390_memory_init(ram_addr_t mem_size)                               
{                                                                               
    MemoryRegion *sysmem = get_system_memory();                                 
    ram_addr_t chunk, offset = 0;                                               
    unsigned int number = 0;                                                    
    gchar *name;                                                                
                                                                                
    /* allocate RAM for core */                                                 
    name = g_strdup_printf("s390.ram");                                         
    while (mem_size) {                                                          
        MemoryRegion *ram = g_new(MemoryRegion, 1);                             
        uint64_t size = mem_size;

The most significant 32 bits of size are zeros because mem_size
is effectively uint.                                 
                                                                                
        /* KVM does not allow memslots >= 8 TB */                               
        chunk = MIN(size, KVM_SLOT_MAX_BYTES);

Thus the result of MIN() is guaranteed to fit into chunk despite of its
type being wider.

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



IMHO there is no bug! Thus I think Marcel's fix is sufficient. A simple
cast to ram_addr_t could be even simpler, but I did not check if that
silences Clang. @Marcel would you like to try that out?

> 
> >
> > >
> > >> 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 tend to agree with you. The usage of the types is IMHO messy in the
function under discussion. But I'm not a memory guy, and I would hate to
make calls on this.

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

Given that I don't think there is a bug I would like any cleanup
done as a separate cleanup patch.

This snipped does seem to synchronize the formal and effective arguments
of memory_region_allocate_system_memory() and memory_region_add_subregion()
which I like, but I don't like the #ifdef stuff.

Philippe, your input is appreciated! Feel free to post a separate cleanup patch
if you like.

For the Clang issue, I think we should go with the least invasive solution.

> > >
> > > I will have a look.
> > >

I had have a look in Chirstian's stead.

My conclusion is

Reviewed-by: Halil Pasic <address@hidden>

for the Marcel patch.

CCing Claudio who is more a memory guy than myself.

Regards,
Halil




reply via email to

[Prev in Thread] Current Thread [Next in Thread]