| Message ID | 20190316095049.28941-1-marcel.apfelbaum@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/s390x: fix clang compilation on 32bit machines | expand |
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 <marcel.apfelbaum@gmail.com> > --- > 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; > >
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 <marcel.apfelbaum@gmail.com> >> --- >> 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; >> >>
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. > 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.
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
Le lun. 18 mars 2019 11:34, Marcel Apfelbaum <marcel.apfelbaum@gmail.com> 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 > > > > >
On Mon, 18 Mar 2019 22:08:50 +0100 Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> wrote: > Le lun. 18 mars 2019 11:34, Marcel Apfelbaum <marcel.apfelbaum@gmail.com> 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 <pasic@linux.ibm.com> for the Marcel patch. CCing Claudio who is more a memory guy than myself. Regards, Halil
Hi, On 3/22/19 5:52 PM, Halil Pasic wrote: > On Mon, 18 Mar 2019 22:08:50 +0100 > Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> wrote: > >> Le lun. 18 mars 2019 11:34, Marcel Apfelbaum <marcel.apfelbaum@gmail.com> 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. >>>>> >>>>>> v: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? I confirm casting the result of MIN(...) to ram_addr_t silences clang. >>>>> 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 <pasic@linux.ibm.com> > > for the Marcel patch. Appreciated. Let me know if you prefer me to send a V2 using the cast. Thanks for looking into it, Marcel > CCing Claudio who is more a memory guy than myself. > > Regards, > Halil >
On Sun, 24 Mar 2019 09:09:05 +0200 Marcel Apfelbaum <marcel.apfelbaum@gmail.com> wrote: > Appreciated. Let me know if you prefer me to send a V2 using the cast. Yes, I would prefer a V2 using the cast. I think Connie should be back next week, and can then pick your patch. > > Thanks for looking into it, Thank you! Regards, Halil
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;
Configuring QEMU with: configure --cc=clang --target-list=s390x-softmmu And compiling it using a 32 bit machine leads to: 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); ~ ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~ Fix the missmatch by declaring the 'chunk' variable as uint64_t. Signed-off-by: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> --- hw/s390x/s390-virtio-ccw.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)