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

[Cornelia Huck <cohuck@redhat.com>]

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?

>  #define S390_SKEYS_SAVE_FLAG_EOS 0x01
>  #define S390_SKEYS_SAVE_FLAG_SKEYS 0x02
>  #define S390_SKEYS_SAVE_FLAG_ERROR 0x04
> diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
> index 70b95550a8..5161a1659b 100644
> --- a/hw/s390x/s390-stattrib.c
> +++ b/hw/s390x/s390-stattrib.c
> @@ -10,6 +10,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/units.h"
>  #include "hw/boards.h"
>  #include "cpu.h"
>  #include "migration/qemu-file.h"
> @@ -20,7 +21,7 @@
>  #include "qapi/error.h"
>  #include "qapi/qmp/qdict.h"
>  
> -#define CMMA_BLOCK_SIZE  (1 << 10)
> +#define CMMA_BLOCK_SIZE  (1 * KiB)

This one looks fine.

>  
>  #define STATTR_FLAG_EOS     0x01ULL
>  #define STATTR_FLAG_MORE    0x02ULL
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 047d577313..bd2a024efd 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -13,6 +13,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/units.h"
>  #include "qapi/error.h"
>  #include "cpu.h"
>  #include "sysemu/sysemu.h"
> @@ -289,7 +290,7 @@ static void sclp_realize(DeviceState *dev, Error **errp)
>      ret = s390_set_memory_limit(machine->maxram_size, &hw_limit);
>      if (ret == -E2BIG) {
>          error_setg(&err, "host supports a maximum of %" PRIu64 " GB",
> -                   hw_limit >> 30);
> +                   hw_limit / GiB);
>      } else if (ret) {
>          error_setg(&err, "setting the guest size failed");
>      }