[Adeos-main] [PATCH 0/2] Host realtime clock support

[Adeos-main] [PATCH 0/2] Host realtime clock support

[Wolfgang Mauerer]

(This is the ipipe part of the mechanism, please see the Xenomai
 mailing list for the patches that make use of the infrastructure)

This patch series extends Xenomai with a new clock, CLOCK_HOST_REALTIME.
It allows for sharing NTP-corrected real time timestamps between
Linux/ipipe and Xenomai. The data are also available in userspace and can
be read without switching to kernel mode. Notice, however, that the new
clock only enables to read to time, but cannot serve as a full time basis.
Some changes to the ipipe layer are required as basis.

In contrast to the initial approach, we don't use a transactional mechanism
to copy the information over from Linux, but use classical synchronisation.
The code can be compiled in conditionally for both, ipipe and Xenomai. When
disabled by architectures that don't support apt clock sources, there is
no runtime-overhead associated with the feature.

Some points that may require further discussion:

- POSIX only specifies a few clock_ids, and these have already been
  extended by the Linux kernel. We use the maximum id (16) for the new
  clock, but it might also make sense to use 7 (CLOCK_MONOTONIC_COARSE+1)
  or 4 (CLOCK_THREAD_CPUTIME_ID+1).

- The current implementation deals with x86_64's TSC. Support for other
  architectures can be added. Additionally, the user has to make sure that
  the TSC clock source remains active once selected.  To implement 
  deactivation (e.g., when the Linux clock source is changed), more 
  ipipe hooks would be required, though.

  There are two alternatives including other architectures:

  * We can create a new clocksource that abstracts the per-architecture
    differences, and use this clocksource as basis for
    Xenomai. Essentially, this means mapping all desired
    non-x86-Clocksources to the interface offered in this patch.
    This requires more changes in the ipipe layer than variant B, namely,
  * We can create a union in struct xnvdso of all arch-specific clock
    datasets and introduce feature flags like XNVDSO_FEAT_HOSTRT_X86,
    XNVDSO_FEAT_HOSTRT_WHATEVER. The reader-side code then needs to
    match the data provided, which requires more changes on the
    Xenomai side.

- The series supports dynamic switching from a non-TSC clocksource to
  clocksource_tsc, but does not handle the other direction. This is
  intentional, because when a user wants meaningful synchronised time
  stamps, the reliability of the underlying clock should be ensured.

  Naturally, it would be possible to detect this change at the kernel
  level, but this would make the ipipe part more intrusive.

The code compiles with all combinations of IPIPE_HOSTRT and
OPT_XENO_HOSTRT.

As always, thanks for your comments and review!

Best, Wolfgang

---
 arch/x86/Kconfig              |    1
 arch/x86/include/asm/tsc.h    |    1
 arch/x86/kernel/tsc.c         |    4 +--
 arch/x86/kernel/vsyscall_64.c |    6 +++++
 include/linux/ipipe_tickdev.h |   25 +++++++++++++++++++++++
 kernel/ipipe/Kconfig          |    3 ++
 kernel/ipipe/core.c           |   44 ++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 82 insertions(+), 2 deletions(-)

[Adeos-main] [PATCH 1/2] ipipe: Pass NTP-corrected time information from Linux to higher domains

[Wolfgang Mauerer]

Introduce a mechanism to pass all information
required to implement CLOCK_HOST_REALTIME for
clock_gettime with NTP corrections as delivered by the
Linux domain to other domains. Essentially, this is an
equivalent of the Linux vsyscall to perform this very action.

We need to ensure that updates of the timing information are
atomic wrt. non-Linux domains by turning of hw interrupts.
A spinlock protects against concurrent writers, for example
do_settimeofday.

The timing information is only updated from a single CPU
in the Linux domain. When a reader on another CPU runs in
parallel with an update on CPU 0, an inconsistent state can
arise. This is mended by using a sequence counter to ensure
that the data structure was not modified during the read, which
also allows for lock-less access from userland.

Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
---
 include/linux/ipipe_tickdev.h |   25 +++++++++++++++++++++++
 kernel/ipipe/Kconfig          |    3 ++
 kernel/ipipe/core.c           |   44 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 0 deletions(-)

diff --git a/include/linux/ipipe_tickdev.h b/include/linux/ipipe_tickdev.h
index 4a1cb1b..86f13e0 100644
--- a/include/linux/ipipe_tickdev.h
+++ b/include/linux/ipipe_tickdev.h
@@ -25,6 +25,7 @@
 #if defined(CONFIG_IPIPE) && defined(CONFIG_GENERIC_CLOCKEVENTS)
 
 #include <linux/clockchips.h>
+#include <linux/clocksource.h>
 
 struct tick_device;
 
@@ -44,6 +45,30 @@ struct ipipe_tick_device {
 	int real_shift;
 };
 
+/*
+ * NOTE: When modifying this structure, make sure to keep the Xenomai
+ * definition include/nucleus/vdso.h in synch.
+ */
+struct ipipe_hostrt_data {
+	short live;
+	seqcount_t seqcount;
+	time_t wall_time_sec;
+	u32 wall_time_nsec;
+	struct timespec wall_to_monotonic;
+	cycle_t cycle_last;
+	cycle_t mask;
+	u32 mult;
+	u32 shift;
+};
+
+#ifdef CONFIG_IPIPE_HOSTRT
+void ipipe_set_hostrt_data(struct ipipe_hostrt_data *exchg);
+void update_ipipe_hostrt(struct timespec *wall_time, struct clocksource *clock);
+#else /* !CONFIG_IPIPE_HOSTRT */
+static inline void ipipe_set_hostrt_data(struct ipipe_hostrt_data *exchg) { };
+static inline void update_ipipe_hostrt(struct timespec *wall_time, struct clocksource *clock) {};
+#endif
+
 int ipipe_request_tickdev(const char *devname,
 			  void (*emumode)(enum clock_event_mode mode,
 					  struct clock_event_device *cdev),
diff --git a/kernel/ipipe/Kconfig b/kernel/ipipe/Kconfig
index de5e6a3..0849493 100644
--- a/kernel/ipipe/Kconfig
+++ b/kernel/ipipe/Kconfig
@@ -33,3 +33,6 @@ config IPIPE_UNMASKED_CONTEXT_SWITCH
        bool
        depends on IPIPE
        default n
+
+config IPIPE_HOSTRT
+       bool
diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
index 63deaf9..9b21db9 100644
--- a/kernel/ipipe/core.c
+++ b/kernel/ipipe/core.c
@@ -48,6 +48,49 @@ static unsigned long __ipipe_domain_slot_map;
 
 struct ipipe_domain ipipe_root;
 
+#ifdef CONFIG_IPIPE_HOSTRT
+static struct ipipe_hostrt_data *hostrt_data = NULL;
+
+void ipipe_set_hostrt_data(struct ipipe_hostrt_data *exchg)
+{
+	hostrt_data = exchg;
+	hostrt_data->live = 0;
+}
+
+static IPIPE_DEFINE_SPINLOCK(__ipipe_hostrtlock);
+/*
+ * NOTE: The architecture specific code must only call this function
+ * when a clocksource suitable for CLOCK_HOST_REALTIME is enabled.
+ */
+void update_ipipe_hostrt(struct timespec *wall_time, struct clocksource *clock)
+{
+	unsigned long flags;
+
+	if (unlikely(!hostrt_data))
+		return;
+
+	/*
+	 * The ipipe spinlock protects against Xenomai-side readers on the
+	 * same CPU, the sequence counter is for lockless access from
+	 * different CPUs.
+	 */
+	spin_lock_irqsave(&__ipipe_hostrtlock, flags);
+	write_seqcount_begin(&hostrt_data->seqcount);
+
+	hostrt_data->live = 1;
+	hostrt_data->cycle_last = clock->cycle_last;
+	hostrt_data->mask = clock->mask;
+	hostrt_data->mult = clock->mult;
+	hostrt_data->shift = clock->shift;
+	hostrt_data->wall_time_sec = wall_time->tv_sec;
+	hostrt_data->wall_time_nsec = wall_time->tv_nsec;
+	hostrt_data->wall_to_monotonic = wall_to_monotonic;
+
+	write_seqcount_end(&hostrt_data->seqcount);
+	spin_unlock_irqrestore(&__ipipe_hostrtlock, flags);
+};
+#endif /* CONFIG_IPIPE_HOSTRT */
+
 #ifndef CONFIG_SMP
 /*
  * Create an alias to the unique root status, so that arch-dep code
@@ -1937,6 +1980,7 @@ EXPORT_SYMBOL(ipipe_free_ptdkey);
 EXPORT_SYMBOL(ipipe_set_ptd);
 EXPORT_SYMBOL(ipipe_get_ptd);
 EXPORT_SYMBOL(ipipe_set_irq_affinity);
+EXPORT_SYMBOL(ipipe_set_hostrt_data);
 EXPORT_SYMBOL(ipipe_send_ipi);
 EXPORT_SYMBOL(__ipipe_pend_irq);
 EXPORT_SYMBOL(__ipipe_set_irq_pending);
-- 
1.6.4

[Adeos-main] [PATCH 2/2] ipipe: CLOCK_HOST_REALTIME: x86-specific part

[Wolfgang Mauerer]

x86 specific mechanism to make NTP-corrected time information
available to non-Linux domains.

Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
---
 arch/x86/Kconfig              |    1 +
 arch/x86/include/asm/tsc.h    |    1 +
 arch/x86/kernel/tsc.c         |    4 ++--
 arch/x86/kernel/vsyscall_64.c |    6 ++++++
 4 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 89b72a9..4ab6f49 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -50,6 +50,7 @@ config X86
 	select HAVE_KERNEL_BZIP2
 	select HAVE_KERNEL_LZMA
 	select HAVE_ARCH_KMEMCHECK
+	select IPIPE_HOSTRT if IPIPE && GENERIC_CLOCKEVENTS
 
 config OUTPUT_FORMAT
 	string
diff --git a/arch/x86/include/asm/tsc.h b/arch/x86/include/asm/tsc.h
index c042729..aed7d66 100644
--- a/arch/x86/include/asm/tsc.h
+++ b/arch/x86/include/asm/tsc.h
@@ -14,6 +14,7 @@
  */
 typedef unsigned long long cycles_t;
 
+extern struct clocksource clocksource_tsc;
 extern unsigned int cpu_khz;
 extern unsigned int tsc_khz;
 
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 597683a..5e298a2 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -699,7 +699,7 @@ core_initcall(cpufreq_tsc);
 
 /* clocksource code */
 
-static struct clocksource clocksource_tsc;
+struct clocksource clocksource_tsc;
 
 /*
  * We compare the TSC to the cycle_last value in the clocksource
@@ -745,7 +745,7 @@ static void resume_tsc(void)
 	clocksource_tsc.cycle_last = 0;
 }
 
-static struct clocksource clocksource_tsc = {
+struct clocksource clocksource_tsc = {
 	.name                   = "tsc",
 	.rating                 = 300,
 	.read                   = read_tsc,
diff --git a/arch/x86/kernel/vsyscall_64.c b/arch/x86/kernel/vsyscall_64.c
index 8cb4974..f1b90f7 100644
--- a/arch/x86/kernel/vsyscall_64.c
+++ b/arch/x86/kernel/vsyscall_64.c
@@ -32,6 +32,7 @@
 #include <linux/cpu.h>
 #include <linux/smp.h>
 #include <linux/notifier.h>
+#include <linux/ipipe_tickdev.h>
 
 #include <asm/vsyscall.h>
 #include <asm/pgtable.h>
@@ -89,6 +90,11 @@ void update_vsyscall(struct timespec *wall_time, struct clocksource *clock)
 	vsyscall_gtod_data.wall_to_monotonic = wall_to_monotonic;
 	vsyscall_gtod_data.wall_time_coarse = __current_kernel_time();
 	write_sequnlock_irqrestore(&vsyscall_gtod_data.lock, flags);
+
+#ifdef CONFIG_IPIPE_HOSTRT
+	if (clock == &clocksource_tsc)
+		update_ipipe_hostrt(wall_time, clock);
+#endif
 }
 
 /* RED-PEN may want to readd seq locking, but then the variable should be
-- 
1.6.4

Re: [Adeos-main] [PATCH 2/2] ipipe: CLOCK_HOST_REALTIME: x86-specific part

[Gilles Chanteperdrix]

Wolfgang Mauerer wrote:
> diff --git a/arch/x86/kernel/vsyscall_64.c b/arch/x86/kernel/vsyscall_64.c
> index 8cb4974..f1b90f7 100644
> --- a/arch/x86/kernel/vsyscall_64.c
> +++ b/arch/x86/kernel/vsyscall_64.c
> @@ -32,6 +32,7 @@
>  #include <linux/cpu.h>
>  #include <linux/smp.h>
>  #include <linux/notifier.h>
> +#include <linux/ipipe_tickdev.h>
>  
>  #include <asm/vsyscall.h>
>  #include <asm/pgtable.h>
> @@ -89,6 +90,11 @@ void update_vsyscall(struct timespec *wall_time, struct clocksource *clock)
>  	vsyscall_gtod_data.wall_to_monotonic = wall_to_monotonic;
>  	vsyscall_gtod_data.wall_time_coarse = __current_kernel_time();
>  	write_sequnlock_irqrestore(&vsyscall_gtod_data.lock, flags);
> +
> +#ifdef CONFIG_IPIPE_HOSTRT
> +	if (clock == &clocksource_tsc)
> +		update_ipipe_hostrt(wall_time, clock);
> +#endif

The idea was to use ipipe_dispatch_event here, and implement the event
handler in Xenomai. I wonder, however, if we should not do this at the
call sites ov update_vsyscall, or wrap update_vsyscall, so that we can
modify only architecture-independent code.

-- 
					    Gilles.

Re: [Adeos-main] [PATCH 1/2] ipipe: Pass NTP-corrected time information from Linux to higher domains

[Philippe Gerum]

On Fri, 2010-07-02 at 13:50 +0200, Wolfgang Mauerer wrote:

<snip>

> diff --git a/include/linux/ipipe_tickdev.h b/include/linux/ipipe_tickdev.h
> index 4a1cb1b..86f13e0 100644
> --- a/include/linux/ipipe_tickdev.h
> +++ b/include/linux/ipipe_tickdev.h
> @@ -25,6 +25,7 @@
>  #if defined(CONFIG_IPIPE) && defined(CONFIG_GENERIC_CLOCKEVENTS)

Since we should have CONFIG_HAVE_IPIPE_HOSTRT by now, let's use it.

>  
>  #include <linux/clockchips.h>
> +#include <linux/clocksource.h>
>  
>  struct tick_device;
>  
> @@ -44,6 +45,30 @@ struct ipipe_tick_device {
>  	int real_shift;
>  };
>  
> +/*
> + * NOTE: When modifying this structure, make sure to keep the Xenomai
> + * definition include/nucleus/vdso.h in synch.
> + */
> +struct ipipe_hostrt_data {
> +	short live;
> +	seqcount_t seqcount;
> +	time_t wall_time_sec;
> +	u32 wall_time_nsec;
> +	struct timespec wall_to_monotonic;
> +	cycle_t cycle_last;
> +	cycle_t mask;
> +	u32 mult;
> +	u32 shift;
> +};
> +
> +#ifdef CONFIG_IPIPE_HOSTRT
> +void ipipe_set_hostrt_data(struct ipipe_hostrt_data *exchg);
> +void update_ipipe_hostrt(struct timespec *wall_time, struct clocksource *clock);

Naming consistency -> ipipe_update_hostrt()

We should probably mark this as an internal interface as well, e.g.
__ipipe_update_hostrt.

> +#else /* !CONFIG_IPIPE_HOSTRT */
> +static inline void ipipe_set_hostrt_data(struct ipipe_hostrt_data *exchg) { };
> +static inline void update_ipipe_hostrt(struct timespec *wall_time, struct clocksource *clock) {};
> +#endif
> +

<snip>

> diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
> index 63deaf9..9b21db9 100644
> --- a/kernel/ipipe/core.c
> +++ b/kernel/ipipe/core.c
> @@ -48,6 +48,49 @@ static unsigned long __ipipe_domain_slot_map;
>  
>  struct ipipe_domain ipipe_root;
>  
> +#ifdef CONFIG_IPIPE_HOSTRT

Same remark as previously regarding the use of CONFIG_HAVE_IPIPE_HOSTRT
in noarch code. In case the arch-dep code does support the feature, we
want the noarch section to always compile the generic bits. There is no
incentive for overcomplicated and error prone feature selection, only
for saving a few hundred bytes, most - if not all - people will want
anyway.

-- 
Philippe.

Re: [Adeos-main] [PATCH 1/2] ipipe: Pass NTP-corrected time information from Linux to higher domains

[Jan Kiszka]

Philippe Gerum wrote:
> On Fri, 2010-07-02 at 13:50 +0200, Wolfgang Mauerer wrote:
> 
> <snip>
> 
>> diff --git a/include/linux/ipipe_tickdev.h b/include/linux/ipipe_tickdev.h
>> index 4a1cb1b..86f13e0 100644
>> --- a/include/linux/ipipe_tickdev.h
>> +++ b/include/linux/ipipe_tickdev.h
>> @@ -25,6 +25,7 @@
>>  #if defined(CONFIG_IPIPE) && defined(CONFIG_GENERIC_CLOCKEVENTS)
> 
> Since we should have CONFIG_HAVE_IPIPE_HOSTRT by now, let's use it.

Don't get yet how this fits here.

> 
>>  
>>  #include <linux/clockchips.h>
>> +#include <linux/clocksource.h>
>>  
>>  struct tick_device;
>>  
>> @@ -44,6 +45,30 @@ struct ipipe_tick_device {
>>  	int real_shift;
>>  };
>>  
>> +/*
>> + * NOTE: When modifying this structure, make sure to keep the Xenomai
>> + * definition include/nucleus/vdso.h in synch.
>> + */
>> +struct ipipe_hostrt_data {
>> +	short live;
>> +	seqcount_t seqcount;
>> +	time_t wall_time_sec;
>> +	u32 wall_time_nsec;
>> +	struct timespec wall_to_monotonic;
>> +	cycle_t cycle_last;
>> +	cycle_t mask;
>> +	u32 mult;
>> +	u32 shift;
>> +};
>> +
>> +#ifdef CONFIG_IPIPE_HOSTRT
>> +void ipipe_set_hostrt_data(struct ipipe_hostrt_data *exchg);
>> +void update_ipipe_hostrt(struct timespec *wall_time, struct clocksource *clock);
> 
> Naming consistency -> ipipe_update_hostrt()
> 
> We should probably mark this as an internal interface as well, e.g.
> __ipipe_update_hostrt.

Will fix.

> 
>> +#else /* !CONFIG_IPIPE_HOSTRT */
>> +static inline void ipipe_set_hostrt_data(struct ipipe_hostrt_data *exchg) { };
>> +static inline void update_ipipe_hostrt(struct timespec *wall_time, struct clocksource *clock) {};
>> +#endif
>> +
> 
> <snip>
> 
>> diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
>> index 63deaf9..9b21db9 100644
>> --- a/kernel/ipipe/core.c
>> +++ b/kernel/ipipe/core.c
>> @@ -48,6 +48,49 @@ static unsigned long __ipipe_domain_slot_map;
>>  
>>  struct ipipe_domain ipipe_root;
>>  
>> +#ifdef CONFIG_IPIPE_HOSTRT
> 
> Same remark as previously regarding the use of CONFIG_HAVE_IPIPE_HOSTRT
> in noarch code. In case the arch-dep code does support the feature, we
> want the noarch section to always compile the generic bits. There is no
> incentive for overcomplicated and error prone feature selection, only
> for saving a few hundred bytes, most - if not all - people will want
> anyway.

We are following kernel-style here: arch selects HAVE_*, generic code
enables some feature depending on HAVE_* and further dependencies (here:
CONFIG_IPIPE).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Adeos-main] [PATCH 2/2] ipipe: CLOCK_HOST_REALTIME: x86-specific part

[Philippe Gerum]

On Fri, 2010-07-02 at 13:50 +0200, Wolfgang Mauerer wrote:
> x86 specific mechanism to make NTP-corrected time information
> available to non-Linux domains.
> 
> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> ---
>  arch/x86/Kconfig              |    1 +
>  arch/x86/include/asm/tsc.h    |    1 +
>  arch/x86/kernel/tsc.c         |    4 ++--
>  arch/x86/kernel/vsyscall_64.c |    6 ++++++
>  4 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 89b72a9..4ab6f49 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -50,6 +50,7 @@ config X86
>  	select HAVE_KERNEL_BZIP2
>  	select HAVE_KERNEL_LZMA
>  	select HAVE_ARCH_KMEMCHECK
> +	select IPIPE_HOSTRT if IPIPE && GENERIC_CLOCKEVENTS

The selection logic is rather to define HAVE_IPIPE_HOSTRT
unconditionally for x86, so that the noarch section could provide the
required bits. CONFIG_IPIPE_HOSTRT as a mean to deselect the feature for
the architecture seems overkill. As explained earlier, if we have it for
the arch, then we want it in.

GENERIC_CLOCKEVENTS is def_bool y in Kconfig for x86, so we don't really
need to depend on it.

>  
>  config OUTPUT_FORMAT
>  	string
> diff --git a/arch/x86/include/asm/tsc.h b/arch/x86/include/asm/tsc.h
> index c042729..aed7d66 100644
> --- a/arch/x86/include/asm/tsc.h
> +++ b/arch/x86/include/asm/tsc.h
> @@ -14,6 +14,7 @@
>   */
>  typedef unsigned long long cycles_t;
>  
> +extern struct clocksource clocksource_tsc;
>  extern unsigned int cpu_khz;
>  extern unsigned int tsc_khz;
>  
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index 597683a..5e298a2 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -699,7 +699,7 @@ core_initcall(cpufreq_tsc);
>  
>  /* clocksource code */
>  
> -static struct clocksource clocksource_tsc;
> +struct clocksource clocksource_tsc;
>  
>  /*
>   * We compare the TSC to the cycle_last value in the clocksource
> @@ -745,7 +745,7 @@ static void resume_tsc(void)
>  	clocksource_tsc.cycle_last = 0;
>  }
>  
> -static struct clocksource clocksource_tsc = {
> +struct clocksource clocksource_tsc = {
>  	.name                   = "tsc",
>  	.rating                 = 300,
>  	.read                   = read_tsc,
> diff --git a/arch/x86/kernel/vsyscall_64.c b/arch/x86/kernel/vsyscall_64.c
> index 8cb4974..f1b90f7 100644
> --- a/arch/x86/kernel/vsyscall_64.c
> +++ b/arch/x86/kernel/vsyscall_64.c
> @@ -32,6 +32,7 @@
>  #include <linux/cpu.h>
>  #include <linux/smp.h>
>  #include <linux/notifier.h>
> +#include <linux/ipipe_tickdev.h>
>  
>  #include <asm/vsyscall.h>
>  #include <asm/pgtable.h>
> @@ -89,6 +90,11 @@ void update_vsyscall(struct timespec *wall_time, struct clocksource *clock)
>  	vsyscall_gtod_data.wall_to_monotonic = wall_to_monotonic;
>  	vsyscall_gtod_data.wall_time_coarse = __current_kernel_time();
>  	write_sequnlock_irqrestore(&vsyscall_gtod_data.lock, flags);
> +
> +#ifdef CONFIG_IPIPE_HOSTRT
> +	if (clock == &clocksource_tsc)
> +		update_ipipe_hostrt(wall_time, clock);
> +#endif
>  }

It would be nice to move the test on clock to ipipe_update_hostrt() in
order to hide all nitty-gritty details from mainline. Making the code
unconditionally compiled in removes the need to #ifdefery.

>  
>  /* RED-PEN may want to readd seq locking, but then the variable should be

-- 
Philippe.

Re: [Adeos-main] [PATCH 1/2] ipipe: Pass NTP-corrected time information from Linux to higher domains

[Philippe Gerum]

On Wed, 2010-08-25 at 10:50 +0200, Jan Kiszka wrote:
> Philippe Gerum wrote:
> > On Fri, 2010-07-02 at 13:50 +0200, Wolfgang Mauerer wrote:
> > 
> > <snip>
> > 
> >> diff --git a/include/linux/ipipe_tickdev.h b/include/linux/ipipe_tickdev.h
> >> index 4a1cb1b..86f13e0 100644
> >> --- a/include/linux/ipipe_tickdev.h
> >> +++ b/include/linux/ipipe_tickdev.h
> >> @@ -25,6 +25,7 @@
> >>  #if defined(CONFIG_IPIPE) && defined(CONFIG_GENERIC_CLOCKEVENTS)
> > 
> > Since we should have CONFIG_HAVE_IPIPE_HOSTRT by now, let's use it.
> 
> Don't get yet how this fits here.

arch-dep would define CONFIG_HAVE_IPIPE_HOSTRT [if IPIPE]

> >>  
> >>  #include <linux/clockchips.h>
> >> +#include <linux/clocksource.h>
> >>  
> >>  struct tick_device;
> >>  
> >> @@ -44,6 +45,30 @@ struct ipipe_tick_device {
> >>  	int real_shift;
> >>  };
> >>  
> >> +/*
> >> + * NOTE: When modifying this structure, make sure to keep the Xenomai
> >> + * definition include/nucleus/vdso.h in synch.
> >> + */
> >> +struct ipipe_hostrt_data {
> >> +	short live;
> >> +	seqcount_t seqcount;
> >> +	time_t wall_time_sec;
> >> +	u32 wall_time_nsec;
> >> +	struct timespec wall_to_monotonic;
> >> +	cycle_t cycle_last;
> >> +	cycle_t mask;
> >> +	u32 mult;
> >> +	u32 shift;
> >> +};
> >> +
> >> +#ifdef CONFIG_IPIPE_HOSTRT
> >> +void ipipe_set_hostrt_data(struct ipipe_hostrt_data *exchg);
> >> +void update_ipipe_hostrt(struct timespec *wall_time, struct clocksource *clock);
> > 
> > Naming consistency -> ipipe_update_hostrt()
> > 
> > We should probably mark this as an internal interface as well, e.g.
> > __ipipe_update_hostrt.
> 
> Will fix.
> 
> > 
> >> +#else /* !CONFIG_IPIPE_HOSTRT */
> >> +static inline void ipipe_set_hostrt_data(struct ipipe_hostrt_data *exchg) { };
> >> +static inline void update_ipipe_hostrt(struct timespec *wall_time, struct clocksource *clock) {};
> >> +#endif
> >> +
> > 
> > <snip>
> > 
> >> diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
> >> index 63deaf9..9b21db9 100644
> >> --- a/kernel/ipipe/core.c
> >> +++ b/kernel/ipipe/core.c
> >> @@ -48,6 +48,49 @@ static unsigned long __ipipe_domain_slot_map;
> >>  
> >>  struct ipipe_domain ipipe_root;
> >>  
> >> +#ifdef CONFIG_IPIPE_HOSTRT
> > 
> > Same remark as previously regarding the use of CONFIG_HAVE_IPIPE_HOSTRT
> > in noarch code. In case the arch-dep code does support the feature, we
> > want the noarch section to always compile the generic bits. There is no
> > incentive for overcomplicated and error prone feature selection, only
> > for saving a few hundred bytes, most - if not all - people will want
> > anyway.
> 
> We are following kernel-style here: arch selects HAVE_*, generic code
> enables some feature depending on HAVE_* and further dependencies (here:
> CONFIG_IPIPE).
> 
> Jan
> 

-- 
Philippe.

Re: [Adeos-main] [PATCH 2/2] ipipe: CLOCK_HOST_REALTIME: x86-specific part

[Jan Kiszka]

Philippe Gerum wrote:
> On Fri, 2010-07-02 at 13:50 +0200, Wolfgang Mauerer wrote:
>> x86 specific mechanism to make NTP-corrected time information
>> available to non-Linux domains.
>>
>> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>> ---
>>  arch/x86/Kconfig              |    1 +
>>  arch/x86/include/asm/tsc.h    |    1 +
>>  arch/x86/kernel/tsc.c         |    4 ++--
>>  arch/x86/kernel/vsyscall_64.c |    6 ++++++
>>  4 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index 89b72a9..4ab6f49 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -50,6 +50,7 @@ config X86
>>  	select HAVE_KERNEL_BZIP2
>>  	select HAVE_KERNEL_LZMA
>>  	select HAVE_ARCH_KMEMCHECK
>> +	select IPIPE_HOSTRT if IPIPE && GENERIC_CLOCKEVENTS
> 
> The selection logic is rather to define HAVE_IPIPE_HOSTRT
> unconditionally for x86, so that the noarch section could provide the
> required bits. CONFIG_IPIPE_HOSTRT as a mean to deselect the feature for
> the architecture seems overkill. As explained earlier, if we have it for
> the arch, then we want it in.
> 
> GENERIC_CLOCKEVENTS is def_bool y in Kconfig for x86, so we don't really
> need to depend on it.

You are looking at an older version of this patch, v2 only contains
"select HAVE_IPIPE_HOSTRT".

> 
>>  
>>  config OUTPUT_FORMAT
>>  	string
>> diff --git a/arch/x86/include/asm/tsc.h b/arch/x86/include/asm/tsc.h
>> index c042729..aed7d66 100644
>> --- a/arch/x86/include/asm/tsc.h
>> +++ b/arch/x86/include/asm/tsc.h
>> @@ -14,6 +14,7 @@
>>   */
>>  typedef unsigned long long cycles_t;
>>  
>> +extern struct clocksource clocksource_tsc;
>>  extern unsigned int cpu_khz;
>>  extern unsigned int tsc_khz;
>>  
>> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
>> index 597683a..5e298a2 100644
>> --- a/arch/x86/kernel/tsc.c
>> +++ b/arch/x86/kernel/tsc.c
>> @@ -699,7 +699,7 @@ core_initcall(cpufreq_tsc);
>>  
>>  /* clocksource code */
>>  
>> -static struct clocksource clocksource_tsc;
>> +struct clocksource clocksource_tsc;
>>  
>>  /*
>>   * We compare the TSC to the cycle_last value in the clocksource
>> @@ -745,7 +745,7 @@ static void resume_tsc(void)
>>  	clocksource_tsc.cycle_last = 0;
>>  }
>>  
>> -static struct clocksource clocksource_tsc = {
>> +struct clocksource clocksource_tsc = {
>>  	.name                   = "tsc",
>>  	.rating                 = 300,
>>  	.read                   = read_tsc,
>> diff --git a/arch/x86/kernel/vsyscall_64.c b/arch/x86/kernel/vsyscall_64.c
>> index 8cb4974..f1b90f7 100644
>> --- a/arch/x86/kernel/vsyscall_64.c
>> +++ b/arch/x86/kernel/vsyscall_64.c
>> @@ -32,6 +32,7 @@
>>  #include <linux/cpu.h>
>>  #include <linux/smp.h>
>>  #include <linux/notifier.h>
>> +#include <linux/ipipe_tickdev.h>
>>  
>>  #include <asm/vsyscall.h>
>>  #include <asm/pgtable.h>
>> @@ -89,6 +90,11 @@ void update_vsyscall(struct timespec *wall_time, struct clocksource *clock)
>>  	vsyscall_gtod_data.wall_to_monotonic = wall_to_monotonic;
>>  	vsyscall_gtod_data.wall_time_coarse = __current_kernel_time();
>>  	write_sequnlock_irqrestore(&vsyscall_gtod_data.lock, flags);
>> +
>> +#ifdef CONFIG_IPIPE_HOSTRT
>> +	if (clock == &clocksource_tsc)
>> +		update_ipipe_hostrt(wall_time, clock);
>> +#endif
>>  }
> 
> It would be nice to move the test on clock to ipipe_update_hostrt() in
> order to hide all nitty-gritty details from mainline. Making the code
> unconditionally compiled in removes the need to #ifdefery.

This code is arch-specific while update_ipipe_hostrt is generic.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Adeos-main] [PATCH 1/2] ipipe: Pass NTP-corrected time information from Linux to higher domains

[Jan Kiszka]

Philippe Gerum wrote:
> On Wed, 2010-08-25 at 10:50 +0200, Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> On Fri, 2010-07-02 at 13:50 +0200, Wolfgang Mauerer wrote:
>>>
>>> <snip>
>>>
>>>> diff --git a/include/linux/ipipe_tickdev.h b/include/linux/ipipe_tickdev.h
>>>> index 4a1cb1b..86f13e0 100644
>>>> --- a/include/linux/ipipe_tickdev.h
>>>> +++ b/include/linux/ipipe_tickdev.h
>>>> @@ -25,6 +25,7 @@
>>>>  #if defined(CONFIG_IPIPE) && defined(CONFIG_GENERIC_CLOCKEVENTS)
>>> Since we should have CONFIG_HAVE_IPIPE_HOSTRT by now, let's use it.
>> Don't get yet how this fits here.
> 
> arch-dep would define CONFIG_HAVE_IPIPE_HOSTRT [if IPIPE]
> 

Still don't see the relation to the line you cited above.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Adeos-main] [PATCH 1/2] ipipe: Pass NTP-corrected time information from Linux to higher domains

[Philippe Gerum]

On Wed, 2010-08-25 at 10:58 +0200, Jan Kiszka wrote:
> Philippe Gerum wrote:
> > On Wed, 2010-08-25 at 10:50 +0200, Jan Kiszka wrote:
> >> Philippe Gerum wrote:
> >>> On Fri, 2010-07-02 at 13:50 +0200, Wolfgang Mauerer wrote:
> >>>
> >>> <snip>
> >>>
> >>>> diff --git a/include/linux/ipipe_tickdev.h b/include/linux/ipipe_tickdev.h
> >>>> index 4a1cb1b..86f13e0 100644
> >>>> --- a/include/linux/ipipe_tickdev.h
> >>>> +++ b/include/linux/ipipe_tickdev.h
> >>>> @@ -25,6 +25,7 @@
> >>>>  #if defined(CONFIG_IPIPE) && defined(CONFIG_GENERIC_CLOCKEVENTS)
> >>> Since we should have CONFIG_HAVE_IPIPE_HOSTRT by now, let's use it.
> >> Don't get yet how this fits here.
> > 
> > arch-dep would define CONFIG_HAVE_IPIPE_HOSTRT [if IPIPE]
> > 
> 
> Still don't see the relation to the line you cited above.
> 

That is because you chose to have CONFIG_IPIPE_HOSTRT and
CONFIG_HAVE_IPIPE_HOSTRT. I would have only defined the latter, the way
you define the former. I'm looking for the hostrt support to be compiled
in if CONFIG_HAVE_IPIPE_HOSTRT is available from the arch-dep section,
so we don't need CONFIG_IPIPE_HOSTRT. Generic bits may depend on HAVE_*
as well.

> Jan
> 

-- 
Philippe.

Re: [Adeos-main] [PATCH 1/2] ipipe: Pass NTP-corrected time information from Linux to higher domains

[Jan Kiszka]

Philippe Gerum wrote:
> On Wed, 2010-08-25 at 10:58 +0200, Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> On Wed, 2010-08-25 at 10:50 +0200, Jan Kiszka wrote:
>>>> Philippe Gerum wrote:
>>>>> On Fri, 2010-07-02 at 13:50 +0200, Wolfgang Mauerer wrote:
>>>>>
>>>>> <snip>
>>>>>
>>>>>> diff --git a/include/linux/ipipe_tickdev.h b/include/linux/ipipe_tickdev.h
>>>>>> index 4a1cb1b..86f13e0 100644
>>>>>> --- a/include/linux/ipipe_tickdev.h
>>>>>> +++ b/include/linux/ipipe_tickdev.h
>>>>>> @@ -25,6 +25,7 @@
>>>>>>  #if defined(CONFIG_IPIPE) && defined(CONFIG_GENERIC_CLOCKEVENTS)
>>>>> Since we should have CONFIG_HAVE_IPIPE_HOSTRT by now, let's use it.
>>>> Don't get yet how this fits here.
>>> arch-dep would define CONFIG_HAVE_IPIPE_HOSTRT [if IPIPE]
>>>
>> Still don't see the relation to the line you cited above.
>>
> 
> That is because you chose to have CONFIG_IPIPE_HOSTRT and
> CONFIG_HAVE_IPIPE_HOSTRT. I would have only defined the latter, the way
> you define the former. I'm looking for the hostrt support to be compiled
> in if CONFIG_HAVE_IPIPE_HOSTRT is available from the arch-dep section,
> so we don't need CONFIG_IPIPE_HOSTRT. Generic bits may depend on HAVE_*
> as well.

First of all, the code you cited _above_ is not changed by our patches,
so the context still puzzles me (but maybe you are referring to some
other place in fact).

Second, CONFIG_HAVE_IPIPE_HOSTRT is designed to be set independently of
CONFIG_IPIPE - it's a static arch feature like all the other
CONFIG_HAVE_* in arch/*/Kconfig. So it takes a second, generically
defined CONFIG switch if the generic support also depends on
CONFIG_IPIPE like in this case. That's a kernel convention we follow. If
you want us to do it I-pipe-specific, no problem, I just want to have
this pointed out.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Adeos-main] [PATCH 2/2] ipipe: CLOCK_HOST_REALTIME: x86-specific part

[Philippe Gerum]

On Wed, 2010-08-25 at 10:58 +0200, Jan Kiszka wrote:
> Philippe Gerum wrote:
> > On Fri, 2010-07-02 at 13:50 +0200, Wolfgang Mauerer wrote:
> >> x86 specific mechanism to make NTP-corrected time information
> >> available to non-Linux domains.
> >>
> >> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> >> ---
> >>  arch/x86/Kconfig              |    1 +
> >>  arch/x86/include/asm/tsc.h    |    1 +
> >>  arch/x86/kernel/tsc.c         |    4 ++--
> >>  arch/x86/kernel/vsyscall_64.c |    6 ++++++
> >>  4 files changed, 10 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> >> index 89b72a9..4ab6f49 100644
> >> --- a/arch/x86/Kconfig
> >> +++ b/arch/x86/Kconfig
> >> @@ -50,6 +50,7 @@ config X86
> >>  	select HAVE_KERNEL_BZIP2
> >>  	select HAVE_KERNEL_LZMA
> >>  	select HAVE_ARCH_KMEMCHECK
> >> +	select IPIPE_HOSTRT if IPIPE && GENERIC_CLOCKEVENTS
> > 
> > The selection logic is rather to define HAVE_IPIPE_HOSTRT
> > unconditionally for x86, so that the noarch section could provide the
> > required bits. CONFIG_IPIPE_HOSTRT as a mean to deselect the feature for
> > the architecture seems overkill. As explained earlier, if we have it for
> > the arch, then we want it in.
> > 
> > GENERIC_CLOCKEVENTS is def_bool y in Kconfig for x86, so we don't really
> > need to depend on it.
> 
> You are looking at an older version of this patch, v2 only contains
> "select HAVE_IPIPE_HOSTRT".

True, sorry.

> 
> > 
> >>  
> >>  config OUTPUT_FORMAT
> >>  	string
> >> diff --git a/arch/x86/include/asm/tsc.h b/arch/x86/include/asm/tsc.h
> >> index c042729..aed7d66 100644
> >> --- a/arch/x86/include/asm/tsc.h
> >> +++ b/arch/x86/include/asm/tsc.h
> >> @@ -14,6 +14,7 @@
> >>   */
> >>  typedef unsigned long long cycles_t;
> >>  
> >> +extern struct clocksource clocksource_tsc;
> >>  extern unsigned int cpu_khz;
> >>  extern unsigned int tsc_khz;
> >>  
> >> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> >> index 597683a..5e298a2 100644
> >> --- a/arch/x86/kernel/tsc.c
> >> +++ b/arch/x86/kernel/tsc.c
> >> @@ -699,7 +699,7 @@ core_initcall(cpufreq_tsc);
> >>  
> >>  /* clocksource code */
> >>  
> >> -static struct clocksource clocksource_tsc;
> >> +struct clocksource clocksource_tsc;
> >>  
> >>  /*
> >>   * We compare the TSC to the cycle_last value in the clocksource
> >> @@ -745,7 +745,7 @@ static void resume_tsc(void)
> >>  	clocksource_tsc.cycle_last = 0;
> >>  }
> >>  
> >> -static struct clocksource clocksource_tsc = {
> >> +struct clocksource clocksource_tsc = {
> >>  	.name                   = "tsc",
> >>  	.rating                 = 300,
> >>  	.read                   = read_tsc,
> >> diff --git a/arch/x86/kernel/vsyscall_64.c b/arch/x86/kernel/vsyscall_64.c
> >> index 8cb4974..f1b90f7 100644
> >> --- a/arch/x86/kernel/vsyscall_64.c
> >> +++ b/arch/x86/kernel/vsyscall_64.c
> >> @@ -32,6 +32,7 @@
> >>  #include <linux/cpu.h>
> >>  #include <linux/smp.h>
> >>  #include <linux/notifier.h>
> >> +#include <linux/ipipe_tickdev.h>
> >>  
> >>  #include <asm/vsyscall.h>
> >>  #include <asm/pgtable.h>
> >> @@ -89,6 +90,11 @@ void update_vsyscall(struct timespec *wall_time, struct clocksource *clock)
> >>  	vsyscall_gtod_data.wall_to_monotonic = wall_to_monotonic;
> >>  	vsyscall_gtod_data.wall_time_coarse = __current_kernel_time();
> >>  	write_sequnlock_irqrestore(&vsyscall_gtod_data.lock, flags);
> >> +
> >> +#ifdef CONFIG_IPIPE_HOSTRT
> >> +	if (clock == &clocksource_tsc)
> >> +		update_ipipe_hostrt(wall_time, clock);
> >> +#endif
> >>  }
> > 
> > It would be nice to move the test on clock to ipipe_update_hostrt() in
> > order to hide all nitty-gritty details from mainline. Making the code
> > unconditionally compiled in removes the need to #ifdefery.
> 
> This code is arch-specific while update_ipipe_hostrt is generic.
> 

We don't need #ifdefery with the conditional definition of
ipipe_update_hostrt() in ipipe_tickdev.h.

> Jan
> 

-- 
Philippe.

Re: [Adeos-main] [PATCH 1/2] ipipe: Pass NTP-corrected time information from Linux to higher domains

[Philippe Gerum]

On Wed, 2010-08-25 at 11:19 +0200, Jan Kiszka wrote:
> Philippe Gerum wrote:
> > On Wed, 2010-08-25 at 10:58 +0200, Jan Kiszka wrote:
> >> Philippe Gerum wrote:
> >>> On Wed, 2010-08-25 at 10:50 +0200, Jan Kiszka wrote:
> >>>> Philippe Gerum wrote:
> >>>>> On Fri, 2010-07-02 at 13:50 +0200, Wolfgang Mauerer wrote:
> >>>>>
> >>>>> <snip>
> >>>>>
> >>>>>> diff --git a/include/linux/ipipe_tickdev.h b/include/linux/ipipe_tickdev.h
> >>>>>> index 4a1cb1b..86f13e0 100644
> >>>>>> --- a/include/linux/ipipe_tickdev.h
> >>>>>> +++ b/include/linux/ipipe_tickdev.h
> >>>>>> @@ -25,6 +25,7 @@
> >>>>>>  #if defined(CONFIG_IPIPE) && defined(CONFIG_GENERIC_CLOCKEVENTS)
> >>>>> Since we should have CONFIG_HAVE_IPIPE_HOSTRT by now, let's use it.
> >>>> Don't get yet how this fits here.
> >>> arch-dep would define CONFIG_HAVE_IPIPE_HOSTRT [if IPIPE]
> >>>
> >> Still don't see the relation to the line you cited above.
> >>
> > 
> > That is because you chose to have CONFIG_IPIPE_HOSTRT and
> > CONFIG_HAVE_IPIPE_HOSTRT. I would have only defined the latter, the way
> > you define the former. I'm looking for the hostrt support to be compiled
> > in if CONFIG_HAVE_IPIPE_HOSTRT is available from the arch-dep section,
> > so we don't need CONFIG_IPIPE_HOSTRT. Generic bits may depend on HAVE_*
> > as well.
> 
> First of all, the code you cited _above_ is not changed by our patches,
> so the context still puzzles me (but maybe you are referring to some
> other place in fact).

Patch v2 says:

diff --git a/kernel/ipipe/Kconfig b/kernel/ipipe/Kconfig
index de5e6a3..bc7a00c 100644
--- a/kernel/ipipe/Kconfig
+++ b/kernel/ipipe/Kconfig
@@ -33,3 +33,10 @@ config IPIPE_UNMASKED_CONTEXT_SWITCH
        bool
        depends on IPIPE
        default n
+
+config HAVE_IPIPE_HOSTRT
+       bool
+
+config IPIPE_HOSTRT
+       def_bool y
+       depends on HAVE_IPIPE_HOSTRT && IPIPE

So what's your point?

> 
> Second, CONFIG_HAVE_IPIPE_HOSTRT is designed to be set independently of
> CONFIG_IPIPE - it's a static arch feature like all the other
> CONFIG_HAVE_* in arch/*/Kconfig. So it takes a second, generically
> defined CONFIG switch if the generic support also depends on
> CONFIG_IPIPE like in this case.

Which does not make any sense. We don't want to make this selectable at
all. Mainline has CONFIG_HAVE_SYSCALL_WRAPPERS for instance, and you
won't find any CONFIG_SYSCALL_WRAPPERS, because it makes no sense not to
use them when the architecture _have_ them. It goes exactly the same way
with hostrt.

>  That's a kernel convention we follow. If
> you want us to do it I-pipe-specific, no problem, I just want to have
> this pointed out.

This is no ipipe-specific. It's just sensible.

> 
> Jan
> 

-- 
Philippe.

Re: [Adeos-main] [PATCH 2/2] ipipe: CLOCK_HOST_REALTIME: x86-specific part

[Jan Kiszka]

Philippe Gerum wrote:
>>>> diff --git a/arch/x86/kernel/vsyscall_64.c b/arch/x86/kernel/vsyscall_64.c
>>>> index 8cb4974..f1b90f7 100644
>>>> --- a/arch/x86/kernel/vsyscall_64.c
>>>> +++ b/arch/x86/kernel/vsyscall_64.c
>>>> @@ -32,6 +32,7 @@
>>>>  #include <linux/cpu.h>
>>>>  #include <linux/smp.h>
>>>>  #include <linux/notifier.h>
>>>> +#include <linux/ipipe_tickdev.h>
>>>>  
>>>>  #include <asm/vsyscall.h>
>>>>  #include <asm/pgtable.h>
>>>> @@ -89,6 +90,11 @@ void update_vsyscall(struct timespec *wall_time, struct clocksource *clock)
>>>>  	vsyscall_gtod_data.wall_to_monotonic = wall_to_monotonic;
>>>>  	vsyscall_gtod_data.wall_time_coarse = __current_kernel_time();
>>>>  	write_sequnlock_irqrestore(&vsyscall_gtod_data.lock, flags);
>>>> +
>>>> +#ifdef CONFIG_IPIPE_HOSTRT
>>>> +	if (clock == &clocksource_tsc)
>>>> +		update_ipipe_hostrt(wall_time, clock);
>>>> +#endif
>>>>  }
>>> It would be nice to move the test on clock to ipipe_update_hostrt() in
>>> order to hide all nitty-gritty details from mainline. Making the code
>>> unconditionally compiled in removes the need to #ifdefery.
>> This code is arch-specific while update_ipipe_hostrt is generic.
>>
> 
> We don't need #ifdefery with the conditional definition of
> ipipe_update_hostrt() in ipipe_tickdev.h.

You mean rely on the compiler optimizing the check away? OK, now I get it.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Adeos-main] [PATCH 2/2] ipipe: CLOCK_HOST_REALTIME: x86-specific part

[Philippe Gerum]

On Wed, 2010-08-25 at 11:32 +0200, Jan Kiszka wrote:
> Philippe Gerum wrote:
> >>>> diff --git a/arch/x86/kernel/vsyscall_64.c b/arch/x86/kernel/vsyscall_64.c
> >>>> index 8cb4974..f1b90f7 100644
> >>>> --- a/arch/x86/kernel/vsyscall_64.c
> >>>> +++ b/arch/x86/kernel/vsyscall_64.c
> >>>> @@ -32,6 +32,7 @@
> >>>>  #include <linux/cpu.h>
> >>>>  #include <linux/smp.h>
> >>>>  #include <linux/notifier.h>
> >>>> +#include <linux/ipipe_tickdev.h>
> >>>>  
> >>>>  #include <asm/vsyscall.h>
> >>>>  #include <asm/pgtable.h>
> >>>> @@ -89,6 +90,11 @@ void update_vsyscall(struct timespec *wall_time, struct clocksource *clock)
> >>>>  	vsyscall_gtod_data.wall_to_monotonic = wall_to_monotonic;
> >>>>  	vsyscall_gtod_data.wall_time_coarse = __current_kernel_time();
> >>>>  	write_sequnlock_irqrestore(&vsyscall_gtod_data.lock, flags);
> >>>> +
> >>>> +#ifdef CONFIG_IPIPE_HOSTRT
> >>>> +	if (clock == &clocksource_tsc)
> >>>> +		update_ipipe_hostrt(wall_time, clock);
> >>>> +#endif
> >>>>  }
> >>> It would be nice to move the test on clock to ipipe_update_hostrt() in
> >>> order to hide all nitty-gritty details from mainline. Making the code
> >>> unconditionally compiled in removes the need to #ifdefery.
> >> This code is arch-specific while update_ipipe_hostrt is generic.
> >>
> > 
> > We don't need #ifdefery with the conditional definition of
> > ipipe_update_hostrt() in ipipe_tickdev.h.
> 
> You mean rely on the compiler optimizing the check away? OK, now I get it.

Yep.

> 
> Jan
> 

-- 
Philippe.

Re: [Adeos-main] [PATCH 1/2] ipipe: Pass NTP-corrected time information from Linux to higher domains

[Jan Kiszka]

Philippe Gerum wrote:
> On Wed, 2010-08-25 at 11:19 +0200, Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> On Wed, 2010-08-25 at 10:58 +0200, Jan Kiszka wrote:
>>>> Philippe Gerum wrote:
>>>>> On Wed, 2010-08-25 at 10:50 +0200, Jan Kiszka wrote:
>>>>>> Philippe Gerum wrote:
>>>>>>> On Fri, 2010-07-02 at 13:50 +0200, Wolfgang Mauerer wrote:
>>>>>>>
>>>>>>> <snip>
>>>>>>>
>>>>>>>> diff --git a/include/linux/ipipe_tickdev.h b/include/linux/ipipe_tickdev.h
>>>>>>>> index 4a1cb1b..86f13e0 100644
>>>>>>>> --- a/include/linux/ipipe_tickdev.h
>>>>>>>> +++ b/include/linux/ipipe_tickdev.h
>>>>>>>> @@ -25,6 +25,7 @@
>>>>>>>>  #if defined(CONFIG_IPIPE) && defined(CONFIG_GENERIC_CLOCKEVENTS)
>>>>>>> Since we should have CONFIG_HAVE_IPIPE_HOSTRT by now, let's use it.
>>>>>> Don't get yet how this fits here.
>>>>> arch-dep would define CONFIG_HAVE_IPIPE_HOSTRT [if IPIPE]
>>>>>
>>>> Still don't see the relation to the line you cited above.
>>>>
>>> That is because you chose to have CONFIG_IPIPE_HOSTRT and
>>> CONFIG_HAVE_IPIPE_HOSTRT. I would have only defined the latter, the way
>>> you define the former. I'm looking for the hostrt support to be compiled
>>> in if CONFIG_HAVE_IPIPE_HOSTRT is available from the arch-dep section,
>>> so we don't need CONFIG_IPIPE_HOSTRT. Generic bits may depend on HAVE_*
>>> as well.
>> First of all, the code you cited _above_ is not changed by our patches,
>> so the context still puzzles me (but maybe you are referring to some
>> other place in fact).
> 
> Patch v2 says:
> 
> diff --git a/kernel/ipipe/Kconfig b/kernel/ipipe/Kconfig
> index de5e6a3..bc7a00c 100644
> --- a/kernel/ipipe/Kconfig
> +++ b/kernel/ipipe/Kconfig
> @@ -33,3 +33,10 @@ config IPIPE_UNMASKED_CONTEXT_SWITCH
>         bool
>         depends on IPIPE
>         default n
> +
> +config HAVE_IPIPE_HOSTRT
> +       bool
> +
> +config IPIPE_HOSTRT
> +       def_bool y
> +       depends on HAVE_IPIPE_HOSTRT && IPIPE
> 
> So what's your point?
> 
>> Second, CONFIG_HAVE_IPIPE_HOSTRT is designed to be set independently of
>> CONFIG_IPIPE - it's a static arch feature like all the other
>> CONFIG_HAVE_* in arch/*/Kconfig. So it takes a second, generically
>> defined CONFIG switch if the generic support also depends on
>> CONFIG_IPIPE like in this case.
> 
> Which does not make any sense. We don't want to make this selectable at
> all. Mainline has CONFIG_HAVE_SYSCALL_WRAPPERS for instance, and you
> won't find any CONFIG_SYSCALL_WRAPPERS, because it makes no sense not to
> use them when the architecture _have_ them. It goes exactly the same way
> with hostrt.

Don't find your example. But maybe you should have a look at
[HAVE_]USER_RETURN_NOTIFIER (and maybe I should push [HAVE_]IPIPE_HOSTRT
into arch/Kconfig).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Adeos-main] [PATCH 1/2] ipipe: Pass NTP-corrected time information from Linux to higher domains

[Philippe Gerum]

On Wed, 2010-08-25 at 12:25 +0200, Jan Kiszka wrote:
> Philippe Gerum wrote:
> > On Wed, 2010-08-25 at 11:19 +0200, Jan Kiszka wrote:
> >> Philippe Gerum wrote:
> >>> On Wed, 2010-08-25 at 10:58 +0200, Jan Kiszka wrote:
> >>>> Philippe Gerum wrote:
> >>>>> On Wed, 2010-08-25 at 10:50 +0200, Jan Kiszka wrote:
> >>>>>> Philippe Gerum wrote:
> >>>>>>> On Fri, 2010-07-02 at 13:50 +0200, Wolfgang Mauerer wrote:
> >>>>>>>
> >>>>>>> <snip>
> >>>>>>>
> >>>>>>>> diff --git a/include/linux/ipipe_tickdev.h b/include/linux/ipipe_tickdev.h
> >>>>>>>> index 4a1cb1b..86f13e0 100644
> >>>>>>>> --- a/include/linux/ipipe_tickdev.h
> >>>>>>>> +++ b/include/linux/ipipe_tickdev.h
> >>>>>>>> @@ -25,6 +25,7 @@
> >>>>>>>>  #if defined(CONFIG_IPIPE) && defined(CONFIG_GENERIC_CLOCKEVENTS)
> >>>>>>> Since we should have CONFIG_HAVE_IPIPE_HOSTRT by now, let's use it.
> >>>>>> Don't get yet how this fits here.
> >>>>> arch-dep would define CONFIG_HAVE_IPIPE_HOSTRT [if IPIPE]
> >>>>>
> >>>> Still don't see the relation to the line you cited above.
> >>>>
> >>> That is because you chose to have CONFIG_IPIPE_HOSTRT and
> >>> CONFIG_HAVE_IPIPE_HOSTRT. I would have only defined the latter, the way
> >>> you define the former. I'm looking for the hostrt support to be compiled
> >>> in if CONFIG_HAVE_IPIPE_HOSTRT is available from the arch-dep section,
> >>> so we don't need CONFIG_IPIPE_HOSTRT. Generic bits may depend on HAVE_*
> >>> as well.
> >> First of all, the code you cited _above_ is not changed by our patches,
> >> so the context still puzzles me (but maybe you are referring to some
> >> other place in fact).
> > 
> > Patch v2 says:
> > 
> > diff --git a/kernel/ipipe/Kconfig b/kernel/ipipe/Kconfig
> > index de5e6a3..bc7a00c 100644
> > --- a/kernel/ipipe/Kconfig
> > +++ b/kernel/ipipe/Kconfig
> > @@ -33,3 +33,10 @@ config IPIPE_UNMASKED_CONTEXT_SWITCH
> >         bool
> >         depends on IPIPE
> >         default n
> > +
> > +config HAVE_IPIPE_HOSTRT
> > +       bool
> > +
> > +config IPIPE_HOSTRT
> > +       def_bool y
> > +       depends on HAVE_IPIPE_HOSTRT && IPIPE
> > 
> > So what's your point?
> > 
> >> Second, CONFIG_HAVE_IPIPE_HOSTRT is designed to be set independently of
> >> CONFIG_IPIPE - it's a static arch feature like all the other
> >> CONFIG_HAVE_* in arch/*/Kconfig. So it takes a second, generically
> >> defined CONFIG switch if the generic support also depends on
> >> CONFIG_IPIPE like in this case.
> > 
> > Which does not make any sense. We don't want to make this selectable at
> > all. Mainline has CONFIG_HAVE_SYSCALL_WRAPPERS for instance, and you
> > won't find any CONFIG_SYSCALL_WRAPPERS, because it makes no sense not to
> > use them when the architecture _have_ them. It goes exactly the same way
> > with hostrt.
> 
> Don't find your example.

I just did:
 find . -name 'Kconfig*' -print |xargs grep SYSCALL_WRAPPERS

>  But maybe you should have a look at
> [HAVE_]USER_RETURN_NOTIFIER (and maybe I should push [HAVE_]IPIPE_HOSTRT
> into arch/Kconfig).

And select it conditionally on IPIPE in arch/x86/Kconfig? why not.

> 
> Jan
> 

-- 
Philippe.

Re: [Adeos-main] [PATCH 1/2] ipipe: Pass NTP-corrected time information from Linux to higher domains

[Jan Kiszka]

Philippe Gerum wrote:
> On Wed, 2010-08-25 at 12:25 +0200, Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> On Wed, 2010-08-25 at 11:19 +0200, Jan Kiszka wrote:
>>>> Philippe Gerum wrote:
>>>>> On Wed, 2010-08-25 at 10:58 +0200, Jan Kiszka wrote:
>>>>>> Philippe Gerum wrote:
>>>>>>> On Wed, 2010-08-25 at 10:50 +0200, Jan Kiszka wrote:
>>>>>>>> Philippe Gerum wrote:
>>>>>>>>> On Fri, 2010-07-02 at 13:50 +0200, Wolfgang Mauerer wrote:
>>>>>>>>>
>>>>>>>>> <snip>
>>>>>>>>>
>>>>>>>>>> diff --git a/include/linux/ipipe_tickdev.h b/include/linux/ipipe_tickdev.h
>>>>>>>>>> index 4a1cb1b..86f13e0 100644
>>>>>>>>>> --- a/include/linux/ipipe_tickdev.h
>>>>>>>>>> +++ b/include/linux/ipipe_tickdev.h
>>>>>>>>>> @@ -25,6 +25,7 @@
>>>>>>>>>>  #if defined(CONFIG_IPIPE) && defined(CONFIG_GENERIC_CLOCKEVENTS)
>>>>>>>>> Since we should have CONFIG_HAVE_IPIPE_HOSTRT by now, let's use it.
>>>>>>>> Don't get yet how this fits here.
>>>>>>> arch-dep would define CONFIG_HAVE_IPIPE_HOSTRT [if IPIPE]
>>>>>>>
>>>>>> Still don't see the relation to the line you cited above.
>>>>>>
>>>>> That is because you chose to have CONFIG_IPIPE_HOSTRT and
>>>>> CONFIG_HAVE_IPIPE_HOSTRT. I would have only defined the latter, the way
>>>>> you define the former. I'm looking for the hostrt support to be compiled
>>>>> in if CONFIG_HAVE_IPIPE_HOSTRT is available from the arch-dep section,
>>>>> so we don't need CONFIG_IPIPE_HOSTRT. Generic bits may depend on HAVE_*
>>>>> as well.
>>>> First of all, the code you cited _above_ is not changed by our patches,
>>>> so the context still puzzles me (but maybe you are referring to some
>>>> other place in fact).
>>> Patch v2 says:
>>>
>>> diff --git a/kernel/ipipe/Kconfig b/kernel/ipipe/Kconfig
>>> index de5e6a3..bc7a00c 100644
>>> --- a/kernel/ipipe/Kconfig
>>> +++ b/kernel/ipipe/Kconfig
>>> @@ -33,3 +33,10 @@ config IPIPE_UNMASKED_CONTEXT_SWITCH
>>>         bool
>>>         depends on IPIPE
>>>         default n
>>> +
>>> +config HAVE_IPIPE_HOSTRT
>>> +       bool
>>> +
>>> +config IPIPE_HOSTRT
>>> +       def_bool y
>>> +       depends on HAVE_IPIPE_HOSTRT && IPIPE
>>>
>>> So what's your point?
>>>
>>>> Second, CONFIG_HAVE_IPIPE_HOSTRT is designed to be set independently of
>>>> CONFIG_IPIPE - it's a static arch feature like all the other
>>>> CONFIG_HAVE_* in arch/*/Kconfig. So it takes a second, generically
>>>> defined CONFIG switch if the generic support also depends on
>>>> CONFIG_IPIPE like in this case.
>>> Which does not make any sense. We don't want to make this selectable at
>>> all. Mainline has CONFIG_HAVE_SYSCALL_WRAPPERS for instance, and you
>>> won't find any CONFIG_SYSCALL_WRAPPERS, because it makes no sense not to
>>> use them when the architecture _have_ them. It goes exactly the same way
>>> with hostrt.
>> Don't find your example.
> 
> I just did:
>  find . -name 'Kconfig*' -print |xargs grep SYSCALL_WRAPPERS

Ah, now I see.

> 
>>  But maybe you should have a look at
>> [HAVE_]USER_RETURN_NOTIFIER (and maybe I should push [HAVE_]IPIPE_HOSTRT
>> into arch/Kconfig).
> 
> And select it conditionally on IPIPE in arch/x86/Kconfig? why not.

I can change this if you want us to, but I think it would be better to
have the generic dependency on IPIPE in a generic Kconfig - not every
arch version.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Adeos-main] [PATCH 1/2] ipipe: Pass NTP-corrected time information from Linux to higher domains

[Philippe Gerum]

On Thu, 2010-08-26 at 11:38 +0200, Jan Kiszka wrote:
> Philippe Gerum wrote:
> > On Wed, 2010-08-25 at 12:25 +0200, Jan Kiszka wrote:
> >> Philippe Gerum wrote:
> >>> On Wed, 2010-08-25 at 11:19 +0200, Jan Kiszka wrote:
> >>>> Philippe Gerum wrote:
> >>>>> On Wed, 2010-08-25 at 10:58 +0200, Jan Kiszka wrote:
> >>>>>> Philippe Gerum wrote:
> >>>>>>> On Wed, 2010-08-25 at 10:50 +0200, Jan Kiszka wrote:
> >>>>>>>> Philippe Gerum wrote:
> >>>>>>>>> On Fri, 2010-07-02 at 13:50 +0200, Wolfgang Mauerer wrote:
> >>>>>>>>>
> >>>>>>>>> <snip>
> >>>>>>>>>
> >>>>>>>>>> diff --git a/include/linux/ipipe_tickdev.h b/include/linux/ipipe_tickdev.h
> >>>>>>>>>> index 4a1cb1b..86f13e0 100644
> >>>>>>>>>> --- a/include/linux/ipipe_tickdev.h
> >>>>>>>>>> +++ b/include/linux/ipipe_tickdev.h
> >>>>>>>>>> @@ -25,6 +25,7 @@
> >>>>>>>>>>  #if defined(CONFIG_IPIPE) && defined(CONFIG_GENERIC_CLOCKEVENTS)
> >>>>>>>>> Since we should have CONFIG_HAVE_IPIPE_HOSTRT by now, let's use it.
> >>>>>>>> Don't get yet how this fits here.
> >>>>>>> arch-dep would define CONFIG_HAVE_IPIPE_HOSTRT [if IPIPE]
> >>>>>>>
> >>>>>> Still don't see the relation to the line you cited above.
> >>>>>>
> >>>>> That is because you chose to have CONFIG_IPIPE_HOSTRT and
> >>>>> CONFIG_HAVE_IPIPE_HOSTRT. I would have only defined the latter, the way
> >>>>> you define the former. I'm looking for the hostrt support to be compiled
> >>>>> in if CONFIG_HAVE_IPIPE_HOSTRT is available from the arch-dep section,
> >>>>> so we don't need CONFIG_IPIPE_HOSTRT. Generic bits may depend on HAVE_*
> >>>>> as well.
> >>>> First of all, the code you cited _above_ is not changed by our patches,
> >>>> so the context still puzzles me (but maybe you are referring to some
> >>>> other place in fact).
> >>> Patch v2 says:
> >>>
> >>> diff --git a/kernel/ipipe/Kconfig b/kernel/ipipe/Kconfig
> >>> index de5e6a3..bc7a00c 100644
> >>> --- a/kernel/ipipe/Kconfig
> >>> +++ b/kernel/ipipe/Kconfig
> >>> @@ -33,3 +33,10 @@ config IPIPE_UNMASKED_CONTEXT_SWITCH
> >>>         bool
> >>>         depends on IPIPE
> >>>         default n
> >>> +
> >>> +config HAVE_IPIPE_HOSTRT
> >>> +       bool
> >>> +
> >>> +config IPIPE_HOSTRT
> >>> +       def_bool y
> >>> +       depends on HAVE_IPIPE_HOSTRT && IPIPE
> >>>
> >>> So what's your point?
> >>>
> >>>> Second, CONFIG_HAVE_IPIPE_HOSTRT is designed to be set independently of
> >>>> CONFIG_IPIPE - it's a static arch feature like all the other
> >>>> CONFIG_HAVE_* in arch/*/Kconfig. So it takes a second, generically
> >>>> defined CONFIG switch if the generic support also depends on
> >>>> CONFIG_IPIPE like in this case.
> >>> Which does not make any sense. We don't want to make this selectable at
> >>> all. Mainline has CONFIG_HAVE_SYSCALL_WRAPPERS for instance, and you
> >>> won't find any CONFIG_SYSCALL_WRAPPERS, because it makes no sense not to
> >>> use them when the architecture _have_ them. It goes exactly the same way
> >>> with hostrt.
> >> Don't find your example.
> > 
> > I just did:
> >  find . -name 'Kconfig*' -print |xargs grep SYSCALL_WRAPPERS
> 
> Ah, now I see.
> 
> > 
> >>  But maybe you should have a look at
> >> [HAVE_]USER_RETURN_NOTIFIER (and maybe I should push [HAVE_]IPIPE_HOSTRT
> >> into arch/Kconfig).
> > 
> > And select it conditionally on IPIPE in arch/x86/Kconfig? why not.
> 
> I can change this if you want us to, but I think it would be better to
> have the generic dependency on IPIPE in a generic Kconfig - not every
> arch version.

What I mean is:

diff --git a/arch/Kconfig b/arch/Kconfig
index acda512..1b87a53 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -151,4 +151,7 @@ config HAVE_MIXED_BREAKPOINTS_REGS
 config HAVE_USER_RETURN_NOTIFIER
 	bool
 
+config HAVE_IPIPE_HOSTRT
+       bool
+
 source "kernel/gcov/Kconfig"
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index dcb0593..96ab0f4 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -58,6 +58,7 @@ config X86
 	select ANON_INODES
 	select HAVE_ARCH_KMEMCHECK
 	select HAVE_USER_RETURN_NOTIFIER
+	select HAVE_IPIPE_HOSTRT if IPIPE
 
 config INSTRUCTION_DECODER
 	def_bool (KPROBES || PERF_EVENTS)


=== Because the following would not work as expected:

	select HAVE_IPIPE_HOSTRT

config HAVE_IPIPE_HOSTRT
        bool
	depends on IPIPE


> 
> Jan
> 

-- 
Philippe.

Re: [Adeos-main] [PATCH 1/2] ipipe: Pass NTP-corrected time information from Linux to higher domains

[Jan Kiszka]

Philippe Gerum wrote:
> On Thu, 2010-08-26 at 11:38 +0200, Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> On Wed, 2010-08-25 at 12:25 +0200, Jan Kiszka wrote:
>>>> Philippe Gerum wrote:
>>>>> On Wed, 2010-08-25 at 11:19 +0200, Jan Kiszka wrote:
>>>>>> Philippe Gerum wrote:
>>>>>>> On Wed, 2010-08-25 at 10:58 +0200, Jan Kiszka wrote:
>>>>>>>> Philippe Gerum wrote:
>>>>>>>>> On Wed, 2010-08-25 at 10:50 +0200, Jan Kiszka wrote:
>>>>>>>>>> Philippe Gerum wrote:
>>>>>>>>>>> On Fri, 2010-07-02 at 13:50 +0200, Wolfgang Mauerer wrote:
>>>>>>>>>>>
>>>>>>>>>>> <snip>
>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/include/linux/ipipe_tickdev.h b/include/linux/ipipe_tickdev.h
>>>>>>>>>>>> index 4a1cb1b..86f13e0 100644
>>>>>>>>>>>> --- a/include/linux/ipipe_tickdev.h
>>>>>>>>>>>> +++ b/include/linux/ipipe_tickdev.h
>>>>>>>>>>>> @@ -25,6 +25,7 @@
>>>>>>>>>>>>  #if defined(CONFIG_IPIPE) && defined(CONFIG_GENERIC_CLOCKEVENTS)
>>>>>>>>>>> Since we should have CONFIG_HAVE_IPIPE_HOSTRT by now, let's use it.
>>>>>>>>>> Don't get yet how this fits here.
>>>>>>>>> arch-dep would define CONFIG_HAVE_IPIPE_HOSTRT [if IPIPE]
>>>>>>>>>
>>>>>>>> Still don't see the relation to the line you cited above.
>>>>>>>>
>>>>>>> That is because you chose to have CONFIG_IPIPE_HOSTRT and
>>>>>>> CONFIG_HAVE_IPIPE_HOSTRT. I would have only defined the latter, the way
>>>>>>> you define the former. I'm looking for the hostrt support to be compiled
>>>>>>> in if CONFIG_HAVE_IPIPE_HOSTRT is available from the arch-dep section,
>>>>>>> so we don't need CONFIG_IPIPE_HOSTRT. Generic bits may depend on HAVE_*
>>>>>>> as well.
>>>>>> First of all, the code you cited _above_ is not changed by our patches,
>>>>>> so the context still puzzles me (but maybe you are referring to some
>>>>>> other place in fact).
>>>>> Patch v2 says:
>>>>>
>>>>> diff --git a/kernel/ipipe/Kconfig b/kernel/ipipe/Kconfig
>>>>> index de5e6a3..bc7a00c 100644
>>>>> --- a/kernel/ipipe/Kconfig
>>>>> +++ b/kernel/ipipe/Kconfig
>>>>> @@ -33,3 +33,10 @@ config IPIPE_UNMASKED_CONTEXT_SWITCH
>>>>>         bool
>>>>>         depends on IPIPE
>>>>>         default n
>>>>> +
>>>>> +config HAVE_IPIPE_HOSTRT
>>>>> +       bool
>>>>> +
>>>>> +config IPIPE_HOSTRT
>>>>> +       def_bool y
>>>>> +       depends on HAVE_IPIPE_HOSTRT && IPIPE
>>>>>
>>>>> So what's your point?
>>>>>
>>>>>> Second, CONFIG_HAVE_IPIPE_HOSTRT is designed to be set independently of
>>>>>> CONFIG_IPIPE - it's a static arch feature like all the other
>>>>>> CONFIG_HAVE_* in arch/*/Kconfig. So it takes a second, generically
>>>>>> defined CONFIG switch if the generic support also depends on
>>>>>> CONFIG_IPIPE like in this case.
>>>>> Which does not make any sense. We don't want to make this selectable at
>>>>> all. Mainline has CONFIG_HAVE_SYSCALL_WRAPPERS for instance, and you
>>>>> won't find any CONFIG_SYSCALL_WRAPPERS, because it makes no sense not to
>>>>> use them when the architecture _have_ them. It goes exactly the same way
>>>>> with hostrt.
>>>> Don't find your example.
>>> I just did:
>>>  find . -name 'Kconfig*' -print |xargs grep SYSCALL_WRAPPERS
>> Ah, now I see.
>>
>>>>  But maybe you should have a look at
>>>> [HAVE_]USER_RETURN_NOTIFIER (and maybe I should push [HAVE_]IPIPE_HOSTRT
>>>> into arch/Kconfig).
>>> And select it conditionally on IPIPE in arch/x86/Kconfig? why not.
>> I can change this if you want us to, but I think it would be better to
>> have the generic dependency on IPIPE in a generic Kconfig - not every
>> arch version.
> 
> What I mean is:
> 
> diff --git a/arch/Kconfig b/arch/Kconfig
> index acda512..1b87a53 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -151,4 +151,7 @@ config HAVE_MIXED_BREAKPOINTS_REGS
>  config HAVE_USER_RETURN_NOTIFIER
>  	bool
>  
> +config HAVE_IPIPE_HOSTRT
> +       bool
> +
>  source "kernel/gcov/Kconfig"
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index dcb0593..96ab0f4 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -58,6 +58,7 @@ config X86
>  	select ANON_INODES
>  	select HAVE_ARCH_KMEMCHECK
>  	select HAVE_USER_RETURN_NOTIFIER
> +	select HAVE_IPIPE_HOSTRT if IPIPE
>  
>  config INSTRUCTION_DECODER
>  	def_bool (KPROBES || PERF_EVENTS)
> 

I fully understood what you meant, and to end this discussion I already
implemented it. We will just have to make sure that every arch properly
adds the required "if IPIPE" suffix.

Jan


-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Adeos-main] [PATCH 1/2] ipipe: Pass NTP-corrected time information from Linux to higher domains

[Philippe Gerum]

On Thu, 2010-08-26 at 12:47 +0200, Jan Kiszka wrote:
> Philippe Gerum wrote:
> > On Thu, 2010-08-26 at 11:38 +0200, Jan Kiszka wrote:
> >> Philippe Gerum wrote:
> >>> On Wed, 2010-08-25 at 12:25 +0200, Jan Kiszka wrote:
> >>>> Philippe Gerum wrote:
> >>>>> On Wed, 2010-08-25 at 11:19 +0200, Jan Kiszka wrote:
> >>>>>> Philippe Gerum wrote:
> >>>>>>> On Wed, 2010-08-25 at 10:58 +0200, Jan Kiszka wrote:
> >>>>>>>> Philippe Gerum wrote:
> >>>>>>>>> On Wed, 2010-08-25 at 10:50 +0200, Jan Kiszka wrote:
> >>>>>>>>>> Philippe Gerum wrote:
> >>>>>>>>>>> On Fri, 2010-07-02 at 13:50 +0200, Wolfgang Mauerer wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> <snip>
> >>>>>>>>>>>
> >>>>>>>>>>>> diff --git a/include/linux/ipipe_tickdev.h b/include/linux/ipipe_tickdev.h
> >>>>>>>>>>>> index 4a1cb1b..86f13e0 100644
> >>>>>>>>>>>> --- a/include/linux/ipipe_tickdev.h
> >>>>>>>>>>>> +++ b/include/linux/ipipe_tickdev.h
> >>>>>>>>>>>> @@ -25,6 +25,7 @@
> >>>>>>>>>>>>  #if defined(CONFIG_IPIPE) && defined(CONFIG_GENERIC_CLOCKEVENTS)
> >>>>>>>>>>> Since we should have CONFIG_HAVE_IPIPE_HOSTRT by now, let's use it.
> >>>>>>>>>> Don't get yet how this fits here.
> >>>>>>>>> arch-dep would define CONFIG_HAVE_IPIPE_HOSTRT [if IPIPE]
> >>>>>>>>>
> >>>>>>>> Still don't see the relation to the line you cited above.
> >>>>>>>>
> >>>>>>> That is because you chose to have CONFIG_IPIPE_HOSTRT and
> >>>>>>> CONFIG_HAVE_IPIPE_HOSTRT. I would have only defined the latter, the way
> >>>>>>> you define the former. I'm looking for the hostrt support to be compiled
> >>>>>>> in if CONFIG_HAVE_IPIPE_HOSTRT is available from the arch-dep section,
> >>>>>>> so we don't need CONFIG_IPIPE_HOSTRT. Generic bits may depend on HAVE_*
> >>>>>>> as well.
> >>>>>> First of all, the code you cited _above_ is not changed by our patches,
> >>>>>> so the context still puzzles me (but maybe you are referring to some
> >>>>>> other place in fact).
> >>>>> Patch v2 says:
> >>>>>
> >>>>> diff --git a/kernel/ipipe/Kconfig b/kernel/ipipe/Kconfig
> >>>>> index de5e6a3..bc7a00c 100644
> >>>>> --- a/kernel/ipipe/Kconfig
> >>>>> +++ b/kernel/ipipe/Kconfig
> >>>>> @@ -33,3 +33,10 @@ config IPIPE_UNMASKED_CONTEXT_SWITCH
> >>>>>         bool
> >>>>>         depends on IPIPE
> >>>>>         default n
> >>>>> +
> >>>>> +config HAVE_IPIPE_HOSTRT
> >>>>> +       bool
> >>>>> +
> >>>>> +config IPIPE_HOSTRT
> >>>>> +       def_bool y
> >>>>> +       depends on HAVE_IPIPE_HOSTRT && IPIPE
> >>>>>
> >>>>> So what's your point?
> >>>>>
> >>>>>> Second, CONFIG_HAVE_IPIPE_HOSTRT is designed to be set independently of
> >>>>>> CONFIG_IPIPE - it's a static arch feature like all the other
> >>>>>> CONFIG_HAVE_* in arch/*/Kconfig. So it takes a second, generically
> >>>>>> defined CONFIG switch if the generic support also depends on
> >>>>>> CONFIG_IPIPE like in this case.
> >>>>> Which does not make any sense. We don't want to make this selectable at
> >>>>> all. Mainline has CONFIG_HAVE_SYSCALL_WRAPPERS for instance, and you
> >>>>> won't find any CONFIG_SYSCALL_WRAPPERS, because it makes no sense not to
> >>>>> use them when the architecture _have_ them. It goes exactly the same way
> >>>>> with hostrt.
> >>>> Don't find your example.
> >>> I just did:
> >>>  find . -name 'Kconfig*' -print |xargs grep SYSCALL_WRAPPERS
> >> Ah, now I see.
> >>
> >>>>  But maybe you should have a look at
> >>>> [HAVE_]USER_RETURN_NOTIFIER (and maybe I should push [HAVE_]IPIPE_HOSTRT
> >>>> into arch/Kconfig).
> >>> And select it conditionally on IPIPE in arch/x86/Kconfig? why not.
> >> I can change this if you want us to, but I think it would be better to
> >> have the generic dependency on IPIPE in a generic Kconfig - not every
> >> arch version.
> > 
> > What I mean is:
> > 
> > diff --git a/arch/Kconfig b/arch/Kconfig
> > index acda512..1b87a53 100644
> > --- a/arch/Kconfig
> > +++ b/arch/Kconfig
> > @@ -151,4 +151,7 @@ config HAVE_MIXED_BREAKPOINTS_REGS
> >  config HAVE_USER_RETURN_NOTIFIER
> >  	bool
> >  
> > +config HAVE_IPIPE_HOSTRT
> > +       bool
> > +
> >  source "kernel/gcov/Kconfig"
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index dcb0593..96ab0f4 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -58,6 +58,7 @@ config X86
> >  	select ANON_INODES
> >  	select HAVE_ARCH_KMEMCHECK
> >  	select HAVE_USER_RETURN_NOTIFIER
> > +	select HAVE_IPIPE_HOSTRT if IPIPE
> >  
> >  config INSTRUCTION_DECODER
> >  	def_bool (KPROBES || PERF_EVENTS)
> > 
> 
> I fully understood what you meant, and to end this discussion I already
> implemented it. We will just have to make sure that every arch properly
> adds the required "if IPIPE" suffix.

Given that "the other way" (i.e. moving everything to the generic
section) woul not work, and that adding an I-pipe symbol conditionally
to have the pipeline enabled is not generally silly, I think it's ok as
well.

> 
> Jan
> 
> 

-- 
Philippe.

[Adeos-main] [PATCH 2/2] ipipe: CLOCK_HOST_REALTIME: x86-specific part

[Wolfgang Mauerer]

x86 specific mechanism to make NTP-corrected time information
available to non-Linux domains.

Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@domain.hid>
Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
---
 arch/x86/Kconfig                  |    1 +
 arch/x86/include/asm/ipipe_base.h |    3 ++-
 arch/x86/include/asm/tsc.h        |    1 +
 arch/x86/kernel/tsc.c             |    4 ++--
 arch/x86/kernel/vsyscall_64.c     |    6 ++++++
 5 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 89b72a9..4b1adbc 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -50,6 +50,7 @@ config X86
 	select HAVE_KERNEL_BZIP2
 	select HAVE_KERNEL_LZMA
 	select HAVE_ARCH_KMEMCHECK
+	select HAVE_IPIPE_HOSTRT
 
 config OUTPUT_FORMAT
 	string
diff --git a/arch/x86/include/asm/ipipe_base.h b/arch/x86/include/asm/ipipe_base.h
index 1098d6f..6131fe9 100644
--- a/arch/x86/include/asm/ipipe_base.h
+++ b/arch/x86/include/asm/ipipe_base.h
@@ -66,7 +66,8 @@
 #define IPIPE_EVENT_INIT	(IPIPE_FIRST_EVENT + 4)
 #define IPIPE_EVENT_EXIT	(IPIPE_FIRST_EVENT + 5)
 #define IPIPE_EVENT_CLEANUP	(IPIPE_FIRST_EVENT + 6)
-#define IPIPE_LAST_EVENT	IPIPE_EVENT_CLEANUP
+#define IPIPE_EVENT_HOSTRT	(IPIPE_FIRST_EVENT + 7)
+#define IPIPE_LAST_EVENT	IPIPE_EVENT_HOSTRT
 #define IPIPE_NR_EVENTS		(IPIPE_LAST_EVENT + 1)
 
 #define ex_do_divide_error			0
diff --git a/arch/x86/include/asm/tsc.h b/arch/x86/include/asm/tsc.h
index c042729..aed7d66 100644
--- a/arch/x86/include/asm/tsc.h
+++ b/arch/x86/include/asm/tsc.h
@@ -14,6 +14,7 @@
  */
 typedef unsigned long long cycles_t;
 
+extern struct clocksource clocksource_tsc;
 extern unsigned int cpu_khz;
 extern unsigned int tsc_khz;
 
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 597683a..5e298a2 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -699,7 +699,7 @@ core_initcall(cpufreq_tsc);
 
 /* clocksource code */
 
-static struct clocksource clocksource_tsc;
+struct clocksource clocksource_tsc;
 
 /*
  * We compare the TSC to the cycle_last value in the clocksource
@@ -745,7 +745,7 @@ static void resume_tsc(void)
 	clocksource_tsc.cycle_last = 0;
 }
 
-static struct clocksource clocksource_tsc = {
+struct clocksource clocksource_tsc = {
 	.name                   = "tsc",
 	.rating                 = 300,
 	.read                   = read_tsc,
diff --git a/arch/x86/kernel/vsyscall_64.c b/arch/x86/kernel/vsyscall_64.c
index 8cb4974..f1b90f7 100644
--- a/arch/x86/kernel/vsyscall_64.c
+++ b/arch/x86/kernel/vsyscall_64.c
@@ -32,6 +32,7 @@
 #include <linux/cpu.h>
 #include <linux/smp.h>
 #include <linux/notifier.h>
+#include <linux/ipipe_tickdev.h>
 
 #include <asm/vsyscall.h>
 #include <asm/pgtable.h>
@@ -89,6 +90,11 @@ void update_vsyscall(struct timespec *wall_time, struct clocksource *clock)
 	vsyscall_gtod_data.wall_to_monotonic = wall_to_monotonic;
 	vsyscall_gtod_data.wall_time_coarse = __current_kernel_time();
 	write_sequnlock_irqrestore(&vsyscall_gtod_data.lock, flags);
+
+#ifdef CONFIG_IPIPE_HOSTRT
+	if (clock == &clocksource_tsc)
+		update_ipipe_hostrt(wall_time, clock);
+#endif
 }
 
 /* RED-PEN may want to readd seq locking, but then the variable should be
-- 
1.6.4