Re: [Minios-devel] [UNIKRAFT PATCH v4 3/9] plat/linuxu: Add linuxu (x86_64) timer support

[Florian Schmidt <Florian.Schmidt@xxxxxxxxx>]

Hi Sharan,

going through these comments at the moment:

On 08/27/2018 02:38 PM, Costin Lupu wrote:

> > Missing include of signal.h
> Right, more than that the current patch should come *after* the patch
> adding interrupts support in linuxu.
I'll reverse the order and include linuxu/signal.h

> > > +void ukplat_time_init(void)
> > > +{
> > > +    struct uk_sigevent sigev;
> > > +    struct itimerspec its;
> > > +    int rc;
> > > +
> > > +    ukplat_irq_register(TIMER_SIGNUM, timer_handler, NULL);
> > > +
> > > +    memset(&sigev, 0, sizeof(sigev));
> > > +    sigev.sigev_notify = 0;
> >
> > A minor comment, prefer if we use SIGALRM inplace of TIMER_SIGNUM :)
> > since we are working with signal numbers here.
> TIMER_SIGNUM is a define over the preferred signal number (in our case
> SIGALRM). I don't see what's wrong with this approach.
I think this is fine and we can keep it like that?

> > >      void ukplat_time_fini(void)
> > >    {
> >
> > Do we want to unregister the timer interrupt while cleaning up the
> > platform?
> Yeah, why not?
>
> > > +    sys_timer_delete(timerid);
> > >    }
Also seems like the right thing to do to me. ANy reasons why that would 
be a bad idea?
Cheers,
Florian

--
Dr. Florian Schmidt
フローリアン・シュミット
Research Scientist,
Systems and Machine Learning Group
NEC Laboratories Europe
Kurfürsten-Anlage 36, D-69115 Heidelberg
Tel.     +49 (0)6221 4342-265
Fax:     +49 (0)6221 4342-155
e-mail:  florian.schmidt@xxxxxxxxx
============================================================
Registered at Amtsgericht Mannheim, Germany, HRB728558

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