Re: [Qemu-devel] [PATCH 1/2] tests/boot-sector: Do not overwrite the x86 buffer on other architectures

[Cornelia Huck]

On Wed,  9 Aug 2017 06:59:37 +0200
Thomas Huth <address@hidden> wrote:

> Re-using the boot_sector code buffer from x86 for other architectures
> is not very nice, especially if we add more architectures later. It's
> also ugly that the test uses a huge pre-initialized array - the size
> of the executable is very huge due to this array. So let's use a
> separate buffer for each architecture instead, allocated from the heap,
> so that we really just use the memory that we need.
> 
> Suggested-by: Michael Tsirkin <address@hidden>
> Signed-off-by: Thomas Huth <address@hidden>
> ---
>  tests/boot-sector.c | 37 ++++++++++++++++++++++++-------------
>  1 file changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/tests/boot-sector.c b/tests/boot-sector.c
> index e3880f4..4ea1373 100644
> --- a/tests/boot-sector.c
> +++ b/tests/boot-sector.c
> @@ -21,13 +21,12 @@
>  #define SIGNATURE 0xdead
>  #define SIGNATURE_OFFSET 0x10
>  #define BOOT_SECTOR_ADDRESS 0x7c00
> +#define SIGNATURE_ADDR (BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET)

Do you want to use this new #define in boot_sector_test() as well?

>  
> -/* Boot sector code: write SIGNATURE into memory,
> +/* x86 boot sector code: write SIGNATURE into memory,
>   * then halt.
> - * Q35 machine requires a minimum 0x7e000 bytes disk.
> - * (bug or feature?)
>   */
> -static uint8_t boot_sector[0x7e000] = {
> +static uint8_t x86_boot_sector[512] = {
>      /* The first sector will be placed at RAM address 00007C00, and
>       * the BIOS transfers control to 00007C00
>       */
> @@ -50,8 +49,8 @@ static uint8_t boot_sector[0x7e000] = {
>      [0x07] = HIGH(SIGNATURE),
>      /* 7c08:  mov %ax,0x7c10 */
>      [0x08] = 0xa3,
> -    [0x09] = LOW(BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET),
> -    [0x0a] = HIGH(BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET),
> +    [0x09] = LOW(SIGNATURE_ADDR),
> +    [0x0a] = HIGH(SIGNATURE_ADDR),
>  
>      /* 7c0b cli */
>      [0x0b] = 0xfa,
> @@ -72,7 +71,9 @@ static uint8_t boot_sector[0x7e000] = {
>  int boot_sector_init(char *fname)
>  {
>      int fd, ret;
> -    size_t len = sizeof boot_sector;
> +    size_t len;
> +    char *boot_code;
> +    const char *arch = qtest_get_arch();
>  
>      fd = mkstemp(fname);
>      if (fd < 0) {
> @@ -80,16 +81,26 @@ int boot_sector_init(char *fname)
>          return 1;
>      }
>  
> -    /* For Open Firmware based system, we can use a Forth script instead */
> -    if (strcmp(qtest_get_arch(), "ppc64") == 0) {
> -        len = sprintf((char *)boot_sector, "\\ Bootscript\n%x %x c! %x %x 
> c!\n",
> -                LOW(SIGNATURE), BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET,
> -                HIGH(SIGNATURE), BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET + 1);
> +    if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) {
> +        /* Q35 requires a minimum 0x7e000 bytes disk (bug or feature?) */
> +        len = 0x7e000;

Use the maximum of (0x7e000, sizeof(x86_boot_sector))? (Not that it is
likely that the boot sector will ever grow, but I think it is cleaner.)

> +        boot_code = g_malloc(len);

Would g_malloc_0() be better?

> +        memcpy(boot_code, x86_boot_sector, sizeof x86_boot_sector);

sizeof(x86_boot_sector)?

> +    } else if (g_str_equal(arch, "ppc64")) {
> +        /* For Open Firmware based system, use a Forth script */
> +        boot_code = g_strdup_printf("\\ Bootscript\n%x %x c! %x %x c!\n",
> +                                    LOW(SIGNATURE), SIGNATURE_ADDR,
> +                                    HIGH(SIGNATURE), SIGNATURE_ADDR + 1);
> +        len = strlen(boot_code);
> +    } else {
> +        g_assert_not_reached();
>      }
>  
> -    ret = write(fd, boot_sector, len);
> +    ret = write(fd, boot_code, len);
>      close(fd);
>  
> +    g_free(boot_code);
> +
>      if (ret != len) {
>          fprintf(stderr, "Could not write \"%s\"", fname);
>          return 1;

This makes the code much nicer :)