Re: [Qemu-devel] [PATCH 1/2] tests/boot-sector: Do not overwrite the x86 buffer on other architectures
On Wed, 9 Aug 2017 11:18:33 +0200
Thomas Huth wrote:
> On 09.08.2017 11:05, Cornelia Huck wrote:
> > On Wed, 9 Aug 2017 06:59:37 +0200
> > Thomas Huth wrote:
> >> @@ -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.
It's probably a bit paranoid, but I don't think it hurts.
>
> >> +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.
After your patch, the original sizeof callers are gone, no? (I really
prefer the sizeof() variant...)
Re: [Qemu-devel] [PATCH 1/2] tests/boot-sector: Do not overwrite the x86 buffer on other architectures
On 09.08.2017 11:05, Cornelia Huck wrote:
> On Wed, 9 Aug 2017 06:59:37 +0200
> Thomas Huth 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
>> Signed-off-by: Thomas Huth
>> ---
>> 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 7C00, and
>> * the BIOS transfers control to 7C00
>> */
>> @@ -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
Re: [Qemu-devel] [PATCH 1/2] tests/boot-sector: Do not overwrite the x86 buffer on other architectures
On Wed, 9 Aug 2017 06:59:37 +0200
Thomas Huth 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
> Signed-off-by: Thomas Huth
> ---
> 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 7C00, and
> * the BIOS transfers control to 7C00
> */
> @@ -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 :)
