All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/4] MIPS: Fix several VDSO-related issues
@ 2017-06-28 15:55 Aleksandar Markovic
  2017-06-28 15:55 ` [PATCH v2 1/4] MIPS: VDSO: Fix conversions in do_monotonic()/do_monotonic_coarse() Aleksandar Markovic
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Aleksandar Markovic @ 2017-06-28 15:55 UTC (permalink / raw)
  To: linux-mips
  Cc: Aleksandar Markovic, Douglas Leung, Goran Ferenc, James Hogan,
	linux-kernel, Miodrag Dinic, Paul Burton, Petar Jovanovic,
	Raghu Gandham, Ralf Baechle

From: Aleksandar Markovic <aleksandar.markovic@imgtec.com>

v1->v2:

    - updated recipient lists using get_maintainer.pl
    - rebased to the letest kernel code

The patches in this series all deal with VDSO, and all originate from
the develpoment of Android emulator for Mips..

The first patch is a fix for incorrect time values returned under
certain conditions.

The second and third patches provide fallback mechanism for
clock_gettime() and gettimeofday() system calls within kernel VDSO
code. This actually brings Mips code to be in sync with other major
platforms (intel, arm, and others) (with respect to the division of
responsibility between glibc and kernel regarding VDSO functions
fallbacks). It seems to us that proposed organization is simpler
and easier for maintenance in the long run. However, since it affects
interaction between glibc and kernel, it needs to be communicated to
and reviewed by Mips glibc developers.

The fourth patch is just a correction of a comment.

Aleksandar Markovic (1):
  MIPS: VDSO: Fix a mismatch between comment and preprocessor constant

Goran Ferenc (3):
  MIPS: VDSO: Fix conversions in do_monotonic()/do_monotonic_coarse()
  MIPS: VDSO: Add implementation of clock_gettime() fallback
  MIPS: VDSO: Add implementation of gettimeofday() fallback

 arch/mips/include/asm/vdso.h  |  4 +--
 arch/mips/vdso/gettimeofday.c | 59 ++++++++++++++++++++++++++++++++++++-------
 2 files changed, 52 insertions(+), 11 deletions(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 1/4] MIPS: VDSO: Fix conversions in do_monotonic()/do_monotonic_coarse()
  2017-06-28 15:55 [PATCH v2 0/4] MIPS: Fix several VDSO-related issues Aleksandar Markovic
@ 2017-06-28 15:55 ` Aleksandar Markovic
  2017-06-28 15:55 ` [PATCH v2 2/4] MIPS: VDSO: Add implementation of clock_gettime() fallback Aleksandar Markovic
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 13+ messages in thread
From: Aleksandar Markovic @ 2017-06-28 15:55 UTC (permalink / raw)
  To: linux-mips
  Cc: Goran Ferenc, Miodrag Dinic, Aleksandar Markovic, Douglas Leung,
	James Hogan, linux-kernel, Paul Burton, Petar Jovanovic,
	Raghu Gandham, Ralf Baechle

From: Goran Ferenc <goran.ferenc@imgtec.com>

Fix incorrect calculation in do_monotonic() and do_monotonic_coarse()
function that in turn caused incorrect values returned by the vdso
version of system call clock_gettime() on mips64 if its system clock
ID parameter was CLOCK_MONOTONIC or CLOCK_MONOTONIC_COARSE.

Consider these variables and their types on mips32 and mips64:

tk->wall_to_monotonic.tv_sec  s64, s64   (kernel/vdso.c)
vdso_data.wall_to_mono_sec    u32, u32   (kernel/vdso.c)
to_mono_sec                   u32, u32   (vdso/gettimeofday.c)
ts->tv_sec                    s32, s64   (vdso/gettimeofday.c)

For mips64 case, u32 vdso_data.wall_to_mono_sec variable is updated
from the 64-bit signed variable tk->wall_to_monotonic.tv_sec
(kernel/vdso.c:76) which is a negative number holding the time passed
from 1970-01-01 to the time boot started. This 64-bit signed value is
currently around 47+ years, in seconds. For instance, let this value
be:

-1489757461

or

11111111111111111111111111111111 10100111001101000001101011101011

By updating 32-bit vdso_data.wall_to_mono_sec variable, we lose upper
32 bits (signed 1's).

to_mono_sec variable is a parameter of do_monotonic() and
do_monotonic_coarse() functions which holds vdso_data.wall_to_mono_sec
value. Its value needs to be added (or subtracted considering it holds
negative value from the tk->wall_to_monotonic.tv_sec) to the current
time passed from 1970-01-01 (ts->tv_sec), which is again something like
47+ years, but increased by the time passed from the boot to the
current time. ts->tv_sec is 32-bit long in case of 32-bit architecture
and 64-bit long in case of 64-bit architecture. Consider the update of
ts->tv_sec (vdso/gettimeofday.c:55 & 167):

ts->tv_sec += to_mono_sec;

mips32 case: This update will be performed correctly, since both
ts->tv_sec and to_mono_sec are 32-bit long and the sign in to_mono_sec
is preserved. Implicit conversion from u32 to s32 will be done
correctly.

mips64 case: This update will be wrong, since the implicit conversion
will not be done correctly. The reason is that the conversion will be
from u32 to s64. This is because to_mono_sec is 32-bit long for both
mips32 and mips64 cases and s64..33 bits of converted to_mono_sec
variable will be zeros.

So, in order to make MIPS64 implementation work properly for
MONOTONIC and MONOTONIC_COARSE clock ids on mips64, the size of
wall_to_mono_sec variable in mips_vdso_data union and respective
parameters in do_monotonic() and do_monotonic_coarse() functions
should be changed from u32 to u64. Because of consistency, this
size change from u32 and u64 is also done for wall_to_mono_nsec
variable and corresponding function parameters.

As far as similar situations for other architectures are concerned,
let's take a look at arm. Arm has two distinct vdso_data structures
for 32-bit & 64-bit cases, and arm's wall_to_mono_sec and
wall_to_mono_nsec are u32 for 32-bit and u64 for 64-bit cases.
On the other hand, MIPS has only one structure (mips_vdso_data),
hence the need for changing the size of above mentioned parameters.

Signed-off-by: Goran Ferenc <goran.ferenc@imgtec.com>
Signed-off-by: Miodrag Dinic <miodrag.dinic@imgtec.com>
Signed-off-by: Aleksandar Markovic <aleksandar.markovic@imgtec.com>
---
 arch/mips/include/asm/vdso.h  | 4 ++--
 arch/mips/vdso/gettimeofday.c | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/mips/include/asm/vdso.h b/arch/mips/include/asm/vdso.h
index 8f4ca5d..b7cd6cf 100644
--- a/arch/mips/include/asm/vdso.h
+++ b/arch/mips/include/asm/vdso.h
@@ -79,8 +79,8 @@ union mips_vdso_data {
 	struct {
 		u64 xtime_sec;
 		u64 xtime_nsec;
-		u32 wall_to_mono_sec;
-		u32 wall_to_mono_nsec;
+		u64 wall_to_mono_sec;
+		u64 wall_to_mono_nsec;
 		u32 seq_count;
 		u32 cs_shift;
 		u8 clock_mode;
diff --git a/arch/mips/vdso/gettimeofday.c b/arch/mips/vdso/gettimeofday.c
index ce89c9e..fd7d433 100644
--- a/arch/mips/vdso/gettimeofday.c
+++ b/arch/mips/vdso/gettimeofday.c
@@ -39,8 +39,8 @@ static __always_inline int do_monotonic_coarse(struct timespec *ts,
 					       const union mips_vdso_data *data)
 {
 	u32 start_seq;
-	u32 to_mono_sec;
-	u32 to_mono_nsec;
+	u64 to_mono_sec;
+	u64 to_mono_nsec;
 
 	do {
 		start_seq = vdso_data_read_begin(data);
@@ -148,8 +148,8 @@ static __always_inline int do_monotonic(struct timespec *ts,
 {
 	u32 start_seq;
 	u64 ns;
-	u32 to_mono_sec;
-	u32 to_mono_nsec;
+	u64 to_mono_sec;
+	u64 to_mono_nsec;
 
 	do {
 		start_seq = vdso_data_read_begin(data);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 2/4] MIPS: VDSO: Add implementation of clock_gettime() fallback
  2017-06-28 15:55 [PATCH v2 0/4] MIPS: Fix several VDSO-related issues Aleksandar Markovic
  2017-06-28 15:55 ` [PATCH v2 1/4] MIPS: VDSO: Fix conversions in do_monotonic()/do_monotonic_coarse() Aleksandar Markovic
@ 2017-06-28 15:55 ` Aleksandar Markovic
  2017-07-06  0:00     ` Maciej W. Rozycki
  2017-06-28 15:55 ` [PATCH v2 3/4] MIPS: VDSO: Add implementation of gettimeofday() fallback Aleksandar Markovic
  2017-06-28 15:55 ` [PATCH v2 4/4] MIPS: VDSO: Fix a mismatch between comment and preprocessor constant Aleksandar Markovic
  3 siblings, 1 reply; 13+ messages in thread
From: Aleksandar Markovic @ 2017-06-28 15:55 UTC (permalink / raw)
  To: linux-mips
  Cc: Goran Ferenc, Miodrag Dinic, Aleksandar Markovic, Douglas Leung,
	James Hogan, linux-kernel, Paul Burton, Petar Jovanovic,
	Raghu Gandham, Ralf Baechle

From: Goran Ferenc <goran.ferenc@imgtec.com>

This patch adds clock_gettime_fallback() function that wraps assembly
invocation of clock_gettime() syscall using __NR_clock_gettime.

This function is used if pure VDSO implementation of clock_gettime()
does not succeed for any reason. For example, it is called if the
clkid parameter of clock_gettime() is not one of the clkids listed
in the switch-case block of the function __vdso_clock_gettime()
(one such case for clkid is CLOCK_BOOTIME).

If syscall invocation via __NR_clock_gettime fails, register a3 will
be set. So, after the syscall, register a3 is tested and the return
value is negated if it's set.

Signed-off-by: Goran Ferenc <goran.ferenc@imgtec.com>
Signed-off-by: Miodrag Dinic <miodrag.dinic@imgtec.com>
Signed-off-by: Aleksandar Markovic <aleksandar.markovic@imgtec.com>
---
 arch/mips/vdso/gettimeofday.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/arch/mips/vdso/gettimeofday.c b/arch/mips/vdso/gettimeofday.c
index fd7d433..5f63375 100644
--- a/arch/mips/vdso/gettimeofday.c
+++ b/arch/mips/vdso/gettimeofday.c
@@ -20,6 +20,24 @@
 #include <asm/unistd.h>
 #include <asm/vdso.h>
 
+static __always_inline long clock_gettime_fallback(clockid_t _clkid,
+					   struct timespec *_ts)
+{
+	register struct timespec *ts asm("a1") = _ts;
+	register clockid_t clkid asm("a0") = _clkid;
+	register long ret asm("v0");
+	register long nr asm("v0") = __NR_clock_gettime;
+	register long error asm("a3");
+
+	asm volatile(
+	"       syscall\n"
+	: "=r" (ret), "=r" (error)
+	: "r" (clkid), "r" (ts), "r" (nr)
+	: "memory");
+
+	return error ? -ret : ret;
+}
+
 static __always_inline int do_realtime_coarse(struct timespec *ts,
 					      const union mips_vdso_data *data)
 {
@@ -207,7 +225,7 @@ int __vdso_gettimeofday(struct timeval *tv, struct timezone *tz)
 int __vdso_clock_gettime(clockid_t clkid, struct timespec *ts)
 {
 	const union mips_vdso_data *data = get_vdso_data();
-	int ret;
+	int ret = -1;
 
 	switch (clkid) {
 	case CLOCK_REALTIME_COARSE:
@@ -223,10 +241,11 @@ int __vdso_clock_gettime(clockid_t clkid, struct timespec *ts)
 		ret = do_monotonic(ts, data);
 		break;
 	default:
-		ret = -ENOSYS;
 		break;
 	}
 
-	/* If we return -ENOSYS libc should fall back to a syscall. */
+	if (ret)
+		ret = clock_gettime_fallback(clkid, ts);
+
 	return ret;
 }
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 3/4] MIPS: VDSO: Add implementation of gettimeofday() fallback
  2017-06-28 15:55 [PATCH v2 0/4] MIPS: Fix several VDSO-related issues Aleksandar Markovic
  2017-06-28 15:55 ` [PATCH v2 1/4] MIPS: VDSO: Fix conversions in do_monotonic()/do_monotonic_coarse() Aleksandar Markovic
  2017-06-28 15:55 ` [PATCH v2 2/4] MIPS: VDSO: Add implementation of clock_gettime() fallback Aleksandar Markovic
@ 2017-06-28 15:55 ` Aleksandar Markovic
  2017-06-28 15:55 ` [PATCH v2 4/4] MIPS: VDSO: Fix a mismatch between comment and preprocessor constant Aleksandar Markovic
  3 siblings, 0 replies; 13+ messages in thread
From: Aleksandar Markovic @ 2017-06-28 15:55 UTC (permalink / raw)
  To: linux-mips
  Cc: Goran Ferenc, Miodrag Dinic, Aleksandar Markovic, Douglas Leung,
	James Hogan, linux-kernel, Paul Burton, Petar Jovanovic,
	Raghu Gandham, Ralf Baechle

From: Goran Ferenc <goran.ferenc@imgtec.com>

This patch adds gettimeofday_fallback() function that wraps assembly
invocation of gettimeofday() syscall using __NR_gettimeofday.

This function is used if pure VDSO implementation gettimeofday()
does not succeed for any reason. Its imeplementation is enclosed in
"#ifdef CONFIG_MIPS_CLOCK_VSYSCALL" to be in sync with the similar
arrangement for __vdso_gettimeofday().

If syscall invocation via __NR_gettimeofday fails, register a3 will
be set. So, after the syscall, register a3 is tested and the return
valuem is negated if it's set.

Signed-off-by: Goran Ferenc <goran.ferenc@imgtec.com>
Signed-off-by: Miodrag Dinic <miodrag.dinic@imgtec.com>
Signed-off-by: Aleksandar Markovic <aleksandar.markovic@imgtec.com>
---
 arch/mips/vdso/gettimeofday.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/arch/mips/vdso/gettimeofday.c b/arch/mips/vdso/gettimeofday.c
index 5f63375..23305bf 100644
--- a/arch/mips/vdso/gettimeofday.c
+++ b/arch/mips/vdso/gettimeofday.c
@@ -20,6 +20,28 @@
 #include <asm/unistd.h>
 #include <asm/vdso.h>
 
+#ifdef CONFIG_MIPS_CLOCK_VSYSCALL
+
+static __always_inline long gettimeofday_fallback(struct timeval *_tv,
+					  struct timezone *_tz)
+{
+	register struct timezone *tz asm("a1") = _tz;
+	register struct timeval *tv asm("a0") = _tv;
+	register long ret asm("v0");
+	register long nr asm("v0") = __NR_gettimeofday;
+	register long error asm("a3");
+
+	asm volatile(
+	"       syscall\n"
+	: "=r" (ret), "=r" (error)
+	: "r" (tv), "r" (tz), "r" (nr)
+	: "memory");
+
+	return error ? -ret : ret;
+}
+
+#endif
+
 static __always_inline long clock_gettime_fallback(clockid_t _clkid,
 					   struct timespec *_ts)
 {
@@ -205,7 +227,7 @@ int __vdso_gettimeofday(struct timeval *tv, struct timezone *tz)
 
 	ret = do_realtime(&ts, data);
 	if (ret)
-		return ret;
+		return gettimeofday_fallback(tv, tz);
 
 	if (tv) {
 		tv->tv_sec = ts.tv_sec;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 4/4] MIPS: VDSO: Fix a mismatch between comment and preprocessor constant
  2017-06-28 15:55 [PATCH v2 0/4] MIPS: Fix several VDSO-related issues Aleksandar Markovic
                   ` (2 preceding siblings ...)
  2017-06-28 15:55 ` [PATCH v2 3/4] MIPS: VDSO: Add implementation of gettimeofday() fallback Aleksandar Markovic
@ 2017-06-28 15:55 ` Aleksandar Markovic
  3 siblings, 0 replies; 13+ messages in thread
From: Aleksandar Markovic @ 2017-06-28 15:55 UTC (permalink / raw)
  To: linux-mips
  Cc: Aleksandar Markovic, Douglas Leung, Goran Ferenc, James Hogan,
	linux-kernel, Miodrag Dinic, Paul Burton, Petar Jovanovic,
	Raghu Gandham, Ralf Baechle

From: Aleksandar Markovic <aleksandar.markovic@imgtec.com>

Sync the comment with its preprocessor constant counterpart.

Signed-off-by: Aleksandar Markovic <aleksandar.markovic@imgtec.com>
---
 arch/mips/vdso/gettimeofday.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/vdso/gettimeofday.c b/arch/mips/vdso/gettimeofday.c
index 23305bf..974276e 100644
--- a/arch/mips/vdso/gettimeofday.c
+++ b/arch/mips/vdso/gettimeofday.c
@@ -242,7 +242,7 @@ int __vdso_gettimeofday(struct timeval *tv, struct timezone *tz)
 	return 0;
 }
 
-#endif /* CONFIG_CLKSRC_MIPS_GIC */
+#endif /* CONFIG_MIPS_CLOCK_VSYSCALL */
 
 int __vdso_clock_gettime(clockid_t clkid, struct timespec *ts)
 {
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/4] MIPS: VDSO: Add implementation of clock_gettime() fallback
@ 2017-07-06  0:00     ` Maciej W. Rozycki
  0 siblings, 0 replies; 13+ messages in thread
From: Maciej W. Rozycki @ 2017-07-06  0:00 UTC (permalink / raw)
  To: Aleksandar Markovic
  Cc: linux-mips, Goran Ferenc, Miodrag Dinic, Aleksandar Markovic,
	Douglas Leung, James Hogan, linux-kernel, Paul Burton,
	Petar Jovanovic, Raghu Gandham, Ralf Baechle

On Wed, 28 Jun 2017, Aleksandar Markovic wrote:

> diff --git a/arch/mips/vdso/gettimeofday.c b/arch/mips/vdso/gettimeofday.c
> index fd7d433..5f63375 100644
> --- a/arch/mips/vdso/gettimeofday.c
> +++ b/arch/mips/vdso/gettimeofday.c
> @@ -20,6 +20,24 @@
>  #include <asm/unistd.h>
>  #include <asm/vdso.h>
>  
> +static __always_inline long clock_gettime_fallback(clockid_t _clkid,
> +					   struct timespec *_ts)
> +{
> +	register struct timespec *ts asm("a1") = _ts;
> +	register clockid_t clkid asm("a0") = _clkid;
> +	register long ret asm("v0");
> +	register long nr asm("v0") = __NR_clock_gettime;
> +	register long error asm("a3");
> +
> +	asm volatile(
> +	"       syscall\n"
> +	: "=r" (ret), "=r" (error)
> +	: "r" (clkid), "r" (ts), "r" (nr)
> +	: "memory");
> +
> +	return error ? -ret : ret;
> +}

 Hmm, are you sure it is safe nowadays WRT the syscall restart convention 
to leave out the instruction explicitly loading the syscall number that 
would normally immediately precede SYSCALL (and would have to forcibly use 
the 32-bit encoding in the microMIPS case)?

  Maciej

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/4] MIPS: VDSO: Add implementation of clock_gettime() fallback
@ 2017-07-06  0:00     ` Maciej W. Rozycki
  0 siblings, 0 replies; 13+ messages in thread
From: Maciej W. Rozycki @ 2017-07-06  0:00 UTC (permalink / raw)
  To: Aleksandar Markovic
  Cc: linux-mips, Goran Ferenc, Miodrag Dinic, Aleksandar Markovic,
	Douglas Leung, James Hogan, linux-kernel, Paul Burton,
	Petar Jovanovic, Raghu Gandham, Ralf Baechle

On Wed, 28 Jun 2017, Aleksandar Markovic wrote:

> diff --git a/arch/mips/vdso/gettimeofday.c b/arch/mips/vdso/gettimeofday.c
> index fd7d433..5f63375 100644
> --- a/arch/mips/vdso/gettimeofday.c
> +++ b/arch/mips/vdso/gettimeofday.c
> @@ -20,6 +20,24 @@
>  #include <asm/unistd.h>
>  #include <asm/vdso.h>
>  
> +static __always_inline long clock_gettime_fallback(clockid_t _clkid,
> +					   struct timespec *_ts)
> +{
> +	register struct timespec *ts asm("a1") = _ts;
> +	register clockid_t clkid asm("a0") = _clkid;
> +	register long ret asm("v0");
> +	register long nr asm("v0") = __NR_clock_gettime;
> +	register long error asm("a3");
> +
> +	asm volatile(
> +	"       syscall\n"
> +	: "=r" (ret), "=r" (error)
> +	: "r" (clkid), "r" (ts), "r" (nr)
> +	: "memory");
> +
> +	return error ? -ret : ret;
> +}

 Hmm, are you sure it is safe nowadays WRT the syscall restart convention 
to leave out the instruction explicitly loading the syscall number that 
would normally immediately precede SYSCALL (and would have to forcibly use 
the 32-bit encoding in the microMIPS case)?

  Maciej

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/4] MIPS: VDSO: Add implementation of clock_gettime() fallback
@ 2017-07-06  9:05       ` James Hogan
  0 siblings, 0 replies; 13+ messages in thread
From: James Hogan @ 2017-07-06  9:05 UTC (permalink / raw)
  To: Maciej W. Rozycki
  Cc: Aleksandar Markovic, linux-mips, Goran Ferenc, Miodrag Dinic,
	Aleksandar Markovic, Douglas Leung, linux-kernel, Paul Burton,
	Petar Jovanovic, Raghu Gandham, Ralf Baechle

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

On Thu, Jul 06, 2017 at 01:00:34AM +0100, Maciej W. Rozycki wrote:
> On Wed, 28 Jun 2017, Aleksandar Markovic wrote:
> 
> > diff --git a/arch/mips/vdso/gettimeofday.c b/arch/mips/vdso/gettimeofday.c
> > index fd7d433..5f63375 100644
> > --- a/arch/mips/vdso/gettimeofday.c
> > +++ b/arch/mips/vdso/gettimeofday.c
> > @@ -20,6 +20,24 @@
> >  #include <asm/unistd.h>
> >  #include <asm/vdso.h>
> >  
> > +static __always_inline long clock_gettime_fallback(clockid_t _clkid,
> > +					   struct timespec *_ts)
> > +{
> > +	register struct timespec *ts asm("a1") = _ts;
> > +	register clockid_t clkid asm("a0") = _clkid;
> > +	register long ret asm("v0");
> > +	register long nr asm("v0") = __NR_clock_gettime;
> > +	register long error asm("a3");
> > +
> > +	asm volatile(
> > +	"       syscall\n"
> > +	: "=r" (ret), "=r" (error)
> > +	: "r" (clkid), "r" (ts), "r" (nr)
> > +	: "memory");
> > +
> > +	return error ? -ret : ret;
> > +}
> 
>  Hmm, are you sure it is safe nowadays WRT the syscall restart convention 
> to leave out the instruction explicitly loading the syscall number that 
> would normally immediately precede SYSCALL

It should be fine. syscall restart only rewinds one (32-bit)
instruction, and it preserves the syscall number in pt_regs::regs[0]
(see handle_signal() / do_signal() and this code in e.g. scall32-o32.S:)

sw      t1, PT_R0(sp)           # save it for syscall restarting

> (and would have to forcibly use the 32-bit encoding in the microMIPS
> case)?

I don't believe there is a 16-bit SYSCALL encoding in microMIPS, at
least I can't see one in the 5.04 manual.

However, the clobber list is incomplete.
The following registers are written as outputs:
	$2 (v0), $7 (a3)
The following registers are used as arguments and should be preserved:
	$4-$6 (a0-a2), [$8-$9 (a4-a5)] (n32 / n64 only)
And the following other registers are preserved:
	$16-$23, $28-$31
So assuming you already have $2 and $7 as outputs, the clobber list
should be:
	"$1", "$3", ["$8", "$9",] "$10", "$11", "$12", "$13", "$14",
	"$15", "$24", "$25", "hi", "lo", "memory"

(only o32 needs to mark $8-$9 clobbered, but no harm doing so on n32/n64
too)

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/4] MIPS: VDSO: Add implementation of clock_gettime() fallback
@ 2017-07-06  9:05       ` James Hogan
  0 siblings, 0 replies; 13+ messages in thread
From: James Hogan @ 2017-07-06  9:05 UTC (permalink / raw)
  To: Maciej W. Rozycki
  Cc: Aleksandar Markovic, linux-mips, Goran Ferenc, Miodrag Dinic,
	Aleksandar Markovic, Douglas Leung, linux-kernel, Paul Burton,
	Petar Jovanovic, Raghu Gandham, Ralf Baechle

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

On Thu, Jul 06, 2017 at 01:00:34AM +0100, Maciej W. Rozycki wrote:
> On Wed, 28 Jun 2017, Aleksandar Markovic wrote:
> 
> > diff --git a/arch/mips/vdso/gettimeofday.c b/arch/mips/vdso/gettimeofday.c
> > index fd7d433..5f63375 100644
> > --- a/arch/mips/vdso/gettimeofday.c
> > +++ b/arch/mips/vdso/gettimeofday.c
> > @@ -20,6 +20,24 @@
> >  #include <asm/unistd.h>
> >  #include <asm/vdso.h>
> >  
> > +static __always_inline long clock_gettime_fallback(clockid_t _clkid,
> > +					   struct timespec *_ts)
> > +{
> > +	register struct timespec *ts asm("a1") = _ts;
> > +	register clockid_t clkid asm("a0") = _clkid;
> > +	register long ret asm("v0");
> > +	register long nr asm("v0") = __NR_clock_gettime;
> > +	register long error asm("a3");
> > +
> > +	asm volatile(
> > +	"       syscall\n"
> > +	: "=r" (ret), "=r" (error)
> > +	: "r" (clkid), "r" (ts), "r" (nr)
> > +	: "memory");
> > +
> > +	return error ? -ret : ret;
> > +}
> 
>  Hmm, are you sure it is safe nowadays WRT the syscall restart convention 
> to leave out the instruction explicitly loading the syscall number that 
> would normally immediately precede SYSCALL

It should be fine. syscall restart only rewinds one (32-bit)
instruction, and it preserves the syscall number in pt_regs::regs[0]
(see handle_signal() / do_signal() and this code in e.g. scall32-o32.S:)

sw      t1, PT_R0(sp)           # save it for syscall restarting

> (and would have to forcibly use the 32-bit encoding in the microMIPS
> case)?

I don't believe there is a 16-bit SYSCALL encoding in microMIPS, at
least I can't see one in the 5.04 manual.

However, the clobber list is incomplete.
The following registers are written as outputs:
	$2 (v0), $7 (a3)
The following registers are used as arguments and should be preserved:
	$4-$6 (a0-a2), [$8-$9 (a4-a5)] (n32 / n64 only)
And the following other registers are preserved:
	$16-$23, $28-$31
So assuming you already have $2 and $7 as outputs, the clobber list
should be:
	"$1", "$3", ["$8", "$9",] "$10", "$11", "$12", "$13", "$14",
	"$15", "$24", "$25", "hi", "lo", "memory"

(only o32 needs to mark $8-$9 clobbered, but no harm doing so on n32/n64
too)

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/4] MIPS: VDSO: Add implementation of clock_gettime() fallback
@ 2017-07-06 13:12         ` Maciej W. Rozycki
  0 siblings, 0 replies; 13+ messages in thread
From: Maciej W. Rozycki @ 2017-07-06 13:12 UTC (permalink / raw)
  To: James Hogan
  Cc: Aleksandar Markovic, linux-mips, Goran Ferenc, Miodrag Dinic,
	Aleksandar Markovic, Douglas Leung, linux-kernel, Paul Burton,
	Petar Jovanovic, Raghu Gandham, Ralf Baechle

On Thu, 6 Jul 2017, James Hogan wrote:

> > > +	asm volatile(
> > > +	"       syscall\n"
> > > +	: "=r" (ret), "=r" (error)
> > > +	: "r" (clkid), "r" (ts), "r" (nr)
> > > +	: "memory");
> > > +
> > > +	return error ? -ret : ret;
> > > +}
> > 
> >  Hmm, are you sure it is safe nowadays WRT the syscall restart convention 
> > to leave out the instruction explicitly loading the syscall number that 
> > would normally immediately precede SYSCALL
> 
> It should be fine. syscall restart only rewinds one (32-bit)
> instruction, and it preserves the syscall number in pt_regs::regs[0]
> (see handle_signal() / do_signal() and this code in e.g. scall32-o32.S:)
> 
> sw      t1, PT_R0(sp)           # save it for syscall restarting

 Fair enough, I just wanted to be sure.

 [This user code is bundled with the kernel, so it can assume whatever the 
kernel does, however general user code does have to conform to the legacy 
restart convention, unless it also requires a kernel version that is new 
enough and has a safety check in place.]

> > (and would have to forcibly use the 32-bit encoding in the microMIPS
> > case)?
> 
> I don't believe there is a 16-bit SYSCALL encoding in microMIPS, at
> least I can't see one in the 5.04 manual.

 I referred to the preceding instruction, presumably LI, that does have a 
16-bit variant in the microMIPS instruction set.

  Maciej

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/4] MIPS: VDSO: Add implementation of clock_gettime() fallback
@ 2017-07-06 13:12         ` Maciej W. Rozycki
  0 siblings, 0 replies; 13+ messages in thread
From: Maciej W. Rozycki @ 2017-07-06 13:12 UTC (permalink / raw)
  To: James Hogan
  Cc: Aleksandar Markovic, linux-mips, Goran Ferenc, Miodrag Dinic,
	Aleksandar Markovic, Douglas Leung, linux-kernel, Paul Burton,
	Petar Jovanovic, Raghu Gandham, Ralf Baechle

On Thu, 6 Jul 2017, James Hogan wrote:

> > > +	asm volatile(
> > > +	"       syscall\n"
> > > +	: "=r" (ret), "=r" (error)
> > > +	: "r" (clkid), "r" (ts), "r" (nr)
> > > +	: "memory");
> > > +
> > > +	return error ? -ret : ret;
> > > +}
> > 
> >  Hmm, are you sure it is safe nowadays WRT the syscall restart convention 
> > to leave out the instruction explicitly loading the syscall number that 
> > would normally immediately precede SYSCALL
> 
> It should be fine. syscall restart only rewinds one (32-bit)
> instruction, and it preserves the syscall number in pt_regs::regs[0]
> (see handle_signal() / do_signal() and this code in e.g. scall32-o32.S:)
> 
> sw      t1, PT_R0(sp)           # save it for syscall restarting

 Fair enough, I just wanted to be sure.

 [This user code is bundled with the kernel, so it can assume whatever the 
kernel does, however general user code does have to conform to the legacy 
restart convention, unless it also requires a kernel version that is new 
enough and has a safety check in place.]

> > (and would have to forcibly use the 32-bit encoding in the microMIPS
> > case)?
> 
> I don't believe there is a 16-bit SYSCALL encoding in microMIPS, at
> least I can't see one in the 5.04 manual.

 I referred to the preceding instruction, presumably LI, that does have a 
16-bit variant in the microMIPS instruction set.

  Maciej

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/4] MIPS: VDSO: Add implementation of clock_gettime() fallback
@ 2017-07-06 14:09           ` James Hogan
  0 siblings, 0 replies; 13+ messages in thread
From: James Hogan @ 2017-07-06 14:09 UTC (permalink / raw)
  To: Maciej W. Rozycki
  Cc: Aleksandar Markovic, linux-mips, Goran Ferenc, Miodrag Dinic,
	Aleksandar Markovic, Douglas Leung, linux-kernel, Paul Burton,
	Petar Jovanovic, Raghu Gandham, Ralf Baechle

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

On Thu, Jul 06, 2017 at 02:12:37PM +0100, Maciej W. Rozycki wrote:
> On Thu, 6 Jul 2017, James Hogan wrote:
> > > (and would have to forcibly use the 32-bit encoding in the microMIPS
> > > case)?
> > 
> > I don't believe there is a 16-bit SYSCALL encoding in microMIPS, at
> > least I can't see one in the 5.04 manual.
> 
>  I referred to the preceding instruction, presumably LI, that does have a 
> 16-bit variant in the microMIPS instruction set.

Ah yes, I see what you mean.

Hopefully microMIPS support is new enough for that not to matter.

It appears that the restart behaviour was improved in v2.6.36 in commit
8f5a00eb422e ("MIPS: Sanitize restart logics"), whereas first mentions
of micromips are in v3.9, in commit f8fa4811dbb2 ("MIPS: Add support for
the M14KEc core.").

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/4] MIPS: VDSO: Add implementation of clock_gettime() fallback
@ 2017-07-06 14:09           ` James Hogan
  0 siblings, 0 replies; 13+ messages in thread
From: James Hogan @ 2017-07-06 14:09 UTC (permalink / raw)
  To: Maciej W. Rozycki
  Cc: Aleksandar Markovic, linux-mips, Goran Ferenc, Miodrag Dinic,
	Aleksandar Markovic, Douglas Leung, linux-kernel, Paul Burton,
	Petar Jovanovic, Raghu Gandham, Ralf Baechle

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

On Thu, Jul 06, 2017 at 02:12:37PM +0100, Maciej W. Rozycki wrote:
> On Thu, 6 Jul 2017, James Hogan wrote:
> > > (and would have to forcibly use the 32-bit encoding in the microMIPS
> > > case)?
> > 
> > I don't believe there is a 16-bit SYSCALL encoding in microMIPS, at
> > least I can't see one in the 5.04 manual.
> 
>  I referred to the preceding instruction, presumably LI, that does have a 
> 16-bit variant in the microMIPS instruction set.

Ah yes, I see what you mean.

Hopefully microMIPS support is new enough for that not to matter.

It appears that the restart behaviour was improved in v2.6.36 in commit
8f5a00eb422e ("MIPS: Sanitize restart logics"), whereas first mentions
of micromips are in v3.9, in commit f8fa4811dbb2 ("MIPS: Add support for
the M14KEc core.").

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2017-07-06 14:09 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-28 15:55 [PATCH v2 0/4] MIPS: Fix several VDSO-related issues Aleksandar Markovic
2017-06-28 15:55 ` [PATCH v2 1/4] MIPS: VDSO: Fix conversions in do_monotonic()/do_monotonic_coarse() Aleksandar Markovic
2017-06-28 15:55 ` [PATCH v2 2/4] MIPS: VDSO: Add implementation of clock_gettime() fallback Aleksandar Markovic
2017-07-06  0:00   ` Maciej W. Rozycki
2017-07-06  0:00     ` Maciej W. Rozycki
2017-07-06  9:05     ` James Hogan
2017-07-06  9:05       ` James Hogan
2017-07-06 13:12       ` Maciej W. Rozycki
2017-07-06 13:12         ` Maciej W. Rozycki
2017-07-06 14:09         ` James Hogan
2017-07-06 14:09           ` James Hogan
2017-06-28 15:55 ` [PATCH v2 3/4] MIPS: VDSO: Add implementation of gettimeofday() fallback Aleksandar Markovic
2017-06-28 15:55 ` [PATCH v2 4/4] MIPS: VDSO: Fix a mismatch between comment and preprocessor constant Aleksandar Markovic

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.