From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thomas Huth <thuth@redhat.com>
Subject: Re: [kvm-unit-tests PATCH] powerpc: Add tests for RTAS
Date: Thu, 3 Mar 2016 17:54:54 +0100
Message-ID: <56D86C5E.8050900@redhat.com>
References: <1457008099-29944-1-git-send-email-lvivier@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Cc: drjones@redhat.com, dgibson@redhat.com, pbonzini@redhat.com
To: Laurent Vivier <lvivier@redhat.com>, kvm@vger.kernel.org,
	kvm-ppc@vger.kernel.org
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:49444 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S932886AbcCCQzD (ORCPT <rfc822;kvm@vger.kernel.org>);
	Thu, 3 Mar 2016 11:55:03 -0500
In-Reply-To: <1457008099-29944-1-git-send-email-lvivier@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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