Re: [Xen-devel] [PATCH v2 6/6] x86: implement Hyper-V clock source

[Wei Liu <wl@xxxxxxx>]

On Fri, Dec 20, 2019 at 05:05:24PM +0100, Jan Beulich wrote:
> On 18.12.2019 15:42, Wei Liu wrote:
> > --- a/xen/arch/x86/time.c
> > +++ b/xen/arch/x86/time.c
> > @@ -31,6 +31,7 @@
> >  #include <asm/processor.h>
> >  #include <asm/fixmap.h>
> >  #include <asm/guest.h>
> > +#include <asm/guest/hyperv-tlfs.h>
> 
> Can this please move ...
> 
> > @@ -644,6 +645,103 @@ static struct platform_timesource __initdata 
> > plt_xen_timer =
> >  };
> >  #endif
> >  
> > +#ifdef CONFIG_HYPERV_GUEST
> 
> ... here, to avoid the dependency on the header when the option is
> off?
> 
> > +/************************************************************
> > + * HYPER-V REFERENCE TSC
> > + */
> > +
> > +static struct ms_hyperv_tsc_page *hyperv_tsc;
> > +static struct page_info *hyperv_tsc_page;
> > +
> > +static int64_t __init init_hyperv_timer(struct platform_timesource *pts)
> > +{
> > +    paddr_t maddr;
> > +    uint64_t tsc_msr, freq;
> > +
> > +    if ( !(ms_hyperv.features & HV_MSR_REFERENCE_TSC_AVAILABLE) )
> > +        return 0;
> > +
> > +    hyperv_tsc_page = alloc_domheap_page(NULL, 0);
> > +    if ( !hyperv_tsc_page )
> > +        return 0;
> > +
> > +    hyperv_tsc = __map_domain_page_global(hyperv_tsc_page);
> > +    if ( !hyperv_tsc )
> > +    {
> > +        free_domheap_page(hyperv_tsc_page);
> > +        hyperv_tsc_page = NULL;
> > +        return 0;
> > +    }
> > +
> > +    maddr = page_to_maddr(hyperv_tsc_page);
> > +
> > +    /*
> > +     * Per Hyper-V TLFS:
> > +     *   1. Read existing MSR value
> > +     *   2. Preserve bits [11:1]
> > +     *   3. Set bits [63:12] to be guest physical address of tsc page
> > +     *   4. Set enabled bit (0)
> > +     *   5. Write back new MSR value
> > +     */
> > +    rdmsrl(HV_X64_MSR_REFERENCE_TSC, tsc_msr);
> > +    tsc_msr &= 0xffeULL;
> 
> There's no real need for the ULL suffix.
> 
> > +    tsc_msr |=  maddr | 1 /* enabled */;
> 
> Stray double blank after |= ?
> 
> > +    wrmsrl(HV_X64_MSR_REFERENCE_TSC, tsc_msr);
> > +
> > +    /* Get TSC frequency from Hyper-V */
> > +    rdmsrl(HV_X64_MSR_TSC_FREQUENCY, freq);
> > +    pts->frequency = freq;
> > +
> > +    return freq;
> > +}
> > +
> > +static inline uint64_t read_hyperv_timer(void)
> > +{
> > +    uint64_t scale, offset, ret, tsc;
> > +    uint32_t seq;
> > +    const struct ms_hyperv_tsc_page *tsc_page = hyperv_tsc;
> > +
> > +    do {
> > +        seq = tsc_page->tsc_sequence;
> > +
> > +        /* Seq 0 is special. It means the TSC enlightenment is not
> > +         * available at the moment. The reference time can only be
> > +         * obtained from the Reference Counter MSR.
> > +         */
> > +        if ( seq == 0 )
> > +        {
> > +            rdmsrl(HV_X64_MSR_TIME_REF_COUNT, ret);
> > +            return ret;
> > +        }
> > +
> > +        /* rdtsc_ordered already contains a load fence */
> > +        tsc = rdtsc_ordered();
> > +        scale = tsc_page->tsc_scale;
> > +        offset = tsc_page->tsc_offset;
> > +
> > +        smp_rmb();
> > +
> > +    } while (tsc_page->tsc_sequence != seq);
> > +
> > +    /* ret = ((tsc * scale) >> 64) + offset; */
> > +    asm ( "mul %[scale]; add %[offset], %[ret]"
> > +          : "+a" (tsc), [ret] "=d" (ret)
> 
> This needs to be "=&d", or else %rdx may be used to address
> %[offset] (when in memory).
> 
> With these taken care of
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> 

Thanks. I have addressed yours, Andrew's and Paul's comment (checking
HV_X64_ACCESS_FREQUENCY_MSRS) and will push patches that are needed for
this clock source to work (patch 1, 5 and 6).

Patches for cleaning up viridian code (2-4) will be posted separately
with Paul's comments addressed.

Wei.

> Jan

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