|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [UNIKRAFT PATCH v4 3/9] plat/linuxu: Add linuxu (x86_64) timer support
Hi Sharan,
Please see my comments line.
On 08/27/2018 02:45 PM, Sharan Santhanam wrote:
> Hello,
>
> Please find the comments inline:
>
>
> On 08/20/2018 01:21 PM, Florian Schmidt wrote:
>> From: Costin Lupu <costin.lupu@xxxxxxxxx>
>>
>> We use sys_timer_* syscalls for timer support on plat/linuxu.
>>
>> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
>> Signed-off-by: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
>> ---
>> plat/linuxu/Makefile.uk | 1 +
>> plat/linuxu/include/linuxu/syscall-x86_64.h | 5 ++
>> plat/linuxu/include/linuxu/syscall.h | 27 +++++++++++
>> plat/linuxu/include/linuxu/time.h | 45 ++++++++++++++++++
>> plat/linuxu/time.c | 52 ++++++++++++++++++++-
>> 5 files changed, 129 insertions(+), 1 deletion(-)
>> create mode 100644 plat/linuxu/include/linuxu/time.h
>>
>> diff --git a/plat/linuxu/Makefile.uk b/plat/linuxu/Makefile.uk
>> index 38fcedc..4c815e5 100644
>> --- a/plat/linuxu/Makefile.uk
>> +++ b/plat/linuxu/Makefile.uk
>> @@ -31,3 +31,4 @@ LIBLINUXUPLAT_SRCS-y +=
>> $(LIBLINUXUPLAT_BASE)/shutdown.c
>> LIBLINUXUPLAT_SRCS-y += $(LIBLINUXUPLAT_BASE)/memory.c
>> LIBLINUXUPLAT_SRCS-y += $(LIBLINUXUPLAT_BASE)/lcpu.c
>> LIBLINUXUPLAT_SRCS-y += $(LIBLINUXUPLAT_BASE)/time.c
>> +LIBLINUXUPLAT_SRCS-y +=
>> $(UK_PLAT_COMMON_BASE)/lcpu.c|common
>> diff --git a/plat/linuxu/include/linuxu/syscall-x86_64.h
>> b/plat/linuxu/include/linuxu/syscall-x86_64.h
>> index c3c4550..09efee3 100644
>> --- a/plat/linuxu/include/linuxu/syscall-x86_64.h
>> +++ b/plat/linuxu/include/linuxu/syscall-x86_64.h
>> @@ -46,6 +46,11 @@
>> #define __SC_MUNMAP 11
>> #define __SC_IOCTL 16
>> #define __SC_EXIT 60
>> +#define __SC_TIMER_CREATE 222
>> +#define __SC_TIMER_SETTIME 223
>> +#define __SC_TIMER_GETTIME 224
>> +#define __SC_TIMER_GETOVERRUN 225
>> +#define __SC_TIMER_DELETE 226
>> #define __SC_PSELECT6 270
>> /* NOTE: from linux-4.6.3 (arch/x86/entry/entry_64.S):
>> diff --git a/plat/linuxu/include/linuxu/syscall.h
>> b/plat/linuxu/include/linuxu/syscall.h
>> index 7ebc9c0..e287009 100644
>> --- a/plat/linuxu/include/linuxu/syscall.h
>> +++ b/plat/linuxu/include/linuxu/syscall.h
>> @@ -108,4 +108,31 @@ static inline int sys_pselect6(int nfds,
>> (long) sigmask);
>> }
>> +static inline int sys_timer_create(clockid_t which_clock,
>> + struct uk_sigevent *timer_event_spec, timer_t *created_timer_id)
>> +{
>> + return (int) syscall3(__SC_TIMER_CREATE,
>> + (long) which_clock,
>> + (long) timer_event_spec,
>> + (long) created_timer_id);
>> +
>> +}
>> +
>> +static inline int sys_timer_settime(timer_t timerid, int flags,
>> + const struct itimerspec *value, struct itimerspec *oldvalue)
>> +{
>> + return (int) syscall4(__SC_TIMER_SETTIME,
>> + (long) timerid,
>> + (long) flags,
>> + (long) value,
>> + (long) oldvalue);
>> +
>> +}
>> +
>> +static inline int sys_timer_delete(timer_t timerid)
>> +{
>> + return (int) syscall1(__SC_TIMER_DELETE,
>> + (long) timerid);
>> +}
>> +
>> #endif /* __SYSCALL_H__ */
>> diff --git a/plat/linuxu/include/linuxu/time.h
>> b/plat/linuxu/include/linuxu/time.h
>> new file mode 100644
>> index 0000000..c4c97e1
>> --- /dev/null
>> +++ b/plat/linuxu/include/linuxu/time.h
>> @@ -0,0 +1,45 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause */
>> +/*
>> + * Authors: Costin Lupu <costin.lupu@xxxxxxxxx>
>> + *
>> + * Copyright (c) 2018, NEC Europe Ltd., NEC Corporation. All rights
>> reserved.
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + *
>> + * 1. Redistributions of source code must retain the above copyright
>> + * notice, this list of conditions and the following disclaimer.
>> + * 2. Redistributions in binary form must reproduce the above copyright
>> + * notice, this list of conditions and the following disclaimer in
>> the
>> + * documentation and/or other materials provided with the
>> distribution.
>> + * 3. Neither the name of the copyright holder nor the names of its
>> + * contributors may be used to endorse or promote products derived
>> from
>> + * this software without specific prior written permission.
>> + *
>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
>> CONTRIBUTORS "AS IS"
>> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
>> TO, THE
>> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
>> PURPOSE
>> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR
>> CONTRIBUTORS BE
>> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
>> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
>> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
>> BUSINESS
>> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
>> WHETHER IN
>> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
>> OTHERWISE)
>> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
>> ADVISED OF THE
>> + * POSSIBILITY OF SUCH DAMAGE.
>> + *
>> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
>> + */
>> +#ifndef __LINUXU_TIME_H__
>> +#define __LINUXU_TIME_H__
>> + > +#define TIMER_INTVAL_MSEC 10
>
> Missing include of signal.h
Right, more than that the current patch should come *after* the patch
adding interrupts support in linuxu.
>> +#define TIMER_SIGNUM SIGALRM
>> +
>> +
>> +/* POSIX definitions */
>> +
>> +#define CLOCK_REALTIME 0
>> +
>> +#endif /* __LINUXU_TIME_H__ */
>> diff --git a/plat/linuxu/time.c b/plat/linuxu/time.c
>> index 8a95ab5..e1fae2c 100644
>> --- a/plat/linuxu/time.c
>> +++ b/plat/linuxu/time.c
>> @@ -1,6 +1,7 @@
>> /* SPDX-License-Identifier: BSD-3-Clause */
>> /*
>> * Authors: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
>> + * Costin Lupu <costin.lupu@xxxxxxxxx>
>> *
>> * Copyright (c) 2017, NEC Europe Ltd., NEC Corporation. All rights
>> reserved.
>> *
>> @@ -32,13 +33,62 @@
>> * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
>> */
>> +#include <string.h>
>> #include <uk/plat/time.h>
>> +#include <uk/plat/irq.h>
>> +#include <uk/assert.h>
>> +#include <linuxu/syscall.h>
>> +#include <linuxu/time.h>
>> -void ukplat_time_init(void)
>> +#define TIMER_INTVAL_NSEC ukarch_time_msec_to_nsec(TIMER_INTVAL_MSEC)
>> +
>> +static timer_t timerid;
>> +
>> +
>> +__nsec ukplat_monotonic_clock(void)
>> {
>> /* TODO */
>> + return 0;
>> +}
>> +
>> +static int timer_handler(void *arg __unused)
>> +{
>> + /* We only use the timer interrupt to wake up. As we end up here,
>> the
>> + * timer interrupt has already done its job and we can acknowledge
>> + * receiving it.
>> + */
>> + return 1;
>> +}
>> +
>> +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.
>> + sigev.sigev_signo = TIMER_SIGNUM;
>> + sigev.sigev_value.sival_ptr = &timerid;
>> +
>
>
> Since we are working on time intervals, should we not use CLOCK_MONOTONIC?
>> + rc = sys_timer_create(CLOCK_REALTIME, &sigev, &timerid);
>> + if (unlikely(rc != 0))
>> + UK_CRASH("Failed to create timer: %d\n", rc);
>> +
>> + /* Initial expiration */
>> + its.it_value.tv_sec = TIMER_INTVAL_NSEC /
>> ukarch_time_sec_to_nsec(1);
>> + its.it_value.tv_nsec = TIMER_INTVAL_NSEC %
>> ukarch_time_sec_to_nsec(1);
>> + /* Timer interval */
>> + its.it_interval = its.it_value;
>> +
>> + rc = sys_timer_settime(timerid, 0, &its, NULL);
>> + if (unlikely(rc != 0))
>> + UK_CRASH("Failed to setup timer: %d\n", rc);
>> }
>> 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);
>> }
>>
>
> Thanks & Regards
> Sharan
>
> _______________________________________________
> Minios-devel mailing list
> Minios-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/minios-devel
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |