qemu-trivial
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Cornelia Huck
Subject: Re: [Qemu-trivial] [PATCH v5 19/46] hw/s390x: Use the IEC binary prefix definitions
Date: Mon, 25 Jun 2018 16:21:25 +0200

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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]