Re: [PATCH v1 01/14] s390x/s390-virtio-ccw: don't crash on weird RAM sizes

[David Hildenbrand <david@redhat.com>]

On 23.09.24 17:36, Thomas Huth wrote:
> On 23/09/2024 11.19, David Hildenbrand wrote:
>> On 10.09.24 19:57, David Hildenbrand wrote:
>>> KVM is not happy when starting a VM with weird RAM sizes:
>>>
>>>     # qemu-system-s390x --enable-kvm --nographic -m 1234K
>>>     qemu-system-s390x: kvm_set_user_memory_region: KVM_SET_USER_MEMORY_REGION
>>>       failed, slot=0, start=0x0, size=0x244000: Invalid argument
>>>     kvm_set_phys_mem: error registering slot: Invalid argument
>>>     Aborted (core dumped)
>>>
>>> Let's handle that in a better way by rejecting such weird RAM sizes
>>> right from the start:
>>>
>>>     # qemu-system-s390x --enable-kvm --nographic -m 1234K
>>>     qemu-system-s390x: ram size must be multiples of 1 MiB
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>    hw/s390x/s390-virtio-ccw.c | 11 +++++++++++
>>>    1 file changed, 11 insertions(+)
>>>
>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>> index 18240a0fd8..e30cf0a2a1 100644
>>> --- a/hw/s390x/s390-virtio-ccw.c
>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>> @@ -180,6 +180,17 @@ static void s390_memory_init(MemoryRegion *ram)
>>>    {
>>>        MemoryRegion *sysmem = get_system_memory();
>>> +    if (!QEMU_IS_ALIGNED(memory_region_size(ram), 1 * MiB)) {
>>> +        /*
>>> +         * The SCLP cannot possibly expose smaller granularity right now
>>> and KVM
>>> +         * cannot handle smaller granularity. As we don't support NUMA, the
>>> +         * region size directly corresponds to machine->ram_size, and the
>>> region
>>> +         * is a single RAM memory region.
>>> +         */
>>> +        error_report("ram size must be multiples of 1 MiB");
>>> +        exit(EXIT_FAILURE);
>>> +    }
>>
>> I'll switch to
>>
>>       error_setg(&error_fatal, "ram size must be multiples of 1 MiB");
>>
>> here, to avoid the manual exit().
>>
>> Please someone shout if I should keep it as is.
> 
> Please keep it, according to include/qapi/error.h:
> 
>    * Please don't error_setg(&error_fatal, ...), use error_report() and
>    * exit(), because that's more obvious.
> 

Controversial, but will do. Thanks!

-- 
Cheers,

David / dhildenb