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 <address@hidden>
> Reviewed-by: Richard Henderson <address@hidden>
> ---
>  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