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

[Leif Lindholm]

On Mon, Apr 01, 2013 at 04:31:13AM +0200, 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.
 
What would be a better way?

> > +    }
> > +  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.
> Does ARM have anything like TSC?
 
Not standardised and architecturally required.
The closest thing we have in base ARMv7-A is the performance monitor
cycle counter - but the performance monitor extension is "strongly
recommended" rather than required.

> > +static inline grub_size_t
> > +page_align (grub_size_t size)
> > +{
> > +  return (size + (1 << 12) - 1) & (~((1 << 12) - 1));
> > +}
> 
> We already have ALIGN_UP

In my defense, that is a verbatim copy from loader/ia64/efi/linux.c :)
There is a comment in that file that some code that I am now copying
in my port might be better to move to efi/mm.c...

/
    Leif