Re: [PATCH v2] vl/s390: fixup ram sizes for compat machines

[David Hildenbrand]

> something like
> 
>     while ((sz >> increment_size) > MAX_STORAGE_INCREMENTS) {
>         increment_size++;
>     }
>     newsz = sz >> increment_size << increment_size;
>     
>     if (sz != newsz) {
>         qemu_printf("Ram size %" PRIu64 "MB was fixed up to %" PRIu64

Maybe warn_report()

>                     "MB to match machine restrictions. Consider updating "
>                     "the guest definition.\n",
>                     sz / MiB, newsz / MiB);

might be able to squeeze that into the previous line.

>     }
>     return newsz;
> 
> ?

Much better.

>>
>>
>>>      if (ret == -E2BIG) {
>>>          error_setg(&err, "host supports a maximum of %" PRIu64 " GB",
>>>                     hw_limit / GiB);
>>> @@ -361,27 +361,20 @@ out:
>>>  static void sclp_memory_init(SCLPDevice *sclp)
>>>  {
>>>      MachineState *machine = MACHINE(qdev_get_machine());
>>> +    MachineClass *machine_class = MACHINE_GET_CLASS(qdev_get_machine());
>>>      ram_addr_t initial_mem = machine->ram_size;
>>>      int increment_size = 20;
>>>  
>>>      /* The storage increment size is a multiple of 1M and is a power of 2.
>>> -     * The number of storage increments must be MAX_STORAGE_INCREMENTS or 
>>> fewer.
>>> +     * For some machine types, the number of storage increments must be
>>> +     * MAX_STORAGE_INCREMENTS or fewer.
>>>       * The variable 'increment_size' is an exponent of 2 that can be
>>>       * used to calculate the size (in bytes) of an increment. */
>>> -    while ((initial_mem >> increment_size) > MAX_STORAGE_INCREMENTS) {
>>> +    while (machine_class->fixup_ram_size != NULL &&
>>> +           (initial_mem >> increment_size) > MAX_STORAGE_INCREMENTS) {
>>>          increment_size++;
>>>      }
>>>      sclp->increment_size = increment_size;
>>
>> IIRC one could define ram size in KB. Not sure if it is worth checking
>> against that.
>> [...]
> 
> If this is not aligned to 1MB, that would already fail when registering the 
> memslot, I think.

True, not sure how cryptic the error will be :)


LGTM


-- 
Thanks,

David / dhildenb