Re: [PATCH 04/10] KVM: arm64: selftests: Add basic support for arch_timers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux