[Qemu-devel] [PATCH 0/2] use sizes.h macros for power-of-two sizes

[Qemu-devel] [PATCH 0/2] use sizes.h macros for power-of-two sizes

[Antony Pavlov]

[PATCH 1/2] include/qemu: introduce sizes.h
[PATCH 2/2] hw/mips: use sizes.h macros

The sizes.h macros is a easy-to-read method of
power-of-two memory sizes representation. The sizes.h
macros are actively used in linux kernel and other
projects, so let's use them in QEMU too.

[Qemu-devel] [PATCH 1/2] include/qemu: introduce sizes.h

[Antony Pavlov]

The header file sizes.h is used in linux kernel,
barebox bootloader and u-boot bootloader. It provides
the short and easy-to-read names for power-of-two
numbers. The numbers like this are othen used
for memory range sizes.

Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
Reviewed-by: Richard Henderson <rth@twiddle.net>
---
 include/qemu/sizes.h | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)
 create mode 100644 include/qemu/sizes.h

diff --git a/include/qemu/sizes.h b/include/qemu/sizes.h
new file mode 100644
index 0000000..a683b60
--- /dev/null
+++ b/include/qemu/sizes.h
@@ -0,0 +1,61 @@
+/*
+ * Handy size definitions
+ *
+ * Copyright (C) 2013 Antony Pavlov <antonynpavlov@gmail.com>
+ *
+ * Inspired by linux/sizes.h
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef QEMU_SIZES_H
+#define QEMU_SIZES_H
+
+#include <qemu/bitops.h>
+
+#define SZ_1                BIT(0)
+#define SZ_2                BIT(1)
+#define SZ_4                BIT(2)
+#define SZ_8                BIT(3)
+#define SZ_16               BIT(4)
+#define SZ_32               BIT(5)
+#define SZ_64               BIT(6)
+#define SZ_128              BIT(7)
+#define SZ_256              BIT(8)
+#define SZ_512              BIT(9)
+
+#define SZ_1K               BIT(10)
+#define SZ_2K               BIT(11)
+#define SZ_4K               BIT(12)
+#define SZ_8K               BIT(13)
+#define SZ_16K              BIT(14)
+#define SZ_32K              BIT(15)
+#define SZ_64K              BIT(16)
+#define SZ_128K             BIT(17)
+#define SZ_256K             BIT(18)
+#define SZ_512K             BIT(19)
+
+#define SZ_1M               BIT(20)
+#define SZ_2M               BIT(21)
+#define SZ_4M               BIT(22)
+#define SZ_8M               BIT(23)
+#define SZ_16M              BIT(24)
+#define SZ_32M              BIT(25)
+#define SZ_64M              BIT(26)
+#define SZ_128M             BIT(27)
+#define SZ_256M             BIT(28)
+#define SZ_512M             BIT(29)
+
+#define SZ_1G               BIT(30)
+#define SZ_2G               BIT(31)
+
+#endif /* QEMU_SIZES_H */
-- 
1.8.4.4

[Qemu-devel] [PATCH 2/2] hw/mips: use sizes.h macros

[Antony Pavlov]

Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
Reviewed-by: Richard Henderson <rth@twiddle.net>
---
 hw/mips/mips_malta.c   | 25 +++++++++++++------------
 include/hw/mips/bios.h |  3 ++-
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index 05c8771..604832f 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -51,6 +51,7 @@
 #include "sysemu/qtest.h"
 #include "qemu/error-report.h"
 #include "hw/empty_slot.h"
+#include "qemu/sizes.h"
 
 //#define DEBUG_BOARD_INIT
 
@@ -63,7 +64,7 @@
 #define FPGA_ADDRESS  0x1f000000ULL
 #define RESET_ADDRESS 0x1fc00000ULL
 
-#define FLASH_SIZE    0x400000
+#define FLASH_SIZE    SZ_4M
 
 #define MAX_IDE_BUS 2
 
@@ -827,8 +828,8 @@ static int64_t load_kernel (void)
     }
 
     prom_set(prom_buf, prom_index++, "memsize");
-    prom_set(prom_buf, prom_index++, "%i",
-             MIN(loaderparams.ram_size, 256 << 20));
+    prom_set(prom_buf, prom_index++, "%li",
+             MIN(loaderparams.ram_size, SZ_256M));
     prom_set(prom_buf, prom_index++, "modetty0");
     prom_set(prom_buf, prom_index++, "38400n8r");
     prom_set(prom_buf, prom_index++, NULL);
@@ -954,10 +955,10 @@ void mips_malta_init(QEMUMachineInitArgs *args)
     env = &cpu->env;
 
     /* allocate RAM */
-    if (ram_size > (2048u << 20)) {
+    if (ram_size > SZ_2G) {
         fprintf(stderr,
-                "qemu: Too much memory for this machine: %d MB, maximum 2048 MB\n",
-                ((unsigned int)ram_size / (1 << 20)));
+                "qemu: Too much memory for this machine: %ld MB, maximum 2048 MB\n",
+                ((unsigned long)ram_size / SZ_1M));
         exit(1);
     }
 
@@ -968,17 +969,17 @@ void mips_malta_init(QEMUMachineInitArgs *args)
 
     /* alias for pre IO hole access */
     memory_region_init_alias(ram_low_preio, NULL, "mips_malta_low_preio.ram",
-                             ram_high, 0, MIN(ram_size, (256 << 20)));
+                             ram_high, 0, MIN(ram_size, SZ_256M));
     memory_region_add_subregion(system_memory, 0, ram_low_preio);
 
     /* alias for post IO hole access, if there is enough RAM */
-    if (ram_size > (512 << 20)) {
+    if (ram_size > SZ_512M) {
         ram_low_postio = g_new(MemoryRegion, 1);
         memory_region_init_alias(ram_low_postio, NULL,
                                  "mips_malta_low_postio.ram",
-                                 ram_high, 512 << 20,
-                                 ram_size - (512 << 20));
-        memory_region_add_subregion(system_memory, 512 << 20, ram_low_postio);
+                                 ram_high, SZ_512M,
+                                 ram_size - SZ_512M);
+        memory_region_add_subregion(system_memory, SZ_512M, ram_low_postio);
     }
 
     /* generate SPD EEPROM data */
@@ -1012,7 +1013,7 @@ void mips_malta_init(QEMUMachineInitArgs *args)
     fl_idx++;
     if (kernel_filename) {
         /* Write a small bootloader to the flash location. */
-        loaderparams.ram_size = MIN(ram_size, 256 << 20);
+        loaderparams.ram_size = MIN(ram_size, SZ_256M);
         loaderparams.kernel_filename = kernel_filename;
         loaderparams.kernel_cmdline = kernel_cmdline;
         loaderparams.initrd_filename = initrd_filename;
diff --git a/include/hw/mips/bios.h b/include/hw/mips/bios.h
index b4b88ac..3d7da4b 100644
--- a/include/hw/mips/bios.h
+++ b/include/hw/mips/bios.h
@@ -1,6 +1,7 @@
 #include "cpu.h"
+#include "qemu/sizes.h"
 
-#define BIOS_SIZE (4 * 1024 * 1024)
+#define BIOS_SIZE SZ_4M
 #ifdef TARGET_WORDS_BIGENDIAN
 #define BIOS_FILENAME "mips_bios.bin"
 #else
-- 
1.8.4.4

Re: [Qemu-devel] [PATCH 2/2] hw/mips: use sizes.h macros

[Andreas Färber]

Am 28.11.2013 07:29, schrieb Antony Pavlov:
> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> Reviewed-by: Richard Henderson <rth@twiddle.net>

Are the int -> long changes caused by the BIT() macro usage? Otherwise I
would've recommended to put that in a follow-up patch.

But either way,

Reviewed-by: Andreas Färber <afaerber@suse.de>

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 2/2] hw/mips: use sizes.h macros

[Stefan Weil]

Am 28.11.2013 07:29, schrieb Antony Pavlov:
> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> Reviewed-by: Richard Henderson <rth@twiddle.net>
> ---
>  hw/mips/mips_malta.c   | 25 +++++++++++++------------
>  include/hw/mips/bios.h |  3 ++-
>  2 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index 05c8771..604832f 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -51,6 +51,7 @@
>  #include "sysemu/qtest.h"
>  #include "qemu/error-report.h"
>  #include "hw/empty_slot.h"
> +#include "qemu/sizes.h"
>  
>  //#define DEBUG_BOARD_INIT
>  
> @@ -63,7 +64,7 @@
>  #define FPGA_ADDRESS  0x1f000000ULL
>  #define RESET_ADDRESS 0x1fc00000ULL
>  
> -#define FLASH_SIZE    0x400000
> +#define FLASH_SIZE    SZ_4M
>  
>  #define MAX_IDE_BUS 2
>  
> @@ -827,8 +828,8 @@ static int64_t load_kernel (void)
>      }
>  
>      prom_set(prom_buf, prom_index++, "memsize");
> -    prom_set(prom_buf, prom_index++, "%i",
> -             MIN(loaderparams.ram_size, 256 << 20));
> +    prom_set(prom_buf, prom_index++, "%li",
> +             MIN(loaderparams.ram_size, SZ_256M));
>      prom_set(prom_buf, prom_index++, "modetty0");
>      prom_set(prom_buf, prom_index++, "38400n8r");
>      prom_set(prom_buf, prom_index++, NULL);
> @@ -954,10 +955,10 @@ void mips_malta_init(QEMUMachineInitArgs *args)
>      env = &cpu->env;
>  
>      /* allocate RAM */
> -    if (ram_size > (2048u << 20)) {
> +    if (ram_size > SZ_2G) {
>          fprintf(stderr,
> -                "qemu: Too much memory for this machine: %d MB, maximum 2048 MB\n",
> -                ((unsigned int)ram_size / (1 << 20)));
> +                "qemu: Too much memory for this machine: %ld MB, maximum 2048 MB\n",
> +                ((unsigned long)ram_size / SZ_1M));
>          exit(1);
>      }
>  
> @@ -968,17 +969,17 @@ void mips_malta_init(QEMUMachineInitArgs *args)
>  
>      /* alias for pre IO hole access */
>      memory_region_init_alias(ram_low_preio, NULL, "mips_malta_low_preio.ram",
> -                             ram_high, 0, MIN(ram_size, (256 << 20)));
> +                             ram_high, 0, MIN(ram_size, SZ_256M));
>      memory_region_add_subregion(system_memory, 0, ram_low_preio);
>  
>      /* alias for post IO hole access, if there is enough RAM */
> -    if (ram_size > (512 << 20)) {
> +    if (ram_size > SZ_512M) {
>          ram_low_postio = g_new(MemoryRegion, 1);
>          memory_region_init_alias(ram_low_postio, NULL,
>                                   "mips_malta_low_postio.ram",
> -                                 ram_high, 512 << 20,
> -                                 ram_size - (512 << 20));
> -        memory_region_add_subregion(system_memory, 512 << 20, ram_low_postio);
> +                                 ram_high, SZ_512M,
> +                                 ram_size - SZ_512M);
> +        memory_region_add_subregion(system_memory, SZ_512M, ram_low_postio);
>      }
>  
>      /* generate SPD EEPROM data */
> @@ -1012,7 +1013,7 @@ void mips_malta_init(QEMUMachineInitArgs *args)
>      fl_idx++;
>      if (kernel_filename) {
>          /* Write a small bootloader to the flash location. */
> -        loaderparams.ram_size = MIN(ram_size, 256 << 20);
> +        loaderparams.ram_size = MIN(ram_size, SZ_256M);
>          loaderparams.kernel_filename = kernel_filename;
>          loaderparams.kernel_cmdline = kernel_cmdline;
>          loaderparams.initrd_filename = initrd_filename;
> diff --git a/include/hw/mips/bios.h b/include/hw/mips/bios.h
> index b4b88ac..3d7da4b 100644
> --- a/include/hw/mips/bios.h
> +++ b/include/hw/mips/bios.h
> @@ -1,6 +1,7 @@
>  #include "cpu.h"
> +#include "qemu/sizes.h"
>  
> -#define BIOS_SIZE (4 * 1024 * 1024)
> +#define BIOS_SIZE SZ_4M
>  #ifdef TARGET_WORDS_BIGENDIAN
>  #define BIOS_FILENAME "mips_bios.bin"
>  #else


What about using (256 * MiB) instead of SZ_256M or (256 << 20)? SZ_256
is better than the last variant, but I prefer the first variant even
more. It is used in QEMU since a long time (for example in eepro100.c
and vdi.c).

Of course the definitions for KiB, MiB, GiB, TiB should be moved from
their current locations to a common header file (I suggest
qemu-common.h) if we agree on using them everywhere.

The SZ_xxx macros might be useful nevertheless, but do we need a new
header file, or could they be added to bitops.h?

Stefan

Re: [Qemu-devel] [PATCH 2/2] hw/mips: use sizes.h macros

[Antony Pavlov]

On Thu, 28 Nov 2013 15:27:18 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 28.11.2013 07:29, schrieb Antony Pavlov:
> > Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> > Reviewed-by: Richard Henderson <rth@twiddle.net>
> 
> Are the int -> long changes caused by the BIT() macro usage? Otherwise I
> would've recommended to put that in a follow-up patch.

Yes, the BIT() macro give us this conversion:

   #define BIT(nr)                 (1UL << (nr))

> But either way,
> 
> Reviewed-by: Andreas Färber <afaerber@suse.de>
> 
> Andreas
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg


-- 
-- 
Best regards,
  Antony Pavlov

Re: [Qemu-devel] [PATCH 2/2] hw/mips: use sizes.h macros

[Antony Pavlov]

On Thu, 28 Nov 2013 18:08:32 +0100
Stefan Weil <sw@weilnetz.de> wrote:

> Am 28.11.2013 07:29, schrieb Antony Pavlov:
> > Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> > Reviewed-by: Richard Henderson <rth@twiddle.net>
> > ---
> >  hw/mips/mips_malta.c   | 25 +++++++++++++------------
> >  include/hw/mips/bios.h |  3 ++-
> >  2 files changed, 15 insertions(+), 13 deletions(-)
> >
> > diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> > index 05c8771..604832f 100644
> > --- a/hw/mips/mips_malta.c
> > +++ b/hw/mips/mips_malta.c
> > @@ -51,6 +51,7 @@
> >  #include "sysemu/qtest.h"
> >  #include "qemu/error-report.h"
> >  #include "hw/empty_slot.h"
> > +#include "qemu/sizes.h"
> >  
> >  //#define DEBUG_BOARD_INIT
> >  
> > @@ -63,7 +64,7 @@
> >  #define FPGA_ADDRESS  0x1f000000ULL
> >  #define RESET_ADDRESS 0x1fc00000ULL
> >  
> > -#define FLASH_SIZE    0x400000
> > +#define FLASH_SIZE    SZ_4M
> >  
> >  #define MAX_IDE_BUS 2
> >  
> > @@ -827,8 +828,8 @@ static int64_t load_kernel (void)
> >      }
> >  
> >      prom_set(prom_buf, prom_index++, "memsize");
> > -    prom_set(prom_buf, prom_index++, "%i",
> > -             MIN(loaderparams.ram_size, 256 << 20));
> > +    prom_set(prom_buf, prom_index++, "%li",
> > +             MIN(loaderparams.ram_size, SZ_256M));
> >      prom_set(prom_buf, prom_index++, "modetty0");
> >      prom_set(prom_buf, prom_index++, "38400n8r");
> >      prom_set(prom_buf, prom_index++, NULL);
> > @@ -954,10 +955,10 @@ void mips_malta_init(QEMUMachineInitArgs *args)
> >      env = &cpu->env;
> >  
> >      /* allocate RAM */
> > -    if (ram_size > (2048u << 20)) {
> > +    if (ram_size > SZ_2G) {
> >          fprintf(stderr,
> > -                "qemu: Too much memory for this machine: %d MB, maximum 2048 MB\n",
> > -                ((unsigned int)ram_size / (1 << 20)));
> > +                "qemu: Too much memory for this machine: %ld MB, maximum 2048 MB\n",
> > +                ((unsigned long)ram_size / SZ_1M));
> >          exit(1);
> >      }
> >  
> > @@ -968,17 +969,17 @@ void mips_malta_init(QEMUMachineInitArgs *args)
> >  
> >      /* alias for pre IO hole access */
> >      memory_region_init_alias(ram_low_preio, NULL, "mips_malta_low_preio.ram",
> > -                             ram_high, 0, MIN(ram_size, (256 << 20)));
> > +                             ram_high, 0, MIN(ram_size, SZ_256M));
> >      memory_region_add_subregion(system_memory, 0, ram_low_preio);
> >  
> >      /* alias for post IO hole access, if there is enough RAM */
> > -    if (ram_size > (512 << 20)) {
> > +    if (ram_size > SZ_512M) {
> >          ram_low_postio = g_new(MemoryRegion, 1);
> >          memory_region_init_alias(ram_low_postio, NULL,
> >                                   "mips_malta_low_postio.ram",
> > -                                 ram_high, 512 << 20,
> > -                                 ram_size - (512 << 20));
> > -        memory_region_add_subregion(system_memory, 512 << 20, ram_low_postio);
> > +                                 ram_high, SZ_512M,
> > +                                 ram_size - SZ_512M);
> > +        memory_region_add_subregion(system_memory, SZ_512M, ram_low_postio);
> >      }
> >  
> >      /* generate SPD EEPROM data */
> > @@ -1012,7 +1013,7 @@ void mips_malta_init(QEMUMachineInitArgs *args)
> >      fl_idx++;
> >      if (kernel_filename) {
> >          /* Write a small bootloader to the flash location. */
> > -        loaderparams.ram_size = MIN(ram_size, 256 << 20);
> > +        loaderparams.ram_size = MIN(ram_size, SZ_256M);
> >          loaderparams.kernel_filename = kernel_filename;
> >          loaderparams.kernel_cmdline = kernel_cmdline;
> >          loaderparams.initrd_filename = initrd_filename;
> > diff --git a/include/hw/mips/bios.h b/include/hw/mips/bios.h
> > index b4b88ac..3d7da4b 100644
> > --- a/include/hw/mips/bios.h
> > +++ b/include/hw/mips/bios.h
> > @@ -1,6 +1,7 @@
> >  #include "cpu.h"
> > +#include "qemu/sizes.h"
> >  
> > -#define BIOS_SIZE (4 * 1024 * 1024)
> > +#define BIOS_SIZE SZ_4M
> >  #ifdef TARGET_WORDS_BIGENDIAN
> >  #define BIOS_FILENAME "mips_bios.bin"
> >  #else
> 
> 
> What about using (256 * MiB) instead of SZ_256M or (256 << 20)? SZ_256
> is better than the last variant, but I prefer the first variant even
> more. It is used in QEMU since a long time (for example in eepro100.c
> and vdi.c).
> 
> Of course the definitions for KiB, MiB, GiB, TiB should be moved from
> their current locations to a common header file (I suggest
> qemu-common.h) if we agree on using them everywhere.
> 
> The SZ_xxx macros might be useful nevertheless, but do we need a new
> header file, or could they be added to bitops.h?

sizes.h is widely used inside linux kernel, u-boot and barebox so it is a de facto standard header file. IMHO it's better to keep it as a separate header file.

-- 
Best regards,
  Antony Pavlov