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

[David Hildenbrand <david@redhat.com>]

On 25.06.2018 15:07, Cornelia Huck wrote:
> On Mon, 25 Jun 2018 09:42:11 -0300
> Philippe Mathieu-Daudé <f4bug@amsat.org> 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é <f4bug@amsat.org>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> Acked-by: Cornelia Huck <cohuck@redhat.com>
> 
> 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"

-- 

Thanks,

David / dhildenb