[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] tests/boot-sector: Do not overwrite the x86
From: |
Thomas Huth |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] tests/boot-sector: Do not overwrite the x86 buffer on other architectures |
Date: |
Wed, 9 Aug 2017 11:18:33 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 |
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