Re: [qemu-s390x] [PATCH v5 19/46] hw/s390x: Use the IEC binary prefix definitions

[Cornelia Huck]

On Mon, 25 Jun 2018 15:16:15 +0200
David Hildenbrand <address@hidden> wrote:

> On 25.06.2018 15:07, Cornelia Huck wrote:
> > On Mon, 25 Jun 2018 09:42:11 -0300
> > Philippe Mathieu-Daudé <address@hidden> wrote:
> >   
> >> It eases code review, unit is explicit.
> >>
> >> Patch generated using:
> >>
> >>   $ git grep -E '(1024|2048|4096|8192|(<<|>>).?(10|20|30))' hw/ include/hw/
> >>
> >> and modified manually.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> >> Reviewed-by: Thomas Huth <address@hidden>
> >> Acked-by: Cornelia Huck <address@hidden>  
> > 
> > Hm, I had not looked at the v2+ patches, as this already carried my
> > ack...
> >   
> >> ---
> >>  hw/s390x/s390-skeys.c    | 3 ++-
> >>  hw/s390x/s390-stattrib.c | 3 ++-  
> > 
> > ...but these two were added later on.
> >   
> >>  hw/s390x/sclp.c          | 3 ++-
> >>  3 files changed, 6 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
> >> index 76241c240e..15f7ab0e53 100644
> >> --- a/hw/s390x/s390-skeys.c
> >> +++ b/hw/s390x/s390-skeys.c
> >> @@ -10,6 +10,7 @@
> >>   */
> >>  
> >>  #include "qemu/osdep.h"
> >> +#include "qemu/units.h"
> >>  #include "hw/boards.h"
> >>  #include "hw/s390x/storage-keys.h"
> >>  #include "qapi/error.h"
> >> @@ -19,7 +20,7 @@
> >>  #include "sysemu/kvm.h"
> >>  #include "migration/register.h"
> >>  
> >> -#define S390_SKEYS_BUFFER_SIZE 131072  /* Room for 128k storage keys */
> >> +#define S390_SKEYS_BUFFER_SIZE (128 * KiB)  /* Room for 128k storage keys 
> >> */  
> > 
> > This one looks confusing to me. We're not allocating 128 chunks of 1
> > KiB size, but space enough for 128k byte values. What do others think?  
> 
> Hm, as this define is called "_SIZE" it should be the right thing to do.
> 
> I would agree if it would be "_SKEY.*_COUNT"

Yes, but I found it a bit non-intuitive, as it is not immediately clear
that we want to support 128k byte values. It's probably clearer than
the previous magic value, though.

No real objections to this change from my side, though.