1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH v5 00/46] Use the IEC binary prefix definitions
  4. View patch

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

Philippe Mathieu-Daudé posted 46 patches 7 years, 7 months ago
Only 43 patches received!
[Qemu-devel] [PATCH v5 19/46] hw/s390x: Use the IEC binary prefix definitions
Posted by Philippe Mathieu-Daudé 7 years, 7 months ago 
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>
---
 hw/s390x/s390-skeys.c    | 3 ++-
 hw/s390x/s390-stattrib.c | 3 ++-
 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 */
 #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)
 
 #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");
     }
-- 
2.18.0
Re: [Qemu-devel] [PATCH v5 19/46] hw/s390x: Use the IEC binary prefix definitions
Posted by Cornelia Huck 7 years, 7 months ago 
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");
>      }
Re: [Qemu-devel] [PATCH v5 19/46] hw/s390x: Use the IEC binary prefix definitions
Posted by David Hildenbrand 7 years, 7 months ago 
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
Re: [Qemu-devel] [PATCH v5 19/46] hw/s390x: Use the IEC binary prefix definitions
Posted by Cornelia Huck 7 years, 7 months ago 
On Mon, 25 Jun 2018 15:16:15 +0200
David Hildenbrand <david@redhat.com> wrote:

> 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"

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.
Re: [Qemu-devel] [PATCH v5 19/46] hw/s390x: Use the IEC binary prefix definitions
Posted by Philippe Mathieu-Daudé 7 years, 7 months ago 
Hi Cornelia,

On 06/25/2018 10:07 AM, 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...

Oops...

>> ---
>>  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?

The idea is use the byte IEC binary prefix to make it more obvious.

If the unit is not 'byte', then this is not a correct use (confusing, as
you said).

> 
>>  #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");
>>      }
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.