Page MenuHomeFreeBSD

Add support for additional architectures in ntp.
ClosedPublic

Authored by cy on Jun 3 2015, 2:51 AM.

Details

Summary

This patch adds support for additional architectures in ntp.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

cy retitled this revision from to Add support for additional architectures in ntp..Jun 3 2015, 2:51 AM
cy updated this object.
cy edited the test plan for this revision. (Show Details)
cy added reviewers: roberto, ian, kib, andrew, jmg.
cy set the repository for this revision to rS FreeBSD src repository.
cy updated this revision to Diff 5901.
cy added a reviewer: imp.Jun 3 2015, 2:57 AM
imp edited edge metadata.Jun 3 2015, 5:02 AM

I wonder why you need these #defines and can't use sizeof(type) instead...

usr.sbin/ntp/config.h
1472 ↗(On Diff #5901)

#ifdef LP64

covers this.

1482 ↗(On Diff #5901)

Same as above.

1501 ↗(On Diff #5901)

I think the more proper thing here is #if !defined(i386) && !defined(powerpc)

andrew added inline comments.Jun 3 2015, 6:17 AM
usr.sbin/ntp/config.h
1545 ↗(On Diff #5901)

I think you mean __aarch64__

kib edited edge metadata.Jun 3 2015, 7:37 AM

And what about SIZEOF_PTHREAD_T ?
Similarly CHAR_UNSIGNED, which should be true for ppc* and probably arms.

IMO, you cannot use single config.h on different arches without loosing the sanity. Better to generate per-arch config.<arch>.h on the native machine and check that in.

Did you looked how similar MD bits are handled for config.h's from e.g. gnu/* ?

roberto edited edge metadata.Jun 3 2015, 8:55 AM

To all: do not forget that originally this file is automatically generated during configure. We are able to modify it because we are not using configure in the freebsd context but keep in mind that this change will have to be merged again during the next update.

As ntp seems to be back evolving regularely, maybe clean up the changes as expected in config.h.in (focus on type sizes instead of just list of architecture-related symbols) and submit upstream? The undermydesk changes I could not care less though :)

jmg edited edge metadata.Jun 3 2015, 4:16 PM
usr.sbin/ntp/config.h
1472 ↗(On Diff #5901)

_LP64 or

__LP64__

from cc -E -dM - < /dev/null

1501 ↗(On Diff #5901)

or swap the orders and make it (reads easier to me):
#if defined(i386) || defined(powerpc)
4
#else
8
#endif

cy marked 3 inline comments as done.Jun 4 2015, 5:12 AM

Roberto@, I agree. This should be submitted upline.

usr.sbin/ntp/config.h
1501 ↗(On Diff #5901)

Wouldn't #ifdef LP64 work here too?

cy edited edge metadata.Jun 4 2015, 5:36 AM
cy updated this revision to Diff 5936.

Updated diff attached.

cy updated this revision to Diff 5943.Jun 4 2015, 12:55 PM

Correctly handle time_t with arm too.

imp added inline comments.Jun 4 2015, 3:01 PM
usr.sbin/ntp/config.h
1501 ↗(On Diff #5943)

My code was correct, but this ifdef is wrong.
All mips, no matter the flavor is 8 byte.
i386 and powerpc are the only two that are still 4 byte.
We don't follow LP64-ness here at all.

So no, LP64 isn't appropriate here.

cy added a comment.Jun 5 2015, 5:50 AM

It's been corrected. Thoughts?

kib removed a reviewer: kib.Jun 5 2015, 8:08 AM
kib resigned from this revision.

So you ignored my feedback.

imp added a comment.Jun 5 2015, 4:11 PM
In D2720#52014, @cy wrote:

It's been corrected. Thoughts?

It hasn't been corrected. time_t is still wrong.

imp edited edge metadata.Jun 5 2015, 4:49 PM
imp added a subscriber: kib.
imp requested changes to this revision.
In D2720#51478, @kib wrote:

And what about SIZEOF_PTHREAD_T ?
Similarly CHAR_UNSIGNED, which should be true for ppc* and probably arms

These suggestions are not implemented, but need to be. This is required for things to work properly on these systems.

IMO, you cannot use single config.h on different arches without loosing the sanity. Better to generate per-arch config.<arch>.h on the native machine and check that in.
Did you looked how similar MD bits are handled for config.h's from e.g. gnu/* ?

These are good suggestions, but are optional. I agree with them, but what you've done here will also work, even if it is a less optimal solution.

This revision now requires changes to proceed.Jun 5 2015, 4:49 PM
ian edited edge metadata.EditedJun 6 2015, 7:50 PM

This diff lacks full context, making it difficult to comment on things that were not changed that should have been.

I think these changes are also needed. There may be a better way to do the big-endian detection.

 /* Default location of crypto key info */
-#define NTP_KEYSDIR "/usr/local/etc"
+#define NTP_KEYSDIR "/etc/ntp"

/* Define WORDS_BIGENDIAN to 1 if your processor stores words with the most
    significant byte first (like Motorola and SPARC, unlike Intel). */
-#if defined AC_APPLE_UNIVERSAL_BUILD
-# if defined __BIG_ENDIAN__
-#  define WORDS_BIGENDIAN 1
-# endif
-#else
-# ifndef WORDS_BIGENDIAN
-/* #  undef WORDS_BIGENDIAN */
-# endif
+#if defined(__ARMEB__) || defined(__MIPSEB__) || defined(__powerpc__) || \
+    defined(__powerpc64__) || defined(__sparc64__)
+#define WORDS_BIGENDIAN 1
 #endif
 
 /* routine worker child proc uses to exit. */
cy marked an inline comment as done.Jun 6 2015, 9:59 PM

Agreed. I've been pondering kib's comment for a while now. Interesting. I think this is more of a long term target, requiring detailed documentation (probably written by me) on which files to generate and how to. The more we can automate the better. Then it would be a matter of getting the target names correct.

I'll upload the latest diff now. At least people can have a look at it while I'm out today. I'll have time to work on it tonight.

cy edited edge metadata.Jun 6 2015, 10:00 PM
cy updated this revision to Diff 5998.

This is the full file with diff.

cy added a comment.Jun 7 2015, 3:26 PM

After having considered kib's suggestion, I'd like to submit his idea.

imp added inline comments.Jun 7 2015, 9:16 PM
config.h
1624 ↗(On Diff #5998)

Don't you think you'd be happier with an #endif here?
:)

ian edited edge metadata.Jun 8 2015, 2:39 PM
ian accepted this revision.

Looks good. I've tested this on arm (after adding the missing #endif), and discovered that it works properly on armv4/5 systems, but ntpd still fails to keep time properly on armv6.

At this point I'm beginning to suspect the timekeeping problem is either an armv6 bug (like some floating point support error), or it's an ntpd bug that's harmless on other systems but triggers on armv6 (there's one of those in the old ntpd due to an unitialized var that's harmless on x86 and only "mostly harmless" on arm).

config.h
1623 ↗(On Diff #5998)

missing #endif here

andrew edited edge metadata.Jun 22 2015, 2:27 PM
andrew accepted this revision.

Was there anything holding this up now? There have been reports this fixes ntp on MIPS.

I just tested this patch on a MIPS64 machine (ERL), running r284544M.

Without this patch, ntpdate reports a large offset when starting, but
the clock is left at 1970. With this patch, ntpdate reports a large
offset when starting, and the date is properly set when the machine
goes multi-user.

cy added a comment.Jun 22 2015, 7:09 PM

I like kib's suggestion. Unfortunately qemu-sbruno with poudriere doesn't support all architectures yet. What if we commit the proposed config for now with the long fee goal of one config file per architecture?

kib added a comment.Jun 23 2015, 5:43 AM
In D2720#55992, @cy wrote:

I like kib's suggestion. Unfortunately qemu-sbruno with poudriere doesn't support all architectures yet. What if we commit the proposed config for now with the long fee goal of one config file per architecture?

I think something must be done now. If there is nothing better than committing the existing patch, so be it.

But, IMO, you could already provide the per-arch config.h. For some arches, where you have access to the machine and can generate the genuine config.h, it would be the real file. For other arches, it could be hand-edited. If people pop up who complain about non-working ntpd on such arches, that means they have both machine and interest, so they should be able to follow your instructions to provide you with the right config.h for commit.

cy added a comment.Jun 24 2015, 2:47 AM

Makes sense. Not all arches are supported by qemu-sbruno. I'll include what I can gen a config file for. In the mean time I'll include a new config.h until I gen arch specific config.h files.

cy added a comment.Jun 24 2015, 2:51 AM

Makes sense. Not all arches are supported by qemu-sbruno. I'll include what I can gen a config file for. In the mean time I'll include a new config.h until I gen arch specific config.h files.

cy edited edge metadata.Jun 24 2015, 2:52 AM
cy updated this revision to Diff 6415.

This should cover it.

This revision was automatically updated to reflect the committed changes.