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

[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<=12;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<=12;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

[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

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.