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

[Thomas Huth]

On 09.08.2017 11:05, Cornelia Huck wrote:
> 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?

Yes, sounds like a good idea.

>>  
>> -/* 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.)

Sounds like a little bit of too much sanity checking for me, but ok, I
can add it.

>> +        boot_code = g_malloc(len);
> 
> Would g_malloc_0() be better?

Good idea, the test is likely more predictable if we don't have random
data in the file here (it should not really matter, but let's better be
safe than sorry).

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

The original code uses sizeof without parenthesis, so I think we should
stay with that coding style.

>> +    } 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 :)

Thanks for the review!

I'll wait for some more feedback, then send a v2...

 Thomas