* [Qemu-devel] [PATCH] test: port postcopy test to ppc64
@ 2016-07-21 10:12 Laurent Vivier
2016-07-21 13:16 ` Thomas Huth
0 siblings, 1 reply; 5+ messages in thread
From: Laurent Vivier @ 2016-07-21 10:12 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, thuth, dgibson, Laurent Vivier
As userfaultfd syscall is available on powerpc, migration
postcopy can be used.
This patch adds the support needed to test this on powerpc,
instead of using a bootsector to run code to modify memory,
we use a FORTH script in "boot-command" property.
As spapr machine doesn't support "-prom-env" argument
(the nvram is initialized by SLOF and not by QEMU),
"boot-command" is provided to SLOF via a file mapped nvram
(with "-drive file=...,if=pflash")
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
tests/Makefile.include | 1 +
tests/postcopy-test.c | 141 ++++++++++++++++++++++++++++++++++++++++++-------
2 files changed, 123 insertions(+), 19 deletions(-)
diff --git a/tests/Makefile.include b/tests/Makefile.include
index e7e50d6..e2d1885 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -268,6 +268,7 @@ check-qtest-sparc-y += tests/prom-env-test$(EXESUF)
#check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
+check-qtest-ppc64-y += tests/postcopy-test$(EXESUF)
check-qtest-generic-y += tests/qom-test$(EXESUF)
diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
index 16465ab..439afd9 100644
--- a/tests/postcopy-test.c
+++ b/tests/postcopy-test.c
@@ -19,6 +19,24 @@
#include "sysemu/char.h"
#include "sysemu/sysemu.h"
+/* These structures are already defined by OpenBIOS and usable with SLOF */
+#define NVRAM_PART_SYSTEM 0x70
+struct nvpart {
+ uint8_t signature;
+ uint8_t checksum;
+ uint16_t len; /* BE, length divided by 16 */
+ char name[12];
+ char content[0];
+};
+
+#define MIN_NVRAM_SIZE 8192 /* from spapr_nvram.c */
+
+/* FW_MAX_SIZE is 4MB, but slof.bin is only 900KB,
+ * so let's modify memory between 1MB and 100MB
+ * to do like PC bootsector
+ */
+#define FORTH_BOOTSCRIPT "boot-command=hex .\" _\" begin %x %x do i c@ 1 + i c! 1000 +loop .\" B\" 0 until"
+
const unsigned start_address = 1024 * 1024;
const unsigned end_address = 100 * 1024 * 1024;
bool got_stop;
@@ -122,6 +140,52 @@ unsigned char bootsect[] = {
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x55, 0xaa
};
+static void init_bootfile_x86(const char *bootpath)
+{
+ FILE *bootfile = fopen(bootpath, "wb");
+
+ g_assert_cmpint(fwrite(bootsect, 512, 1, bootfile), ==, 1);
+ fclose(bootfile);
+}
+
+static void nvpart_checksum(struct nvpart *header)
+{
+ unsigned int i, sum;
+ uint8_t *tmpptr;
+
+ tmpptr = (uint8_t *)header;
+ sum = *tmpptr;
+ for (i = 0; i < 14; i++) {
+ sum += tmpptr[2 + i];
+ sum = (sum + ((sum & 0xff00) >> 8)) & 0xff;
+ }
+ header->checksum = sum & 0xff;
+}
+
+static void init_bootfile_ppc(const char *bootpath)
+{
+ FILE *bootfile;
+ char buf[MIN_NVRAM_SIZE];
+ struct nvpart *header = (struct nvpart *)buf;
+
+ memset(buf, 0, MIN_NVRAM_SIZE);
+
+ /* Create a "common" partition in nvram to store boot-command property */
+
+ header->signature = NVRAM_PART_SYSTEM;
+ memcpy(header->name, "common", 6);
+ header->len = cpu_to_be16(MIN_NVRAM_SIZE >> 4);
+ nvpart_checksum(header); /* can change if we change header->len */
+
+ sprintf(header->content, FORTH_BOOTSCRIPT, end_address, start_address);
+
+ /* Write partition to the NVRAM file */
+
+ bootfile = fopen(bootpath, "wb");
+ g_assert_cmpint(fwrite(buf, MIN_NVRAM_SIZE, 1, bootfile), ==, 1);
+ fclose(bootfile);
+}
+
/*
* Wait for some output in the serial output file,
* we get an 'A' followed by an endless string of 'B's
@@ -131,10 +195,31 @@ static void wait_for_serial(const char *side)
{
char *serialpath = g_strdup_printf("%s/%s", tmpfs, side);
FILE *serialfile = fopen(serialpath, "r");
+ const char *arch = qtest_get_arch();
+ int started = (strcmp(side, "src_serial") == 0 &&
+ strcmp(arch, "ppc64") == 0) ? 0 : 1;
do {
int readvalue = fgetc(serialfile);
+ if (!started) {
+ /* SLOF prints its banner before starting test,
+ * to ignore it, mark the start of the test with '_',
+ * ignore all characters until this marker
+ */
+ switch (readvalue) {
+ case '_':
+ started = 1;
+ break;
+ case EOF:
+ fseek(serialfile, 0, SEEK_SET);
+ usleep(1000);
+ break;
+ default:
+ break;
+ }
+ continue;
+ }
switch (readvalue) {
case 'A':
/* Fine */
@@ -147,6 +232,8 @@ static void wait_for_serial(const char *side)
return;
case EOF:
+ started = (strcmp(side, "src_serial") == 0 &&
+ strcmp(arch, "ppc64") == 0) ? 0 : 1;
fseek(serialfile, 0, SEEK_SET);
usleep(1000);
break;
@@ -295,32 +382,48 @@ static void test_migrate(void)
char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
QTestState *global = global_qtest, *from, *to;
unsigned char dest_byte_a, dest_byte_b, dest_byte_c, dest_byte_d;
- gchar *cmd;
+ gchar *cmd, *cmd_src, *cmd_dst;
QDict *rsp;
char *bootpath = g_strdup_printf("%s/bootsect", tmpfs);
- FILE *bootfile = fopen(bootpath, "wb");
+ const char *arch = qtest_get_arch();
got_stop = false;
- g_assert_cmpint(fwrite(bootsect, 512, 1, bootfile), ==, 1);
- fclose(bootfile);
- cmd = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
- " -name pcsource,debug-threads=on"
- " -serial file:%s/src_serial"
- " -drive file=%s,format=raw",
- tmpfs, bootpath);
- from = qtest_start(cmd);
- g_free(cmd);
+ if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
+ init_bootfile_x86(bootpath);
+ cmd_src = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
+ " -name pcsource,debug-threads=on"
+ " -serial file:%s/src_serial"
+ " -drive file=%s,format=raw",
+ tmpfs, bootpath);
+ cmd_dst = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
+ " -name pcdest,debug-threads=on"
+ " -serial file:%s/dest_serial"
+ " -drive file=%s,format=raw"
+ " -incoming %s",
+ tmpfs, bootpath, uri);
+ } else if (strcmp(arch, "ppc64") == 0) {
+ init_bootfile_ppc(bootpath);
+ cmd_src = g_strdup_printf("-machine accel=kvm:tcg -m 256M"
+ " -name pcsource,debug-threads=on"
+ " -serial file:%s/src_serial"
+ " -drive file=%s,if=pflash,format=raw",
+ tmpfs, bootpath);
+ cmd_dst = g_strdup_printf("-machine accel=kvm:tcg -m 256M"
+ " -name pcdest,debug-threads=on"
+ " -serial file:%s/dest_serial"
+ " -incoming %s",
+ tmpfs, uri);
+ } else {
+ g_assert_not_reached();
+ }
- cmd = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
- " -name pcdest,debug-threads=on"
- " -serial file:%s/dest_serial"
- " -drive file=%s,format=raw"
- " -incoming %s",
- tmpfs, bootpath, uri);
- to = qtest_init(cmd);
- g_free(cmd);
+ from = qtest_start(cmd_src);
+ g_free(cmd_src);
+
+ to = qtest_init(cmd_dst);
+ g_free(cmd_dst);
global_qtest = from;
rsp = qmp("{ 'execute': 'migrate-set-capabilities',"
--
2.5.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] test: port postcopy test to ppc64
2016-07-21 10:12 [Qemu-devel] [PATCH] test: port postcopy test to ppc64 Laurent Vivier
@ 2016-07-21 13:16 ` Thomas Huth
2016-07-21 13:46 ` Laurent Vivier
2016-07-21 16:13 ` Laurent Vivier
0 siblings, 2 replies; 5+ messages in thread
From: Thomas Huth @ 2016-07-21 13:16 UTC (permalink / raw)
To: Laurent Vivier, qemu-devel; +Cc: dgilbert, dgibson
On 21.07.2016 12:12, Laurent Vivier wrote:
> As userfaultfd syscall is available on powerpc, migration
> postcopy can be used.
Good idea to add this test for PPC, too!
> This patch adds the support needed to test this on powerpc,
> instead of using a bootsector to run code to modify memory,
> we use a FORTH script in "boot-command" property.
>
> As spapr machine doesn't support "-prom-env" argument
> (the nvram is initialized by SLOF and not by QEMU),
> "boot-command" is provided to SLOF via a file mapped nvram
> (with "-drive file=...,if=pflash")
I wonder whether we could easily add support for the "-prom-env"
parameter for the sPAPR machine, too, since the NVRAM layout seems to be
pretty much the same as on the old CHRP Mac machines...?
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
> tests/Makefile.include | 1 +
> tests/postcopy-test.c | 141 ++++++++++++++++++++++++++++++++++++++++++-------
> 2 files changed, 123 insertions(+), 19 deletions(-)
>
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index e7e50d6..e2d1885 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -268,6 +268,7 @@ check-qtest-sparc-y += tests/prom-env-test$(EXESUF)
> #check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
> check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
> check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
> +check-qtest-ppc64-y += tests/postcopy-test$(EXESUF)
>
> check-qtest-generic-y += tests/qom-test$(EXESUF)
>
> diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
> index 16465ab..439afd9 100644
> --- a/tests/postcopy-test.c
> +++ b/tests/postcopy-test.c
> @@ -19,6 +19,24 @@
> #include "sysemu/char.h"
> #include "sysemu/sysemu.h"
>
> +/* These structures are already defined by OpenBIOS and usable with SLOF */
> +#define NVRAM_PART_SYSTEM 0x70
> +struct nvpart {
> + uint8_t signature;
> + uint8_t checksum;
> + uint16_t len; /* BE, length divided by 16 */
> + char name[12];
> + char content[0];
IIRC zero-sized arrays are a GCC extension ... for valid C99, it might
be better to use "char content[]" instead?
> +};
>
> +#define MIN_NVRAM_SIZE 8192 /* from spapr_nvram.c */
> +
> +/* FW_MAX_SIZE is 4MB, but slof.bin is only 900KB,
> + * so let's modify memory between 1MB and 100MB
> + * to do like PC bootsector
> + */
> +#define FORTH_BOOTSCRIPT "boot-command=hex .\" _\" begin %x %x do i c@ 1 + i c! 1000 +loop .\" B\" 0 until"
Just a matter of taste, but I somewhat dislike the idea of hiding a
string with format parameters in a macro ... I think I'd rather place
this string directly into the corresponding sprintf() statement below
instead.
> const unsigned start_address = 1024 * 1024;
> const unsigned end_address = 100 * 1024 * 1024;
> bool got_stop;
> @@ -122,6 +140,52 @@ unsigned char bootsect[] = {
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x55, 0xaa
> };
>
> +static void init_bootfile_x86(const char *bootpath)
> +{
> + FILE *bootfile = fopen(bootpath, "wb");
> +
> + g_assert_cmpint(fwrite(bootsect, 512, 1, bootfile), ==, 1);
> + fclose(bootfile);
> +}
> +
> +static void nvpart_checksum(struct nvpart *header)
> +{
> + unsigned int i, sum;
> + uint8_t *tmpptr;
> +
> + tmpptr = (uint8_t *)header;
> + sum = *tmpptr;
> + for (i = 0; i < 14; i++) {
> + sum += tmpptr[2 + i];
> + sum = (sum + ((sum & 0xff00) >> 8)) & 0xff;
> + }
> + header->checksum = sum & 0xff;
> +}
Have you tried to include openbios_firmware_abi.h instead and use
OpenBIOS_finish_partition() here? That would avoid to have this code
duplicated.
> +static void init_bootfile_ppc(const char *bootpath)
> +{
> + FILE *bootfile;
> + char buf[MIN_NVRAM_SIZE];
> + struct nvpart *header = (struct nvpart *)buf;
> +
> + memset(buf, 0, MIN_NVRAM_SIZE);
> +
> + /* Create a "common" partition in nvram to store boot-command property */
> +
> + header->signature = NVRAM_PART_SYSTEM;
> + memcpy(header->name, "common", 6);
> + header->len = cpu_to_be16(MIN_NVRAM_SIZE >> 4);
> + nvpart_checksum(header); /* can change if we change header->len */
> +
> + sprintf(header->content, FORTH_BOOTSCRIPT, end_address, start_address);
> +
> + /* Write partition to the NVRAM file */
> +
> + bootfile = fopen(bootpath, "wb");
> + g_assert_cmpint(fwrite(buf, MIN_NVRAM_SIZE, 1, bootfile), ==, 1);
> + fclose(bootfile);
> +}
> +
> /*
> * Wait for some output in the serial output file,
> * we get an 'A' followed by an endless string of 'B's
> @@ -131,10 +195,31 @@ static void wait_for_serial(const char *side)
> {
> char *serialpath = g_strdup_printf("%s/%s", tmpfs, side);
> FILE *serialfile = fopen(serialpath, "r");
> + const char *arch = qtest_get_arch();
> + int started = (strcmp(side, "src_serial") == 0 &&
> + strcmp(arch, "ppc64") == 0) ? 0 : 1;
>
> do {
> int readvalue = fgetc(serialfile);
>
> + if (!started) {
> + /* SLOF prints its banner before starting test,
> + * to ignore it, mark the start of the test with '_',
> + * ignore all characters until this marker
> + */
> + switch (readvalue) {
> + case '_':
> + started = 1;
> + break;
> + case EOF:
> + fseek(serialfile, 0, SEEK_SET);
> + usleep(1000);
> + break;
> + default:
> + break;
I think you could remove that default case.
> + }
> + continue;
> + }
> switch (readvalue) {
> case 'A':
> /* Fine */
> @@ -147,6 +232,8 @@ static void wait_for_serial(const char *side)
> return;
>
> case EOF:
> + started = (strcmp(side, "src_serial") == 0 &&
> + strcmp(arch, "ppc64") == 0) ? 0 : 1;
I somehow fail to see why is this needed here again? Isn't the initial
setup of "started" at the beginning of the function enough?
> fseek(serialfile, 0, SEEK_SET);
> usleep(1000);
> break;
> @@ -295,32 +382,48 @@ static void test_migrate(void)
> char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
> QTestState *global = global_qtest, *from, *to;
> unsigned char dest_byte_a, dest_byte_b, dest_byte_c, dest_byte_d;
> - gchar *cmd;
> + gchar *cmd, *cmd_src, *cmd_dst;
> QDict *rsp;
>
> char *bootpath = g_strdup_printf("%s/bootsect", tmpfs);
> - FILE *bootfile = fopen(bootpath, "wb");
> + const char *arch = qtest_get_arch();
>
> got_stop = false;
> - g_assert_cmpint(fwrite(bootsect, 512, 1, bootfile), ==, 1);
> - fclose(bootfile);
>
> - cmd = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
> - " -name pcsource,debug-threads=on"
> - " -serial file:%s/src_serial"
> - " -drive file=%s,format=raw",
> - tmpfs, bootpath);
> - from = qtest_start(cmd);
> - g_free(cmd);
> + if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> + init_bootfile_x86(bootpath);
> + cmd_src = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
> + " -name pcsource,debug-threads=on"
> + " -serial file:%s/src_serial"
> + " -drive file=%s,format=raw",
> + tmpfs, bootpath);
> + cmd_dst = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
> + " -name pcdest,debug-threads=on"
> + " -serial file:%s/dest_serial"
> + " -drive file=%s,format=raw"
> + " -incoming %s",
> + tmpfs, bootpath, uri);
> + } else if (strcmp(arch, "ppc64") == 0) {
> + init_bootfile_ppc(bootpath);
> + cmd_src = g_strdup_printf("-machine accel=kvm:tcg -m 256M"
> + " -name pcsource,debug-threads=on"
> + " -serial file:%s/src_serial"
> + " -drive file=%s,if=pflash,format=raw",
> + tmpfs, bootpath);
> + cmd_dst = g_strdup_printf("-machine accel=kvm:tcg -m 256M"
> + " -name pcdest,debug-threads=on"
> + " -serial file:%s/dest_serial"
> + " -incoming %s",
> + tmpfs, uri);
> + } else {
> + g_assert_not_reached();
> + }
>
> - cmd = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
> - " -name pcdest,debug-threads=on"
> - " -serial file:%s/dest_serial"
> - " -drive file=%s,format=raw"
> - " -incoming %s",
> - tmpfs, bootpath, uri);
> - to = qtest_init(cmd);
> - g_free(cmd);
> + from = qtest_start(cmd_src);
> + g_free(cmd_src);
> +
> + to = qtest_init(cmd_dst);
> + g_free(cmd_dst);
>
> global_qtest = from;
> rsp = qmp("{ 'execute': 'migrate-set-capabilities',"
>
Thomas
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] test: port postcopy test to ppc64
2016-07-21 13:16 ` Thomas Huth
@ 2016-07-21 13:46 ` Laurent Vivier
2016-07-21 21:02 ` Thomas Huth
2016-07-21 16:13 ` Laurent Vivier
1 sibling, 1 reply; 5+ messages in thread
From: Laurent Vivier @ 2016-07-21 13:46 UTC (permalink / raw)
To: Thomas Huth, qemu-devel; +Cc: dgilbert, dgibson
On 21/07/2016 15:16, Thomas Huth wrote:
> On 21.07.2016 12:12, Laurent Vivier wrote:
>> As userfaultfd syscall is available on powerpc, migration
>> postcopy can be used.
>
> Good idea to add this test for PPC, too!
The idea comes from David Gilbert :)
Thanks David!
>
>> This patch adds the support needed to test this on powerpc,
>> instead of using a bootsector to run code to modify memory,
>> we use a FORTH script in "boot-command" property.
>>
>> As spapr machine doesn't support "-prom-env" argument
>> (the nvram is initialized by SLOF and not by QEMU),
>> "boot-command" is provided to SLOF via a file mapped nvram
>> (with "-drive file=...,if=pflash")
>
> I wonder whether we could easily add support for the "-prom-env"
> parameter for the sPAPR machine, too, since the NVRAM layout seems to be
> pretty much the same as on the old CHRP Mac machines...?
The only thing we need to have "-prom-env" is to initialize the nvram
partitions in QEMU if they don't exist.
I didn't follow this way because this work is normally to be done by the
firmware not by QEMU. If we do it in QEMU, SLOF will do not, and we
can't be sure we are doing the things well, as SLOF does.
On Mac, it is another history, originally, firmware was OpenHackware,
not OpenBIOS, and OH was only here to allow to boot the kernel nothing
more (it was really a hack: there was not forth interpreter [oh, my god!]).
So all the important stuffs were done by QEMU, like to initialize the
NVRAM, the PCI bus, ... and this allows to set values in the environment
variables.
But if you think it is viable, it should be really easy to do (like
cut'n'paste from OpenBIOS ABI)...
Thanks,
Laurent
>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>> tests/Makefile.include | 1 +
>> tests/postcopy-test.c | 141 ++++++++++++++++++++++++++++++++++++++++++-------
>> 2 files changed, 123 insertions(+), 19 deletions(-)
>>
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index e7e50d6..e2d1885 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -268,6 +268,7 @@ check-qtest-sparc-y += tests/prom-env-test$(EXESUF)
>> #check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
>> check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
>> check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
>> +check-qtest-ppc64-y += tests/postcopy-test$(EXESUF)
>>
>> check-qtest-generic-y += tests/qom-test$(EXESUF)
>>
>> diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
>> index 16465ab..439afd9 100644
>> --- a/tests/postcopy-test.c
>> +++ b/tests/postcopy-test.c
>> @@ -19,6 +19,24 @@
>> #include "sysemu/char.h"
>> #include "sysemu/sysemu.h"
>>
>> +/* These structures are already defined by OpenBIOS and usable with SLOF */
>> +#define NVRAM_PART_SYSTEM 0x70
>> +struct nvpart {
>> + uint8_t signature;
>> + uint8_t checksum;
>> + uint16_t len; /* BE, length divided by 16 */
>> + char name[12];
>> + char content[0];
>
> IIRC zero-sized arrays are a GCC extension ... for valid C99, it might
> be better to use "char content[]" instead?
>
>> +};
>>
>> +#define MIN_NVRAM_SIZE 8192 /* from spapr_nvram.c */
>> +
>> +/* FW_MAX_SIZE is 4MB, but slof.bin is only 900KB,
>> + * so let's modify memory between 1MB and 100MB
>> + * to do like PC bootsector
>> + */
>> +#define FORTH_BOOTSCRIPT "boot-command=hex .\" _\" begin %x %x do i c@ 1 + i c! 1000 +loop .\" B\" 0 until"
>
> Just a matter of taste, but I somewhat dislike the idea of hiding a
> string with format parameters in a macro ... I think I'd rather place
> this string directly into the corresponding sprintf() statement below
> instead.
>
>> const unsigned start_address = 1024 * 1024;
>> const unsigned end_address = 100 * 1024 * 1024;
>> bool got_stop;
>> @@ -122,6 +140,52 @@ unsigned char bootsect[] = {
>> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x55, 0xaa
>> };
>>
>> +static void init_bootfile_x86(const char *bootpath)
>> +{
>> + FILE *bootfile = fopen(bootpath, "wb");
>> +
>> + g_assert_cmpint(fwrite(bootsect, 512, 1, bootfile), ==, 1);
>> + fclose(bootfile);
>> +}
>> +
>> +static void nvpart_checksum(struct nvpart *header)
>> +{
>> + unsigned int i, sum;
>> + uint8_t *tmpptr;
>> +
>> + tmpptr = (uint8_t *)header;
>> + sum = *tmpptr;
>> + for (i = 0; i < 14; i++) {
>> + sum += tmpptr[2 + i];
>> + sum = (sum + ((sum & 0xff00) >> 8)) & 0xff;
>> + }
>> + header->checksum = sum & 0xff;
>> +}
>
> Have you tried to include openbios_firmware_abi.h instead and use
> OpenBIOS_finish_partition() here? That would avoid to have this code
> duplicated.
>
>> +static void init_bootfile_ppc(const char *bootpath)
>> +{
>> + FILE *bootfile;
>> + char buf[MIN_NVRAM_SIZE];
>> + struct nvpart *header = (struct nvpart *)buf;
>> +
>> + memset(buf, 0, MIN_NVRAM_SIZE);
>> +
>> + /* Create a "common" partition in nvram to store boot-command property */
>> +
>> + header->signature = NVRAM_PART_SYSTEM;
>> + memcpy(header->name, "common", 6);
>> + header->len = cpu_to_be16(MIN_NVRAM_SIZE >> 4);
>> + nvpart_checksum(header); /* can change if we change header->len */
>> +
>> + sprintf(header->content, FORTH_BOOTSCRIPT, end_address, start_address);
>> +
>> + /* Write partition to the NVRAM file */
>> +
>> + bootfile = fopen(bootpath, "wb");
>> + g_assert_cmpint(fwrite(buf, MIN_NVRAM_SIZE, 1, bootfile), ==, 1);
>> + fclose(bootfile);
>> +}
>> +
>> /*
>> * Wait for some output in the serial output file,
>> * we get an 'A' followed by an endless string of 'B's
>> @@ -131,10 +195,31 @@ static void wait_for_serial(const char *side)
>> {
>> char *serialpath = g_strdup_printf("%s/%s", tmpfs, side);
>> FILE *serialfile = fopen(serialpath, "r");
>> + const char *arch = qtest_get_arch();
>> + int started = (strcmp(side, "src_serial") == 0 &&
>> + strcmp(arch, "ppc64") == 0) ? 0 : 1;
>>
>> do {
>> int readvalue = fgetc(serialfile);
>>
>> + if (!started) {
>> + /* SLOF prints its banner before starting test,
>> + * to ignore it, mark the start of the test with '_',
>> + * ignore all characters until this marker
>> + */
>> + switch (readvalue) {
>> + case '_':
>> + started = 1;
>> + break;
>> + case EOF:
>> + fseek(serialfile, 0, SEEK_SET);
>> + usleep(1000);
>> + break;
>> + default:
>> + break;
>
> I think you could remove that default case.
>
>> + }
>> + continue;
>> + }
>> switch (readvalue) {
>> case 'A':
>> /* Fine */
>> @@ -147,6 +232,8 @@ static void wait_for_serial(const char *side)
>> return;
>>
>> case EOF:
>> + started = (strcmp(side, "src_serial") == 0 &&
>> + strcmp(arch, "ppc64") == 0) ? 0 : 1;
>
> I somehow fail to see why is this needed here again? Isn't the initial
> setup of "started" at the beginning of the function enough?
>
>> fseek(serialfile, 0, SEEK_SET);
>> usleep(1000);
>> break;
>> @@ -295,32 +382,48 @@ static void test_migrate(void)
>> char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
>> QTestState *global = global_qtest, *from, *to;
>> unsigned char dest_byte_a, dest_byte_b, dest_byte_c, dest_byte_d;
>> - gchar *cmd;
>> + gchar *cmd, *cmd_src, *cmd_dst;
>> QDict *rsp;
>>
>> char *bootpath = g_strdup_printf("%s/bootsect", tmpfs);
>> - FILE *bootfile = fopen(bootpath, "wb");
>> + const char *arch = qtest_get_arch();
>>
>> got_stop = false;
>> - g_assert_cmpint(fwrite(bootsect, 512, 1, bootfile), ==, 1);
>> - fclose(bootfile);
>>
>> - cmd = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
>> - " -name pcsource,debug-threads=on"
>> - " -serial file:%s/src_serial"
>> - " -drive file=%s,format=raw",
>> - tmpfs, bootpath);
>> - from = qtest_start(cmd);
>> - g_free(cmd);
>> + if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>> + init_bootfile_x86(bootpath);
>> + cmd_src = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
>> + " -name pcsource,debug-threads=on"
>> + " -serial file:%s/src_serial"
>> + " -drive file=%s,format=raw",
>> + tmpfs, bootpath);
>> + cmd_dst = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
>> + " -name pcdest,debug-threads=on"
>> + " -serial file:%s/dest_serial"
>> + " -drive file=%s,format=raw"
>> + " -incoming %s",
>> + tmpfs, bootpath, uri);
>> + } else if (strcmp(arch, "ppc64") == 0) {
>> + init_bootfile_ppc(bootpath);
>> + cmd_src = g_strdup_printf("-machine accel=kvm:tcg -m 256M"
>> + " -name pcsource,debug-threads=on"
>> + " -serial file:%s/src_serial"
>> + " -drive file=%s,if=pflash,format=raw",
>> + tmpfs, bootpath);
>> + cmd_dst = g_strdup_printf("-machine accel=kvm:tcg -m 256M"
>> + " -name pcdest,debug-threads=on"
>> + " -serial file:%s/dest_serial"
>> + " -incoming %s",
>> + tmpfs, uri);
>> + } else {
>> + g_assert_not_reached();
>> + }
>>
>> - cmd = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
>> - " -name pcdest,debug-threads=on"
>> - " -serial file:%s/dest_serial"
>> - " -drive file=%s,format=raw"
>> - " -incoming %s",
>> - tmpfs, bootpath, uri);
>> - to = qtest_init(cmd);
>> - g_free(cmd);
>> + from = qtest_start(cmd_src);
>> + g_free(cmd_src);
>> +
>> + to = qtest_init(cmd_dst);
>> + g_free(cmd_dst);
>>
>> global_qtest = from;
>> rsp = qmp("{ 'execute': 'migrate-set-capabilities',"
>>
>
> Thomas
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] test: port postcopy test to ppc64
2016-07-21 13:16 ` Thomas Huth
2016-07-21 13:46 ` Laurent Vivier
@ 2016-07-21 16:13 ` Laurent Vivier
1 sibling, 0 replies; 5+ messages in thread
From: Laurent Vivier @ 2016-07-21 16:13 UTC (permalink / raw)
To: Thomas Huth, qemu-devel; +Cc: dgibson, dgilbert
On 21/07/2016 15:16, Thomas Huth wrote:
> On 21.07.2016 12:12, Laurent Vivier wrote:
...
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index e7e50d6..e2d1885 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -268,6 +268,7 @@ check-qtest-sparc-y += tests/prom-env-test$(EXESUF)
>> #check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
>> check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
>> check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
>> +check-qtest-ppc64-y += tests/postcopy-test$(EXESUF)
>>
>> check-qtest-generic-y += tests/qom-test$(EXESUF)
>>
>> diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
>> index 16465ab..439afd9 100644
>> --- a/tests/postcopy-test.c
>> +++ b/tests/postcopy-test.c
>> @@ -19,6 +19,24 @@
>> #include "sysemu/char.h"
>> #include "sysemu/sysemu.h"
>>
>> +/* These structures are already defined by OpenBIOS and usable with SLOF */
>> +#define NVRAM_PART_SYSTEM 0x70
>> +struct nvpart {
>> + uint8_t signature;
>> + uint8_t checksum;
>> + uint16_t len; /* BE, length divided by 16 */
>> + char name[12];
>> + char content[0];
>
> IIRC zero-sized arrays are a GCC extension ... for valid C99, it might
> be better to use "char content[]" instead?
You're right: I though [0] was more generic than [].
>
>> +};
>>
>> +#define MIN_NVRAM_SIZE 8192 /* from spapr_nvram.c */
>> +
>> +/* FW_MAX_SIZE is 4MB, but slof.bin is only 900KB,
>> + * so let's modify memory between 1MB and 100MB
>> + * to do like PC bootsector
>> + */
>> +#define FORTH_BOOTSCRIPT "boot-command=hex .\" _\" begin %x %x do i c@ 1 + i c! 1000 +loop .\" B\" 0 until"
>
> Just a matter of taste, but I somewhat dislike the idea of hiding a
> string with format parameters in a macro ... I think I'd rather place
> this string directly into the corresponding sprintf() statement below
> instead.
Right
>
>> const unsigned start_address = 1024 * 1024;
>> const unsigned end_address = 100 * 1024 * 1024;
>> bool got_stop;
>> @@ -122,6 +140,52 @@ unsigned char bootsect[] = {
>> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x55, 0xaa
>> };
>>
>> +static void init_bootfile_x86(const char *bootpath)
>> +{
>> + FILE *bootfile = fopen(bootpath, "wb");
>> +
>> + g_assert_cmpint(fwrite(bootsect, 512, 1, bootfile), ==, 1);
>> + fclose(bootfile);
>> +}
>> +
>> +static void nvpart_checksum(struct nvpart *header)
>> +{
>> + unsigned int i, sum;
>> + uint8_t *tmpptr;
>> +
>> + tmpptr = (uint8_t *)header;
>> + sum = *tmpptr;
>> + for (i = 0; i < 14; i++) {
>> + sum += tmpptr[2 + i];
>> + sum = (sum + ((sum & 0xff00) >> 8)) & 0xff;
>> + }
>> + header->checksum = sum & 0xff;
>> +}
>
> Have you tried to include openbios_firmware_abi.h instead and use
> OpenBIOS_finish_partition() here? That would avoid to have this code
> duplicated.
Right again
>> +static void init_bootfile_ppc(const char *bootpath)
>> +{
>> + FILE *bootfile;
>> + char buf[MIN_NVRAM_SIZE];
>> + struct nvpart *header = (struct nvpart *)buf;
>> +
>> + memset(buf, 0, MIN_NVRAM_SIZE);
>> +
>> + /* Create a "common" partition in nvram to store boot-command property */
>> +
>> + header->signature = NVRAM_PART_SYSTEM;
>> + memcpy(header->name, "common", 6);
>> + header->len = cpu_to_be16(MIN_NVRAM_SIZE >> 4);
>> + nvpart_checksum(header); /* can change if we change header->len */
>> +
>> + sprintf(header->content, FORTH_BOOTSCRIPT, end_address, start_address);
>> +
>> + /* Write partition to the NVRAM file */
>> +
>> + bootfile = fopen(bootpath, "wb");
>> + g_assert_cmpint(fwrite(buf, MIN_NVRAM_SIZE, 1, bootfile), ==, 1);
>> + fclose(bootfile);
>> +}
>> +
>> /*
>> * Wait for some output in the serial output file,
>> * we get an 'A' followed by an endless string of 'B's
>> @@ -131,10 +195,31 @@ static void wait_for_serial(const char *side)
>> {
>> char *serialpath = g_strdup_printf("%s/%s", tmpfs, side);
>> FILE *serialfile = fopen(serialpath, "r");
>> + const char *arch = qtest_get_arch();
>> + int started = (strcmp(side, "src_serial") == 0 &&
>> + strcmp(arch, "ppc64") == 0) ? 0 : 1;
>>
>> do {
>> int readvalue = fgetc(serialfile);
>>
>> + if (!started) {
>> + /* SLOF prints its banner before starting test,
>> + * to ignore it, mark the start of the test with '_',
>> + * ignore all characters until this marker
>> + */
>> + switch (readvalue) {
>> + case '_':
>> + started = 1;
>> + break;
>> + case EOF:
>> + fseek(serialfile, 0, SEEK_SET);
>> + usleep(1000);
>> + break;
>> + default:
>> + break;
>
> I think you could remove that default case.
ok
>
>> + }
>> + continue;
>> + }
>> switch (readvalue) {
>> case 'A':
>> /* Fine */
>> @@ -147,6 +232,8 @@ static void wait_for_serial(const char *side)
>> return;
>>
>> case EOF:
>> + started = (strcmp(side, "src_serial") == 0 &&
>> + strcmp(arch, "ppc64") == 0) ? 0 : 1;
>
> I somehow fail to see why is this needed here again? Isn't the initial
> setup of "started" at the beginning of the function enough?
No, because we read a file and this file is growing while we are reading
it, so if we restart to read the file (fseek() below) because we are at
the end, we must skip again the banner.
>
>> fseek(serialfile, 0, SEEK_SET);
>> usleep(1000);
>> break;
>> @@ -295,32 +382,48 @@ static void test_migrate(void)
>> char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
>> QTestState *global = global_qtest, *from, *to;
>> unsigned char dest_byte_a, dest_byte_b, dest_byte_c, dest_byte_d;
>> - gchar *cmd;
>> + gchar *cmd, *cmd_src, *cmd_dst;
>> QDict *rsp;
>>
>> char *bootpath = g_strdup_printf("%s/bootsect", tmpfs);
>> - FILE *bootfile = fopen(bootpath, "wb");
>> + const char *arch = qtest_get_arch();
>>
>> got_stop = false;
>> - g_assert_cmpint(fwrite(bootsect, 512, 1, bootfile), ==, 1);
>> - fclose(bootfile);
>>
>> - cmd = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
>> - " -name pcsource,debug-threads=on"
>> - " -serial file:%s/src_serial"
>> - " -drive file=%s,format=raw",
>> - tmpfs, bootpath);
>> - from = qtest_start(cmd);
>> - g_free(cmd);
>> + if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>> + init_bootfile_x86(bootpath);
>> + cmd_src = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
>> + " -name pcsource,debug-threads=on"
>> + " -serial file:%s/src_serial"
>> + " -drive file=%s,format=raw",
>> + tmpfs, bootpath);
>> + cmd_dst = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
>> + " -name pcdest,debug-threads=on"
>> + " -serial file:%s/dest_serial"
>> + " -drive file=%s,format=raw"
>> + " -incoming %s",
>> + tmpfs, bootpath, uri);
>> + } else if (strcmp(arch, "ppc64") == 0) {
>> + init_bootfile_ppc(bootpath);
>> + cmd_src = g_strdup_printf("-machine accel=kvm:tcg -m 256M"
>> + " -name pcsource,debug-threads=on"
>> + " -serial file:%s/src_serial"
>> + " -drive file=%s,if=pflash,format=raw",
>> + tmpfs, bootpath);
>> + cmd_dst = g_strdup_printf("-machine accel=kvm:tcg -m 256M"
>> + " -name pcdest,debug-threads=on"
>> + " -serial file:%s/dest_serial"
>> + " -incoming %s",
>> + tmpfs, uri);
>> + } else {
>> + g_assert_not_reached();
>> + }
>>
>> - cmd = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
>> - " -name pcdest,debug-threads=on"
>> - " -serial file:%s/dest_serial"
>> - " -drive file=%s,format=raw"
>> - " -incoming %s",
>> - tmpfs, bootpath, uri);
>> - to = qtest_init(cmd);
>> - g_free(cmd);
>> + from = qtest_start(cmd_src);
>> + g_free(cmd_src);
>> +
>> + to = qtest_init(cmd_dst);
>> + g_free(cmd_dst);
>>
>> global_qtest = from;
>> rsp = qmp("{ 'execute': 'migrate-set-capabilities',"
>>
>
> Thomas
I'm going to send a v2...
Thanks,
Laurent
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] test: port postcopy test to ppc64
2016-07-21 13:46 ` Laurent Vivier
@ 2016-07-21 21:02 ` Thomas Huth
0 siblings, 0 replies; 5+ messages in thread
From: Thomas Huth @ 2016-07-21 21:02 UTC (permalink / raw)
To: Laurent Vivier, qemu-devel; +Cc: dgilbert, dgibson
On 21.07.2016 15:46, Laurent Vivier wrote:
>
>
> On 21/07/2016 15:16, Thomas Huth wrote:
>> On 21.07.2016 12:12, Laurent Vivier wrote:
>>> As userfaultfd syscall is available on powerpc, migration
>>> postcopy can be used.
>>
>> Good idea to add this test for PPC, too!
>
> The idea comes from David Gilbert :)
> Thanks David!
>
>>
>>> This patch adds the support needed to test this on powerpc,
>>> instead of using a bootsector to run code to modify memory,
>>> we use a FORTH script in "boot-command" property.
>>>
>>> As spapr machine doesn't support "-prom-env" argument
>>> (the nvram is initialized by SLOF and not by QEMU),
>>> "boot-command" is provided to SLOF via a file mapped nvram
>>> (with "-drive file=...,if=pflash")
>>
>> I wonder whether we could easily add support for the "-prom-env"
>> parameter for the sPAPR machine, too, since the NVRAM layout seems to be
>> pretty much the same as on the old CHRP Mac machines...?
>
> The only thing we need to have "-prom-env" is to initialize the nvram
> partitions in QEMU if they don't exist.
>
> I didn't follow this way because this work is normally to be done by the
> firmware not by QEMU. If we do it in QEMU, SLOF will do not, and we
> can't be sure we are doing the things well, as SLOF does.
I'm slightly familiar with the NVRAM code in SLOF ... shouldn't be too
hard to get a similar layout done with QEMU, especially if we can re-use
some of the OpenBIOS NVRAM functions in QEMU.
> On Mac, it is another history, originally, firmware was OpenHackware,
> not OpenBIOS, and OH was only here to allow to boot the kernel nothing
> more (it was really a hack: there was not forth interpreter [oh, my god!]).
> So all the important stuffs were done by QEMU, like to initialize the
> NVRAM, the PCI bus, ... and this allows to set values in the environment
> variables.
>
> But if you think it is viable, it should be really easy to do (like
> cut'n'paste from OpenBIOS ABI)...
Having -prom-env for SLOF, too, would be really posh, so I think I'll
have a try when I got some spare minutes ... hmm, my TODO list seems to
keep rather growing than shrinking ... maybe I'm doing something wrong ;-)
Thomas
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-07-21 21:02 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-21 10:12 [Qemu-devel] [PATCH] test: port postcopy test to ppc64 Laurent Vivier
2016-07-21 13:16 ` Thomas Huth
2016-07-21 13:46 ` Laurent Vivier
2016-07-21 21:02 ` Thomas Huth
2016-07-21 16:13 ` Laurent Vivier
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.