Re: [PATCH v3 1/1] vl/s390x: fixup ram sizes for compat machines

[Cornelia Huck <cohuck@redhat.com>]

On Thu, 2 Apr 2020 11:39:11 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 02.04.20 11:27, Cornelia Huck wrote:
> > On Wed, 1 Apr 2020 18:34:56 +0200
> > Igor Mammedov <imammedo@redhat.com> wrote:
> >   
> >> On Wed,  1 Apr 2020 08:37:54 -0400
> >> Christian Borntraeger <borntraeger@de.ibm.com> wrote:  
> >   
> >>> +static ram_addr_t s390_fixup_ram_size(ram_addr_t sz)
> >>> +{
> >>> +    /* same logic as in sclp.c */
> >>> +    int increment_size = 20;
> >>> +    ram_addr_t newsz;
> >>> +
> >>> +    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    
> >>                                                    ^^^^^^^^
> >>
> >> for unaware  user it could be confusing as it could be read as 'value was increased'
> >> s/fixed up/amended/ might be better  
> > 
> > "rounded", perhaps?
> >   
> >>  
> >>> +                    "MB to match machine restrictions. Consider updating "
> >>> +                    "the guest definition.i\n", sz / MiB, newsz / MiB);    
> >>
> >> also it might be better to use size_to_str() to format numbers  
> > 
> > The text explicitly talks about 'MB'... not sure if it would be
> > confusing if the user specified MB and ended up with GB or so in this
> > message.
> >   
> >>  
> >>> +    }
> >>> +    return newsz;
> >>> +}
> >>> +  
> > 
> > (If we can agree upon message and format, I'll happily fix that up when
> > applying.)  
> 
> Is the the only thing that blocks this? I would rather try to get this fixed before rc2.
> 

I now have

    if (sz != newsz) {                                                          
        qemu_printf("Ram size %" PRIu64 "MB was fixed up to %" PRIu64           
                    "MB to match machine restrictions. Consider updating "      
                    "the guest definition.\n", (uint64_t) (sz / MiB),           
                    (uint64_t) (newsz / MiB));                                  
    }

Opinions?