Re: [Xen-devel] [PATCH] console: avoid printing no or null time stamps

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.06.18 at 10:43, <julien.grall@xxxxxxx> wrote:
> On 26/06/18 08:24, Jan Beulich wrote:
>> During early boot timestamps aren't very useful, as they're all zero
>> (in "boot" mode) or absent altogether (in "date" and "datems" modes).
>> Log "boot" format timestamps when the date formats aren't available yet,
>> and log raw timestamps when boot ones are still all zero. Also add a
>> "raw" mode.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> ARM side build-tested only; I'm in particular unsure whether
>> READ_SYSREG64(CNTPCT_EL0) can indeed be used right at start of day.
> On most of the platforms, the timer will have been correctly enabled by 
> the firmware. However, on a few platforms it may require additional 
> setup that will be performed by platform_init_time().
> 
> In any case, CNTCPT_EL0 can always be read but may return garbage on 
> those few platforms. I would not worry too much for those platforms 
> thoughts.

Can this be detected, such that the function could be made return zero
instead until the necessary setup has happened?

>> @@ -698,26 +701,30 @@ static void printk_start_of_line(const c
>>       case TSM_DATE_MS:
>>           tm = wallclock_time(&nsec);
>>   
>> -        if ( tm.tm_mday == 0 )
>> -            return;
>> -
>> -        if ( opt_con_timestamp_mode == TSM_DATE )
>> -            snprintf(tstr, sizeof(tstr), "[%04u-%02u-%02u %02u:%02u:%02u] ",
>> -                     1900 + tm.tm_year, tm.tm_mon + 1, tm.tm_mday,
>> -                     tm.tm_hour, tm.tm_min, tm.tm_sec);
>> -        else
>> +        if ( tm.tm_mday )
>> +        {
>>               snprintf(tstr, sizeof(tstr),
>> -                     "[%04u-%02u-%02u %02u:%02u:%02u.%03"PRIu64"] ",
>> +                     opt_con_timestamp_mode == TSM_DATE
>> +                     ? "[%04u-%02u-%02u %02u:%02u:%02u] "
>> +                     : "[%04u-%02u-%02u %02u:%02u:%02u.%03"PRIu64"] ",
>>                        1900 + tm.tm_year, tm.tm_mon + 1, tm.tm_mday,
>>                        tm.tm_hour, tm.tm_min, tm.tm_sec, nsec / 1000000);
> 
> I find this change rather difficult to read because the number of 
> arguments for the 2 formats are different. It would be better to keep 
> the two snprintf separately.

And I find the redundancy rather ugly to maintain, so I'd prefer to stick to
single invocation.

> But I am fairly surprised the compiler is happy with such changes.

Why would it not be - excess arguments are not a problem.

>> --- a/xen/include/asm-arm/time.h
>> +++ b/xen/include/asm-arm/time.h
>> @@ -5,11 +5,11 @@
>>       DT_MATCH_COMPATIBLE("arm,armv7-timer"), \
>>       DT_MATCH_COMPATIBLE("arm,armv8-timer")
>>   
>> -typedef unsigned long cycles_t;
>> +typedef uint64_t cycles_t;
> 
> Please mention this change in the commit message.

Added "For the ARM side get_cycles() to produce a meaningful value,
ARM's cycle_t gets changed to uint64_t."

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel