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?
>
> -/* 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 :)
