| Message ID | 1457008099-29944-1-git-send-email-lvivier@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
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)) > + -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)) >> + -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?) -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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