Re: [PATCH 7/7] Add support for ARM UEFI ("EFI") platforms

[Francesco Lavra]

On 04/01/2013 04:31 AM, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>> +static grub_uint64_t
> 
>> +grub_efi_get_time_ms(void)
>> +{
>> +  grub_efi_time_t now;
>> +  grub_uint64_t retval;
>> +  grub_efi_status_t status;
>> +
>> +  status = efi_call_2 (grub_efi_system_table->runtime_services->get_time,
>> +                   &now, NULL);
>> +  if (status != GRUB_EFI_SUCCESS)
>> +    {
>> +      grub_printf("No time!\n");
>> +      return 0;
> 
> This is about the worse thing you can do. It will make any timeout go wrong.

How would you handle such a case? I guess a machine which can't provide
this runtime service would need some more work in its EFI firmware
before being ready for GRUB, so perhaps this is a moot point.

> 
>> +    }
>> +  retval = now.year * 365 * 24 * 60 * 60 * 1000;
>> +  retval += now.month * 30 * 24 * 60 * 60 * 1000;
>> +  retval += now.day * 24 * 60 * 60 * 1000;
>> +  retval += now.hour * 60 * 60 * 1000;
>> +  retval += now.minute * 60 * 1000;
>> +  retval += now.second * 1000;
>> +  retval += now.nanosecond / 1000;
>> + 
>> +  grub_dprintf("timer", "timestamp: 0x%llx\n", retval);
>> +
>> +  return retval;
> 
> This is almost a verbatim copy of what we had for i386 efi before but it
> went haywire in many ways. Like jumps forward or backward around end of
> month or when one sets datetime. Or around the summer/winter timezone
> transition.

I propose to re-use the existing function grub_datetime2unixtime()
(which handles correctly the number of days of each month, as well as
leap years), instead of doing the calculations here. And take into
account the time_zone member of grub_efi_time_t as well.
Here is how I would write it:

grub_uint64_t
grub_efi_get_time_ms (void)
{
  grub_efi_time_t efi_time;
  struct grub_datetime datetime;
  grub_int32_t unixtime;
  grub_uint64_t retval;

  if (efi_call_2 (grub_efi_system_table->runtime_services->get_time,
                  &efi_time, 0) != GRUB_EFI_SUCCESS)
    {
      grub_printf("No time!\n");
      return 0;
    }
  datetime.year = efi_time.year;
  datetime.month = efi_time.month;
  datetime.day = efi_time.day;
  datetime.hour = efi_time.hour;
  datetime.minute = efi_time.minute;
  datetime.second = efi_time.second;
  if (!grub_datetime2unixtime (&datetime, &unixtime))
        return 0;
  unixtime += 60 * efi_time.time_zone;
  retval = (grub_uint64_t)unixtime * 1000
           + efi_time.nanosecond / 1000000;
  grub_dprintf("timer", "timestamp: 0x%llx\n", retval);
  return retval;
}

Also, there is nothing ARM-specific in this function, so I would put it
in a generic EFI file like kern/efi/efi.c.

> Does ARM have anything like TSC?

ARMv7 does not have an architecturally-defined mechanism of retrieving
the timestamp, it's up to the system peripherals to provide time-keeping
capabilities.

--
Francesco