On 09.08.2017 11:05, Cornelia Huck wrote: > On Wed, 9 Aug 2017 06:59:37 +0200 > Thomas Huth <th...@redhat.com> 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 <m...@redhat.com> >> Signed-off-by: Thomas Huth <th...@redhat.com> >> --- >> 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