Hi Zenghui,
On Sat, Aug 14, 2021 at 2:10 AM Zenghui Yu <yuzenghui@xxxxxxxxxx> wrote:
>
> On 2021/8/14 5:12, Raghavendra Rao Ananta wrote:
> > Add a minimalistic library support to access the virtual timers,
> > that can be used for simple timing functionalities, such as
> > introducing delays in the guest.
> >
> > Signed-off-by: Raghavendra Rao Ananta <rananta@xxxxxxxxxx>
> > ---
> > .../kvm/include/aarch64/arch_timer.h | 138 ++++++++++++++++++
> > 1 file changed, 138 insertions(+)
> > create mode 100644 tools/testing/selftests/kvm/include/aarch64/arch_timer.h
> >
> > diff --git a/tools/testing/selftests/kvm/include/aarch64/arch_timer.h b/tools/testing/selftests/kvm/include/aarch64/arch_timer.h
> > new file mode 100644
> > index 000000000000..e6144ab95348
> > --- /dev/null
> > +++ b/tools/testing/selftests/kvm/include/aarch64/arch_timer.h
> > @@ -0,0 +1,138 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * ARM Generic Interrupt Controller (GIC) specific defines
>
> This isn't GIC specific, but arch timer.
You are right. That's my bad.
>
>
> > + */
> > +
> > +#ifndef SELFTEST_KVM_ARCH_TIMER_H
> > +#define SELFTEST_KVM_ARCH_TIMER_H
> > +
> > +#include <linux/sizes.h>
>
> Do we really need it?
I must have left it from some code re-org. We don't need it anymore.
Will remove.
>
>
> > +
> > +#include "processor.h"
> > +
> > +enum arch_timer {
> > + VIRTUAL,
> > + PHYSICAL,
> > +};
> > +
> > +#define CTL_ENABLE (1 << 0)
> > +#define CTL_ISTATUS (1 << 2)
> > +#define CTL_IMASK (1 << 1)
>
> nitpick: Move CTL_IMASK before CTL_ISTATUS ?
Sure, that's cleaner!
>
>
> > +
> > +#define msec_to_cycles(msec) \
> > + (timer_get_cntfrq() * (uint64_t)(msec) / 1000)
> > +
> > +#define usec_to_cycles(usec) \
> > + (timer_get_cntfrq() * (uint64_t)(usec) / 1000000)
> > +
> > +#define cycles_to_usec(cycles) \
> > + ((uint64_t)(cycles) * 1000000 / timer_get_cntfrq())
> > +
> > +static inline uint32_t timer_get_cntfrq(void)
> > +{
> > + return read_sysreg(cntfrq_el0);
> > +}
> > +
> > +static inline uint64_t timer_get_cntct(enum arch_timer timer)
> > +{
> > + isb();
> > +
> > + switch (timer) {
> > + case VIRTUAL:
> > + return read_sysreg(cntvct_el0);
> > + case PHYSICAL:
> > + return read_sysreg(cntpct_el0);
> > + default:
> > + GUEST_ASSERT_1(0, timer);
> > + }
> > +
> > + /* We should not reach here */
> > + return 0;
> > +}
> > +
> > +static inline void timer_set_cval(enum arch_timer timer, uint64_t cval)
> > +{
> > + switch (timer) {
> > + case VIRTUAL:
> > + return write_sysreg(cntv_cval_el0, cval);
> > + case PHYSICAL:
> > + return write_sysreg(cntp_cval_el0, cval);
> > + default:
> > + GUEST_ASSERT_1(0, timer);
> > + }
> > +
> > + isb();
>
> ISB should be performed before 'return'. And the same for
> timer_set_{tval,ctl}.
Seems like a copy-paste error on my side. The timer_set_*() functions
shouldn't even have a 'return'. Thanks for catching this! Will fix it.
>
>
> Thanks,
> Zenghui
Regards,
Raghavendra
