[kvm-unit-tests PATCH] powerpc: Add tests for sPAPR h-calls

[kvm-unit-tests PATCH] powerpc: Add tests for sPAPR h-calls

[Thomas Huth]

Introduce a test for sPAPR hypercalls, starting with the two
hypercalls H_SET_SPRG0 and H_PAGE_INIT.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 The patch has to be applied on top of Andrew Jones' ppc64 patch
 series. You also need the latest QEMU development version since
 the hypercalls have only been added recently.

 lib/powerpc/asm/hcall.h |   3 +
 powerpc/Makefile.common |   5 +-
 powerpc/spapr_hcall.c   | 144 ++++++++++++++++++++++++++++++++++++++++++++++++
 powerpc/unittests.cfg   |   3 +
 4 files changed, 154 insertions(+), 1 deletion(-)
 create mode 100644 powerpc/spapr_hcall.c

diff --git a/lib/powerpc/asm/hcall.h b/lib/powerpc/asm/hcall.h
index 75eac45..3fe8227 100644
--- a/lib/powerpc/asm/hcall.h
+++ b/lib/powerpc/asm/hcall.h
@@ -11,8 +11,11 @@
 
 #define H_SUCCESS		0
 #define H_HARDWARE		-1
+#define H_PARAMETER		-4
 
+#define H_SET_SPRG0		0x24
 #define H_SET_DABR		0x28
+#define H_PAGE_INIT		0x2c
 #define H_PUT_TERM_CHAR		0x58
 
 #ifndef __ASSEMBLY__
diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
index 3a03ab6..6c0f47c 100644
--- a/powerpc/Makefile.common
+++ b/powerpc/Makefile.common
@@ -9,7 +9,8 @@ ifeq ($(LOADADDR),)
 endif
 
 tests-common = \
-	$(TEST_DIR)/selftest.elf
+	$(TEST_DIR)/selftest.elf \
+	$(TEST_DIR)/spapr_hcall.elf
 
 all: $(TEST_DIR)/boot_rom.bin test_cases
 
@@ -69,3 +70,5 @@ generated_files = $(asm-offsets)
 test_cases: $(generated_files) $(tests-common) $(tests)
 
 $(TEST_DIR)/selftest.elf: $(cstart.o) $(reloc.o) $(TEST_DIR)/selftest.o
+
+$(TEST_DIR)/spapr_hcall.elf: $(cstart.o) $(reloc.o) $(TEST_DIR)/spapr_hcall.o
diff --git a/powerpc/spapr_hcall.c b/powerpc/spapr_hcall.c
new file mode 100644
index 0000000..886217d
--- /dev/null
+++ b/powerpc/spapr_hcall.c
@@ -0,0 +1,144 @@
+/*
+ * Test sPAPR hypervisor calls (aka. h-calls)
+ *
+ * Copyright 2016  Thomas Huth, Red Hat Inc.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+#include <libcflat.h>
+#include <util.h>
+#include <alloc.h>
+#include <asm/hcall.h>
+
+#define PAGE_SIZE 4096
+
+#define H_ZERO_PAGE	(1 << (63-48))
+#define H_COPY_PAGE	(1 << (63-49))
+
+#define mfspr(nr) ({ \
+	u64 ret; \
+	asm volatile("mfspr %0,%1" : "=r"(ret) : "i"(nr)); \
+	ret; \
+})
+
+#define SPR_SPRG0	0x110
+
+static u64 h_set_sprg0(u64 new_sprg0)
+{
+	unsigned long in[8], out[8];
+
+	in[0] = new_sprg0;
+
+	return hcall(H_SET_SPRG0, in, out);
+}
+
+/**
+ * Test the H_SET_SPRG0 h-call by setting some values and
+ * checking whether the SPRG0 register contains the correct
+ * values afterwards
+ */
+static void test_h_set_sprg0(int argc, char **argv)
+{
+	u64 sprg0, sprg0_orig;
+	int rc;
+
+	if (argc > 1)
+		report_abort("Unsupported argument: '%s'", argv[1]);
+
+	sprg0_orig = mfspr(SPR_SPRG0);
+
+	rc = h_set_sprg0(0xcafebabedeadbeefULL);
+	sprg0 = mfspr(SPR_SPRG0);
+	report("sprg0 = 0xcafebabedeadbeef",
+		rc = H_SUCCESS && sprg0 = 0xcafebabedeadbeefULL);
+
+	rc = h_set_sprg0(0xaaaaaaaa55555555ULL);
+	sprg0 = mfspr(SPR_SPRG0);
+	report("sprg0 = 0xaaaaaaaa55555555",
+		rc = H_SUCCESS && sprg0 = 0xaaaaaaaa55555555ULL);
+
+	rc = h_set_sprg0(sprg0_orig);
+	sprg0 = mfspr(SPR_SPRG0);
+	report("sprg0 = 0x%llx",
+		rc = H_SUCCESS && sprg0 = sprg0_orig, sprg0_orig);
+}
+
+static u64 h_page_init(u64 flags, void *destination, void *source)
+{
+	unsigned long in[8], out[8];
+
+	in[0] = flags;
+	in[1] = (unsigned long)destination;
+	in[2] = (unsigned long)source;
+
+	return hcall(H_PAGE_INIT, in, out);
+}
+
+/**
+ * Test the H_PAGE_INIT h-call by using it to clear and to
+ * copy a page, and by checking for the correct values in
+ * the destination page afterwards
+ */
+static void test_h_page_init(int argc, char **argv)
+{
+	u8 *dst, *src;
+	int rc;
+
+	if (argc > 1)
+		report_abort("Unsupported argument: '%s'", argv[1]);
+
+	dst = memalign(PAGE_SIZE, PAGE_SIZE);
+	src = memalign(PAGE_SIZE, PAGE_SIZE);
+	if (!dst || !src)
+		report_abort("Failed to alloc memory");
+
+	memset(dst, 0xaa, PAGE_SIZE);
+	rc = h_page_init(H_ZERO_PAGE, dst, src);
+	report("h_zero_page", rc = H_SUCCESS && *(u64*)dst = 0);
+
+	*(u64*)src = 0xbeefc0dedeadcafeULL;
+	rc = h_page_init(H_COPY_PAGE, dst, src);
+	report("h_copy_page",
+		rc = H_SUCCESS && *(u64*)dst = 0xbeefc0dedeadcafeULL);
+
+	*(u64*)src = 0x9abcdef012345678ULL;
+	rc = h_page_init(H_COPY_PAGE|H_ZERO_PAGE, dst, src);
+	report("h_copy_page+h_zero_page",
+		rc = H_SUCCESS &&  *(u64*)dst = 0x9abcdef012345678ULL);
+
+	rc = h_page_init(H_ZERO_PAGE, dst + 0x123, src);
+	report("h_zero_page unaligned dst", rc = H_PARAMETER);
+
+	rc = h_page_init(H_COPY_PAGE, dst, src + 0x123);
+	report("h_copy_page unaligned src", rc = H_PARAMETER);
+}
+
+struct {
+	const char *name;
+	void (*func)(int argc, char **argv);
+} hctests[] = {
+	{ "h_set_sprg0", test_h_set_sprg0 },
+	{ "h_page_init", test_h_page_init },
+	{ NULL, NULL }
+};
+
+int main(int argc, char **argv)
+{
+	int all = 0;
+	int i;
+
+	report_prefix_push("hypercall");
+
+	if (!argc || (argc = 1 && !strcmp(argv[0], "all")))
+		all = 1;
+
+	for (i = 0; hctests[i].name != NULL; i++) {
+		report_prefix_push(hctests[i].name);
+		if (all || strcmp(argv[0], hctests[i].name) = 0) {
+			hctests[i].func(argc, argv);
+		}
+		report_prefix_pop();
+	}
+
+	return report_summary();
+}
diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
index 60f9be8..d858436 100644
--- a/powerpc/unittests.cfg
+++ b/powerpc/unittests.cfg
@@ -28,3 +28,6 @@ file = selftest.elf
 smp = 2
 extra_params = -m 256 -append 'setup smp=2 mem%6'
 groups = selftest
+
+[spapr_hcall]
+file = spapr_hcall.elf
-- 
1.8.3.1

Re: [kvm-unit-tests PATCH] powerpc: Add tests for sPAPR h-calls

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6461 bytes --]

On Thu, 25 Feb 2016 22:19:13 +0100
Thomas Huth <thuth@redhat.com> wrote:

> Introduce a test for sPAPR hypercalls, starting with the two
> hypercalls H_SET_SPRG0 and H_PAGE_INIT.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  The patch has to be applied on top of Andrew Jones' ppc64 patch
>  series. You also need the latest QEMU development version since
>  the hypercalls have only been added recently.
> 
>  lib/powerpc/asm/hcall.h |   3 +
>  powerpc/Makefile.common |   5 +-
>  powerpc/spapr_hcall.c   | 144 ++++++++++++++++++++++++++++++++++++++++++++++++
>  powerpc/unittests.cfg   |   3 +
>  4 files changed, 154 insertions(+), 1 deletion(-)
>  create mode 100644 powerpc/spapr_hcall.c
> 
> diff --git a/lib/powerpc/asm/hcall.h b/lib/powerpc/asm/hcall.h
> index 75eac45..3fe8227 100644
> --- a/lib/powerpc/asm/hcall.h
> +++ b/lib/powerpc/asm/hcall.h
> @@ -11,8 +11,11 @@
>  
>  #define H_SUCCESS		0
>  #define H_HARDWARE		-1
> +#define H_PARAMETER		-4
>  
> +#define H_SET_SPRG0		0x24
>  #define H_SET_DABR		0x28
> +#define H_PAGE_INIT		0x2c
>  #define H_PUT_TERM_CHAR		0x58
>  
>  #ifndef __ASSEMBLY__
> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> index 3a03ab6..6c0f47c 100644
> --- a/powerpc/Makefile.common
> +++ b/powerpc/Makefile.common
> @@ -9,7 +9,8 @@ ifeq ($(LOADADDR),)
>  endif
>  
>  tests-common = \
> -	$(TEST_DIR)/selftest.elf
> +	$(TEST_DIR)/selftest.elf \
> +	$(TEST_DIR)/spapr_hcall.elf
>  
>  all: $(TEST_DIR)/boot_rom.bin test_cases
>  
> @@ -69,3 +70,5 @@ generated_files = $(asm-offsets)
>  test_cases: $(generated_files) $(tests-common) $(tests)
>  
>  $(TEST_DIR)/selftest.elf: $(cstart.o) $(reloc.o) $(TEST_DIR)/selftest.o
> +
> +$(TEST_DIR)/spapr_hcall.elf: $(cstart.o) $(reloc.o) $(TEST_DIR)/spapr_hcall.o
> diff --git a/powerpc/spapr_hcall.c b/powerpc/spapr_hcall.c
> new file mode 100644
> index 0000000..886217d
> --- /dev/null
> +++ b/powerpc/spapr_hcall.c
> @@ -0,0 +1,144 @@
> +/*
> + * Test sPAPR hypervisor calls (aka. h-calls)
> + *
> + * Copyright 2016  Thomas Huth, Red Hat Inc.
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#include <libcflat.h>
> +#include <util.h>
> +#include <alloc.h>
> +#include <asm/hcall.h>
> +
> +#define PAGE_SIZE 4096
> +
> +#define H_ZERO_PAGE	(1 << (63-48))
> +#define H_COPY_PAGE	(1 << (63-49))
> +
> +#define mfspr(nr) ({ \
> +	u64 ret; \
> +	asm volatile("mfspr %0,%1" : "=r"(ret) : "i"(nr)); \
> +	ret; \
> +})
> +
> +#define SPR_SPRG0	0x110
> +
> +static u64 h_set_sprg0(u64 new_sprg0)
> +{
> +	unsigned long in[8], out[8];
> +
> +	in[0] = new_sprg0;
> +
> +	return hcall(H_SET_SPRG0, in, out);
> +}
> +
> +/**
> + * Test the H_SET_SPRG0 h-call by setting some values and
> + * checking whether the SPRG0 register contains the correct
> + * values afterwards
> + */
> +static void test_h_set_sprg0(int argc, char **argv)
> +{
> +	u64 sprg0, sprg0_orig;
> +	int rc;
> +
> +	if (argc > 1)
> +		report_abort("Unsupported argument: '%s'", argv[1]);
> +
> +	sprg0_orig = mfspr(SPR_SPRG0);
> +
> +	rc = h_set_sprg0(0xcafebabedeadbeefULL);
> +	sprg0 = mfspr(SPR_SPRG0);
> +	report("sprg0 = 0xcafebabedeadbeef",
> +		rc == H_SUCCESS && sprg0 == 0xcafebabedeadbeefULL);
> +
> +	rc = h_set_sprg0(0xaaaaaaaa55555555ULL);
> +	sprg0 = mfspr(SPR_SPRG0);
> +	report("sprg0 = 0xaaaaaaaa55555555",
> +		rc == H_SUCCESS && sprg0 == 0xaaaaaaaa55555555ULL);
> +
> +	rc = h_set_sprg0(sprg0_orig);
> +	sprg0 = mfspr(SPR_SPRG0);
> +	report("sprg0 = 0x%llx",
> +		rc == H_SUCCESS && sprg0 == sprg0_orig, sprg0_orig);
> +}
> +
> +static u64 h_page_init(u64 flags, void *destination, void *source)
> +{
> +	unsigned long in[8], out[8];
> +
> +	in[0] = flags;
> +	in[1] = (unsigned long)destination;
> +	in[2] = (unsigned long)source;
> +
> +	return hcall(H_PAGE_INIT, in, out);
> +}
> +
> +/**
> + * Test the H_PAGE_INIT h-call by using it to clear and to
> + * copy a page, and by checking for the correct values in
> + * the destination page afterwards
> + */
> +static void test_h_page_init(int argc, char **argv)
> +{
> +	u8 *dst, *src;
> +	int rc;
> +
> +	if (argc > 1)
> +		report_abort("Unsupported argument: '%s'", argv[1]);
> +
> +	dst = memalign(PAGE_SIZE, PAGE_SIZE);
> +	src = memalign(PAGE_SIZE, PAGE_SIZE);
> +	if (!dst || !src)
> +		report_abort("Failed to alloc memory");
> +
> +	memset(dst, 0xaa, PAGE_SIZE);
> +	rc = h_page_init(H_ZERO_PAGE, dst, src);
> +	report("h_zero_page", rc == H_SUCCESS && *(u64*)dst == 0);
> +
> +	*(u64*)src = 0xbeefc0dedeadcafeULL;
> +	rc = h_page_init(H_COPY_PAGE, dst, src);
> +	report("h_copy_page",
> +		rc == H_SUCCESS && *(u64*)dst == 0xbeefc0dedeadcafeULL);
> +
> +	*(u64*)src = 0x9abcdef012345678ULL;
> +	rc = h_page_init(H_COPY_PAGE|H_ZERO_PAGE, dst, src);
> +	report("h_copy_page+h_zero_page",
> +		rc == H_SUCCESS &&  *(u64*)dst == 0x9abcdef012345678ULL);
> +
> +	rc = h_page_init(H_ZERO_PAGE, dst + 0x123, src);
> +	report("h_zero_page unaligned dst", rc == H_PARAMETER);
> +
> +	rc = h_page_init(H_COPY_PAGE, dst, src + 0x123);
> +	report("h_copy_page unaligned src", rc == H_PARAMETER);
> +}
> +
> +struct {
> +	const char *name;
> +	void (*func)(int argc, char **argv);
> +} hctests[] = {
> +	{ "h_set_sprg0", test_h_set_sprg0 },
> +	{ "h_page_init", test_h_page_init },
> +	{ NULL, NULL }
> +};
> +
> +int main(int argc, char **argv)
> +{
> +	int all = 0;
> +	int i;
> +
> +	report_prefix_push("hypercall");
> +
> +	if (!argc || (argc == 1 && !strcmp(argv[0], "all")))
> +		all = 1;
> +
> +	for (i = 0; hctests[i].name != NULL; i++) {
> +		report_prefix_push(hctests[i].name);
> +		if (all || strcmp(argv[0], hctests[i].name) == 0) {
> +			hctests[i].func(argc, argv);
> +		}
> +		report_prefix_pop();
> +	}
> +
> +	return report_summary();
> +}
> diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
> index 60f9be8..d858436 100644
> --- a/powerpc/unittests.cfg
> +++ b/powerpc/unittests.cfg
> @@ -28,3 +28,6 @@ file = selftest.elf
>  smp = 2
>  extra_params = -m 256 -append 'setup smp=2 mem=256'
>  groups = selftest
> +
> +[spapr_hcall]
> +file = spapr_hcall.elf
> -- 
> 1.8.3.1
> 


-- 
David Gibson <dgibson@redhat.com>
Senior Software Engineer, Virtualization, Red Hat

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[kvm-unit-tests PATCH] powerpc: Add tests for RTAS

[Laurent Vivier]

By starting with get-time-of-day, set-time-of-day.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 powerpc/Makefile.common |   5 +-
 powerpc/rtas.c          | 148 ++++++++++++++++++++++++++++++++++++++++++++++++
 powerpc/unittests.cfg   |  12 ++++
 3 files changed, 164 insertions(+), 1 deletion(-)
 create mode 100644 powerpc/rtas.c

diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
index 2ce6494..424983e 100644
--- a/powerpc/Makefile.common
+++ b/powerpc/Makefile.common
@@ -6,7 +6,8 @@
 
 tests-common = \
 	$(TEST_DIR)/selftest.elf \
-	$(TEST_DIR)/spapr_hcall.elf
+	$(TEST_DIR)/spapr_hcall.elf \
+	$(TEST_DIR)/rtas.elf
 
 all: $(TEST_DIR)/boot_rom.bin test_cases
 
@@ -66,3 +67,5 @@ test_cases: $(generated_files) $(tests-common) $(tests)
 $(TEST_DIR)/selftest.elf: $(cstart.o) $(reloc.o) $(TEST_DIR)/selftest.o
 
 $(TEST_DIR)/spapr_hcall.elf: $(cstart.o) $(reloc.o) $(TEST_DIR)/spapr_hcall.o
+
+$(TEST_DIR)/rtas.elf: $(cstart.o) $(reloc.o) $(TEST_DIR)/rtas.o
diff --git a/powerpc/rtas.c b/powerpc/rtas.c
new file mode 100644
index 0000000..9d673f0
--- /dev/null
+++ b/powerpc/rtas.c
@@ -0,0 +1,148 @@
+/*
+ * Test the RTAS interface
+ */
+
+#include <libcflat.h>
+#include <util.h>
+#include <asm/rtas.h>
+
+#define DAYS(y,m,d) (365UL * (y) + ((y) / 4) - ((y) / 100) + ((y) / 400) + \
+		     367UL * (m) / 12  + \
+		     (d))
+
+static unsigned long mktime(int year, int month, int day,
+			    int hour, int minute, int second)
+{
+	unsigned long epoch;
+
+	/* Put February at end of the year to avoid leap day this year */
+
+	month -= 2;
+	if (month <= 0) {
+		month += 12;
+		year -= 1;
+	}
+
+	/* compute epoch: substract DAYS(since_March(1-1-1970)) */
+
+	epoch = DAYS(year, month, day) - DAYS(1969, 11, 1);
+
+	epoch = epoch * 24 + hour;
+	epoch = epoch * 60 + minute;
+	epoch = epoch * 60 + second;
+
+	return epoch;
+}
+
+#define DELAY 1
+#define MAX_LOOP 10000000
+
+static void check_get_time_of_day(unsigned long start)
+{
+	int token;
+	int ret;
+	int now[8];
+	unsigned long t1, t2, count;
+
+	token = rtas_token("get-time-of-day");
+	report("token available", token != RTAS_UNKNOWN_SERVICE);
+	if (token = RTAS_UNKNOWN_SERVICE)
+		return;
+
+	ret = rtas_call(token, 0, 8, now);
+	report("execution", ret = 0);
+
+	report("second",  now[5] >= 0 && now[5] <= 59);
+	report("minute", now[4] >= 0 && now[4] <= 59);
+	report("hour", now[3] >= 0 && now[3] <= 23);
+	report("day", now[2] >= 1 && now[2] <= 31);
+	report("month", now[1] >= 1 && now[1] <= 12);
+	report("year", now[0] >= 1970);
+	report("accuracy (< 3s)", mktime(now[0], now[1], now[2],
+					 now[3], now[4], now[5]) - start < 3);
+
+	ret = rtas_call(token, 0, 8, now);
+	t1 = mktime(now[0], now[1], now[2], now[3], now[4], now[5]);
+	count = 0;
+	do {
+		ret = rtas_call(token, 0, 8, now);
+		t2 = mktime(now[0], now[1], now[2], now[3], now[4], now[5]);
+		count++;
+	} while (t1 + DELAY > t2 && count < MAX_LOOP);
+	report("running", t1 + DELAY <= t2);
+}
+
+static void check_set_time_of_day(void)
+{
+	int token;
+	int ret;
+	int date[8];
+	unsigned long t1, t2, count;
+
+	token = rtas_token("set-time-of-day");
+	report("token available", token != RTAS_UNKNOWN_SERVICE);
+	if (token = RTAS_UNKNOWN_SERVICE)
+		return;
+
+	/* 23:59:59 28/2/2000 */
+
+	ret = rtas_call(token, 7, 1, NULL, 2000, 2, 28, 23, 59, 59);
+	report("execution", ret = 0);
+
+	/* check it has worked */
+	ret = rtas_call(rtas_token("get-time-of-day"), 0, 8, date);
+	report("re-read", ret = 0);
+	t1 = mktime(2000, 2, 28, 23, 59, 59);
+	t2 = mktime(date[0], date[1], date[2],
+		    date[3], date[4], date[5]);
+	report("result", t2 - t1 < 2);
+
+	/* check it is running */
+	count = 0;
+	do {
+		ret = rtas_call(rtas_token("get-time-of-day"), 0, 8, date);
+		t2 = mktime(date[0], date[1], date[2],
+			    date[3], date[4], date[5]);
+		count++;
+	} while (t1 + DELAY > t2 && count < MAX_LOOP);
+	report("running", t1 + DELAY <= t2);
+}
+
+int main(int argc, char **argv)
+{
+	int len;
+	long val;
+
+	report_prefix_push("rtas");
+
+	if (!argc)
+		report_abort("no test specified");
+
+	report_prefix_push(argv[0]);
+
+	if (strcmp(argv[0], "get-time-of-day") = 0) {
+
+		len = parse_keyval(argv[1], &val);
+		if (len = -1) {
+			printf("Missing parameter \"date\"\n");
+			abort();
+		}
+		argv[1][len] = '\0';
+
+		check_get_time_of_day(val);
+
+	} else if (strcmp(argv[0], "set-time-of-day") = 0) {
+
+		check_set_time_of_day();
+
+	} else {
+		printf("Unknown subtest\n");
+		abort();
+	}
+
+	report_prefix_pop();
+
+	report_prefix_pop();
+
+	return report_summary();
+}
diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
index d858436..0666922 100644
--- a/powerpc/unittests.cfg
+++ b/powerpc/unittests.cfg
@@ -31,3 +31,15 @@ groups = selftest
 
 [spapr_hcall]
 file = spapr_hcall.elf
+
+[rtas-get-time-of-day]
+file = rtas.elf
+timeout = 5
+extra_params = -append "get-time-of-day date=$(date +%s)"
+groups = rtas
+
+[rtas-set-time-of-day]
+file = rtas.elf
+extra_params = -append "set-time-of-day"
+timeout = 5
+groups = rtas
-- 
2.5.0

Re: [kvm-unit-tests PATCH] powerpc: Add tests for RTAS

[Paolo Bonzini]

On 03/03/2016 13:28, Laurent Vivier wrote:
> +
> +#define DAYS(y,m,d) (365UL * (y) + ((y) / 4) - ((y) / 100) + ((y) / 400) + \
> +		     367UL * (m) / 12  + \

Out of curiosity, where did you take this from?  I knew the variant with
2447/80 instead of 367/12, but they give the same result, as checked
with bc:

$ bc
for (n=1;n<\x12;n++)2447*n/80-367*n/12;
0
0
0
0
0
0
0
0
0
0
0
0

Paolo

> +		     (d))
> +

Re: [kvm-unit-tests PATCH] powerpc: Add tests for RTAS

[Laurent Vivier]

On 03/03/2016 13:45, Paolo Bonzini wrote:
> 
> 
> On 03/03/2016 13:28, Laurent Vivier wrote:
>> +
>> +#define DAYS(y,m,d) (365UL * (y) + ((y) / 4) - ((y) / 100) + ((y) / 400) + \
>> +		     367UL * (m) / 12  + \
> 
> Out of curiosity, where did you take this from?  I knew the variant with
> 2447/80 instead of 367/12, but they give the same result, as checked
> with bc:

Some ideas from:
http://www.cantab.net/users/michael.behrend/algorithms/easter/pages/main.html

30*m + (7*(m+1) / 12)

reduced into

(360m + 7m + 7)/12 -> 367m/12

Laurent

> $ bc
> for (n=1;n<\x12;n++)2447*n/80-367*n/12;
> 0
> 0
> 0
> 0
> 0
> 0
> 0
> 0
> 0
> 0
> 0
> 0
> 
> Paolo
> 
>> +		     (d))
>> +

Re: [kvm-unit-tests PATCH] powerpc: Add tests for RTAS

[Radim Krčmář]

2016-03-03 13:28+0100, Laurent Vivier:
> By starting with get-time-of-day, set-time-of-day.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
> diff --git a/powerpc/rtas.c b/powerpc/rtas.c
> +#define DAYS(y,m,d) (365UL * (y) + ((y) / 4) - ((y) / 100) + ((y) / 400) + \
> +		     367UL * (m) / 12  + \
> +		     (d))

This function is hard to (re)use.
What about putting the "month -= 2" block together with DAYS to give a
better estimate of the amount of days in the gregorian calendar?

  static inline unsigned long days(int year, int month, int day) {
  	month -= 2;
  	if (month <= 0) {
  		month += 12;
  		year -= 1;
  	}
  	return DAYS(year, month, day);
  }

(Or replacing it with an obvious, but slower/bigger implementation? :])

> +	/* Put February at end of the year to avoid leap day this year */
> +
> +	month -= 2;
> +	if (month <= 0) {
> +		month += 12;
> +		year -= 1;
> +	}
> +
> +	/* compute epoch: substract DAYS(since_March(1-1-1970)) */
> +
> +	epoch = DAYS(year, month, day) - DAYS(1969, 11, 1);

You'd then be able to write

  epoch = days(year, month, day) - days(1970, 1, 1);

instead of the obscure chunk of code.

Re: [kvm-unit-tests PATCH] powerpc: Add tests for RTAS

[Paolo Bonzini]

On 03/03/2016 17:03, Radim Krčmář wrote:
>> > diff --git a/powerpc/rtas.c b/powerpc/rtas.c
>> > +#define DAYS(y,m,d) (365UL * (y) + ((y) / 4) - ((y) / 100) + ((y) / 400) + \
>> > +		     367UL * (m) / 12  + \
>> > +		     (d))
> This function is hard to (re)use.
> What about putting the "month -= 2" block together with DAYS to give a
> better estimate of the amount of days in the gregorian calendar?

Even the Gregorian calendar only starts in 1583 though. :)

This is just a utility function for mktime.  I think it's okay.  We
should aim at making libcflat a minimal libc, and in that case we would
move mktime to lib/.  Putting stuff directly in tests is good enough
(worse is better), but let's remember that duplicated code is not.

Paolo

>   static inline unsigned long days(int year, int month, int day) {
>   	month -= 2;
>   	if (month <= 0) {
>   		month += 12;
>   		year -= 1;
>   	}
>   	return DAYS(year, month, day);
>   }
> 
> (Or replacing it with an obvious, but slower/bigger implementation? :])
> 
>> > +	/* Put February at end of the year to avoid leap day this year */
>> > +
>> > +	month -= 2;
>> > +	if (month <= 0) {
>> > +		month += 12;
>> > +		year -= 1;
>> > +	}
>> > +
>> > +	/* compute epoch: substract DAYS(since_March(1-1-1970)) */
>> > +
>> > +	epoch = DAYS(year, month, day) - DAYS(1969, 11, 1);

Re: [kvm-unit-tests PATCH] powerpc: Add tests for RTAS

[Radim Krčmář]

2016-03-03 17:29+0100, Paolo Bonzini:
> On 03/03/2016 17:03, Radim Krčmář wrote:
>>> > diff --git a/powerpc/rtas.c b/powerpc/rtas.c
>>> > +#define DAYS(y,m,d) (365UL * (y) + ((y) / 4) - ((y) / 100) + ((y) / 400) + \
>>> > +		     367UL * (m) / 12  + \
>>> > +		     (d))
>> This function is hard to (re)use.
>> What about putting the "month -= 2" block together with DAYS to give a
>> better estimate of the amount of days in the gregorian calendar?
> 
> Even the Gregorian calendar only starts in 1583 though. :)
> 
> This is just a utility function for mktime.  I think it's okay.  We
> should aim at making libcflat a minimal libc, and in that case we would
> move mktime to lib/.  Putting stuff directly in tests is good enough
> (worse is better), but let's remember that duplicated code is not.

I agree that there is no need to move stuff into lib/.
I just wouldn't expose DAYS in this shape even to mktime, because DAYS
makes little sense without the "month -= 2" fixup.

>>> > +	/* Put February at end of the year to avoid leap day this year */

Leap day is not the only reason.
'367UL * (m) / 12' equation needs to have shifted months to report the
correct numbers of days throughout any year.

>>> > +	/* compute epoch: substract DAYS(since_March(1-1-1970)) */
>>> > +	epoch = DAYS(year, month, day) - DAYS(1969, 11, 1);

And this comment points out that the API is bad. :)

(Btw. am I going overboard?)

Re: [kvm-unit-tests PATCH] powerpc: Add tests for RTAS

[Paolo Bonzini]

On 03/03/2016 18:09, Radim Krčmář wrote:
>>>>> >>> > +	/* compute epoch: substract DAYS(since_March(1-1-1970)) */
>>>>> >>> > +	epoch = DAYS(year, month, day) - DAYS(1969, 11, 1);
> And this comment points out that the API is bad. :)
> 
> (Btw. am I going overboard?)

Only because I've committed it already. ;)

Paolo

Re: [kvm-unit-tests PATCH] powerpc: Add tests for RTAS

[Radim Krčmář]

2016-03-03 18:12+0100, Paolo Bonzini:
> On 03/03/2016 18:09, Radim Krčmář wrote:
>>>>>> >>> > +	/* compute epoch: substract DAYS(since_March(1-1-1970)) */
>>>>>> >>> > +	epoch = DAYS(year, month, day) - DAYS(1969, 11, 1);
>> And this comment points out that the API is bad. :)
>> 
>> (Btw. am I going overboard?)
> 
> Only because I've committed it already. ;)

:D  The patch is ok, someone else is sending mails under my name.

Re: [kvm-unit-tests PATCH] powerpc: Add tests for RTAS

[Thomas Huth]

On 03.03.2016 13:28, Laurent Vivier wrote:
> By starting with get-time-of-day, set-time-of-day.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
...
> diff --git a/powerpc/rtas.c b/powerpc/rtas.c
> new file mode 100644
> index 0000000..9d673f0
> --- /dev/null
> +++ b/powerpc/rtas.c
> @@ -0,0 +1,148 @@
> +/*
> + * Test the RTAS interface
> + */
> +
> +#include <libcflat.h>
> +#include <util.h>
> +#include <asm/rtas.h>
> +
> +#define DAYS(y,m,d) (365UL * (y) + ((y) / 4) - ((y) / 100) + ((y) / 400) + \
> +		     367UL * (m) / 12  + \
> +		     (d))

A friendly comment before that macro would be nice - since it is not so
obvious what it is doing when you see it for the first time.

> +static unsigned long mktime(int year, int month, int day,
> +			    int hour, int minute, int second)
> +{
> +	unsigned long epoch;
> +
> +	/* Put February at end of the year to avoid leap day this year */
> +
> +	month -= 2;
> +	if (month <= 0) {
> +		month += 12;
> +		year -= 1;
> +	}
> +
> +	/* compute epoch: substract DAYS(since_March(1-1-1970)) */
> +
> +	epoch = DAYS(year, month, day) - DAYS(1969, 11, 1);
> +
> +	epoch = epoch * 24 + hour;
> +	epoch = epoch * 60 + minute;
> +	epoch = epoch * 60 + second;
> +
> +	return epoch;
> +}
> +
> +#define DELAY 1
> +#define MAX_LOOP 10000000
> +
> +static void check_get_time_of_day(unsigned long start)
> +{
> +	int token;
> +	int ret;
> +	int now[8];
> +	unsigned long t1, t2, count;
> +
> +	token = rtas_token("get-time-of-day");
> +	report("token available", token != RTAS_UNKNOWN_SERVICE);
> +	if (token = RTAS_UNKNOWN_SERVICE)
> +		return;
> +
> +	ret = rtas_call(token, 0, 8, now);
> +	report("execution", ret = 0);
> +
> +	report("second",  now[5] >= 0 && now[5] <= 59);
> +	report("minute", now[4] >= 0 && now[4] <= 59);
> +	report("hour", now[3] >= 0 && now[3] <= 23);
> +	report("day", now[2] >= 1 && now[2] <= 31);
> +	report("month", now[1] >= 1 && now[1] <= 12);
> +	report("year", now[0] >= 1970);
> +	report("accuracy (< 3s)", mktime(now[0], now[1], now[2],
> +					 now[3], now[4], now[5]) - start < 3);
> +
> +	ret = rtas_call(token, 0, 8, now);

Maybe make sure that ret = 0 here again? ... or you could simply omit
this call and recycle the results from the first rtas_call ?

> +	t1 = mktime(now[0], now[1], now[2], now[3], now[4], now[5]);
> +	count = 0;
> +	do {
> +		ret = rtas_call(token, 0, 8, now);
> +		t2 = mktime(now[0], now[1], now[2], now[3], now[4], now[5]);
> +		count++;
> +	} while (t1 + DELAY > t2 && count < MAX_LOOP);
> +	report("running", t1 + DELAY <= t2);

I think at least here you should add another "ret = 0" check again,
just to be sure.

> +}
> +
> +static void check_set_time_of_day(void)
> +{
> +	int token;
> +	int ret;
> +	int date[8];
> +	unsigned long t1, t2, count;
> +
> +	token = rtas_token("set-time-of-day");
> +	report("token available", token != RTAS_UNKNOWN_SERVICE);
> +	if (token = RTAS_UNKNOWN_SERVICE)
> +		return;
> +
> +	/* 23:59:59 28/2/2000 */
> +
> +	ret = rtas_call(token, 7, 1, NULL, 2000, 2, 28, 23, 59, 59);
> +	report("execution", ret = 0);
> +
> +	/* check it has worked */
> +	ret = rtas_call(rtas_token("get-time-of-day"), 0, 8, date);
> +	report("re-read", ret = 0);
> +	t1 = mktime(2000, 2, 28, 23, 59, 59);
> +	t2 = mktime(date[0], date[1], date[2],
> +		    date[3], date[4], date[5]);
> +	report("result", t2 - t1 < 2);
> +
> +	/* check it is running */
> +	count = 0;
> +	do {
> +		ret = rtas_call(rtas_token("get-time-of-day"), 0, 8, date);
> +		t2 = mktime(date[0], date[1], date[2],
> +			    date[3], date[4], date[5]);
> +		count++;
> +	} while (t1 + DELAY > t2 && count < MAX_LOOP);
> +	report("running", t1 + DELAY <= t2);

Please also add a check for "ret = 0" here ... just to be really,
really sure ;-)

> +}

... but apart from these minor issues, the patch looks pretty good to me
already!

 Thomas