Re: [Minios-devel] [UNIKRAFT PATCHv4 23/43] plat/kvm: Add Arm64 virtual timer library to provide ticks

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien.grall@xxxxxxx>
> Sent: 2018年7月10日 20:55
> To: Wei Chen <Wei.Chen@xxxxxxx>; minios-devel@xxxxxxxxxxxxxxxxxxxx;
> simon.kuenzer@xxxxxxxxx
> Cc: Kaly Xin <Kaly.Xin@xxxxxxx>; nd <nd@xxxxxxx>
> Subject: Re: [Minios-devel] [UNIKRAFT PATCHv4 23/43] plat/kvm: Add Arm64
> virtual timer library to provide ticks
> 
> 
> 
> On 10/07/18 09:23, Wei Chen wrote:
> > Hi Julien,
> 
> Hi Wei,
> 
> >>> diff --git a/plat/kvm/arm/time.c b/plat/kvm/arm/time.c
> >>> new file mode 100644
> >>> index 0000000..ab4968f
> >>> --- /dev/null
> >>> +++ b/plat/kvm/arm/time.c
> >>
> >> The timer is generic on Arm. How about moving that to common arm64 code?
> >>
> >
> > I am re-implementing a timer library for Arm64 to support scheduler.
> > Current timer library just to provide ukplat_monotonic_clock to
> > make uk_printd can print timestamp.
> >
> > I will place the new library to common code.
> 
> You can probably place it in common Arm (32-bit and 64-bit) code.

Ok.

> 
> >
> >>> @@ -0,0 +1,127 @@
> >>> +/* SPDX-License-Identifier: BSD-3-Clause */
> >>
> >> Same remark as before for SPDX.
> >>
> >>> +/*
> >>> + * Authors: Wei Chen <Wei.Chen@xxxxxxx>
> >>> + *
> >>> + * Copyright (c) 2018, Arm Ltd. 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.
> >>> + */
> >>> +#include <stdlib.h>
> >>> +#include <uk/assert.h>
> >>> +#include <uk/plat/time.h>
> >>> +
> >>> +static uint64_t cntvct_at_init;
> >>
> >> How about boot_ticks here?
> >
> > Not a big problem, OK.
> >
> >>
> >>> +static uint32_t counter_freq;
> >>> +/*
> >>> + * Shift factor for TSC scaling multiplier; referred to as S in the
> >> following
> >>
> >> TSC has no meaning on Arm.
> >
> > Time Stamp Counter is a neutral word, I don't think it has obvious
> > architecture characteristic. But if you think the abbreviation TSC
> > is conflict/confused with x86's RDTSC, I would not use the abbreviation.
> 
> To be honest I never heard that word outside of x86 until your e-mail
> :). And wikipedia seems to disagree with you:
> 
> https://en.wikipedia.org/wiki/Time_Stamp_Counter
> 

Haa, yes, the WIKI said this word is original from x86 : )
OK, I will use ticks counter to replace the TSC. But I still think
Time Stamp Counter should be neutral, x86 should not monopolize
this word ; )

> [...]
> 
> >
> > I don't understand it either ; ) This comment existed in mini-os arm32
> > timer originally. And have been copied to lots of projects, I just copied
> > it from one of these projects.
> 
> My advice about Mini-OS arm32: Don't trust it! :)
> 

Yes, definitely!

> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel