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.da...@gmail.com> wrote:
Le lun. 18 mars 2019 11:34, Marcel Apfelbaum <marcel.apfelb...@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 <pa...@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
