Re: [PATCH RFC 3/3] xtf: add minimal HPET functionality test

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, Feb 23, 2018 at 07:07:18PM +0000, Wei Liu wrote:
> On Fri, Feb 23, 2018 at 01:27:43PM +0000, Roger Pau Monne wrote:
> > Add a basic HPET functionality test, note that this test requires the
> > HPET to support level triggered interrupts.
> > 
> > Further improvements should add support for interrupt delivery, and
> > testing all the available timers.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> >  arch/x86/include/arch/lib.h |  14 ++++
> >  docs/all-tests.dox          |   2 +
> >  tests/hpet/Makefile         |   9 +++
> >  tests/hpet/main.c           | 187 ++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 212 insertions(+)
> >  create mode 100644 tests/hpet/Makefile
> >  create mode 100644 tests/hpet/main.c
> > 
> > diff --git a/arch/x86/include/arch/lib.h b/arch/x86/include/arch/lib.h
> > index 6714bdc..3400890 100644
> > --- a/arch/x86/include/arch/lib.h
> > +++ b/arch/x86/include/arch/lib.h
> > @@ -392,6 +392,20 @@ static inline void write_xcr0(uint64_t xcr0)
> >      xsetbv(0, xcr0);
> >  }
> >  
> > +static inline uint64_t rdtsc(void)
> > +{
> > +    uint32_t low, high;
> > +
> > +    asm volatile ("rdtsc" : "=a" (low), "=d" (high));
> > +
> 
> You probably need to add lfence or mfence. See rdtsc_ordered in Xen.

Oh, OK that's news to me. I guess just using a lfence before it
should be fine.

> > +    return ((uint64_t)high << 32) | low;
> > +}
> > +
> [...]
> > +static void set_freq(void)
> > +{
> > +    uint32_t eax, ebx, ecx, edx, base;
> > +    bool found = false;
> > +
> > +    /* Get tsc frequency from cpuid. */
> > +    for ( base = XEN_CPUID_FIRST_LEAF;
> > +          base < XEN_CPUID_FIRST_LEAF + 0x10000; base += 0x100 )
> > +    {
> > +        cpuid(base, &eax, &ebx, &ecx, &edx);
> > +
> > +        if ( (ebx == XEN_CPUID_SIGNATURE_EBX) &&
> > +             (ecx == XEN_CPUID_SIGNATURE_ECX) &&
> > +             (edx == XEN_CPUID_SIGNATURE_EDX) &&
> > +             ((eax - base) >= 2) )
> > +        {
> > +            found = true;
> > +            break;
> > +        }
> > +    }
> > +
> > +    if ( !found )
> > +        panic("Unable to locate Xen CPUID leaves\n");
> > +
> 
> Finding Xen leaves should live in its own function and move to common
> code if possible.

There's already one in common code. The right way to fix this seems to
be to store 'base' and use it in further cpuid related calls.

> > +    cpuid_count(base + 3, 0, &eax, &ebx, &freq, &edx);
> > +    printk("TSC frequency %ukHz\n", freq);
> > +}
> > +
> > +/* Busy-wait implementation based on tsc value. */
> > +static void wait(unsigned int ms)
> > +{
> > +    uint64_t end = rdtsc() + (uint64_t)ms * (uint64_t)freq;
> > +
> > +    while ( rdtsc() < end )
> > +        pause();
> > +}
> 
> Rename to mdelay and move to a helper file?

OK, wasn't sure whether this would be helpful or not.

> > +
> > +void test_main(void)
> > +{
> [...]
> > +    HPET_REG(HPET_Tn_CFG(nr)) &= ~HPET_TN_LEVEL;
> > +    HPET_REG(HPET_STATUS) = 1 << nr;
> > +    wait(200);
> > +    if ( ((HPET_REG(HPET_STATUS) >> nr) & 1) )
> > +        return xtf_failure("Fail: Status bit set for edge interrupt in periodic mode\n");
> > +
> 
> Is it possible to use shorter time in the test? This test as-is will run
> for 1 or 2 seconds which is a bit long as micro-kernel testing.

Likely, I can reduce the periods and waits to something smaller. Let
me use a define and then we can tweak this, but using a 1ms period
should be fine.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel