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

[Igor Mammedov]

On Thu, 2 Apr 2020 11:27:35 +0200
Cornelia Huck <address@hidden> wrote:

> On Wed, 1 Apr 2020 18:34:56 +0200
> Igor Mammedov <address@hidden> wrote:
> 
> > On Wed,  1 Apr 2020 08:37:54 -0400
> > Christian Borntraeger <address@hidden> 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.

MB can be dropped, since it still might not match what user specified with -m
it could be specified in b/kb/mb/gb over there

so I'd drop MB and print value size_to_str() returns
(it will add appropriate suffix if I'm not mistaken)


> >   
> > > +    }
> > > +    return newsz;
> > > +}
> > > +  
> 
> (If we can agree upon message and format, I'll happily fix that up when
> applying.)
> 
>