Re: [Xen-devel] [PATCH for-next 7/7] x86: implement Hyper-V clock source

[Wei Liu <wl@xxxxxxx>]

On Tue, Dec 10, 2019 at 05:59:04PM +0100, Jan Beulich wrote:
> On 25.10.2019 11:16, Wei Liu wrote:
> > @@ -614,6 +615,89 @@ static struct platform_timesource __initdata 
> > plt_xen_timer =
> >  };
> >  #endif
> >  
> > +#ifdef CONFIG_HYPERV_GUEST
> > +/************************************************************
> > + * PLATFORM TIMER 6: HYPER-V REFERENCE TSC
> 
> I don't think numbering is very helpful for optionally built code.
> (I realize though that this same anomaly exists for the Xen guest
> timer already.)

I will delete the numbering bit.

> 
> > + */
> > +
> > +static struct ms_hyperv_tsc_page hyperv_tsc_page __aligned(PAGE_SIZE);
> 
> Does this need to be a statically allocated page?
> 

At first I thought early_time_init was called before allocator has been
setup because arch_init_memory is called right after it.

Upon closer inspection I think that assumption was wrong. So yes I
should be able to just allocate a page from domheap here.

> > +static int64_t __init init_hyperv_timer(struct platform_timesource *pts)
> > +{
> > +    unsigned long maddr;
> 
> paddr_t ?
> 

Ack.

> > +    uint64_t tsc_msr, freq;
> > +
> > +    if ( !hyperv_guest ||
> > +         !(ms_hyperv.features & HV_MSR_REFERENCE_TSC_AVAILABLE) )
> 
> Is the hyperv_guest check really needed? The feature bit won't be
> set without that variable being true anyway, will it?
> 

Yes you're right.

> > +        return 0;
> > +
> > +    maddr = virt_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 &= GENMASK_ULL(11, 1);
> 
> A discussion not so long ago has resulted in, iirc, Andrew and me
> agreeing that in its current shape we don't want to see any uses
> of this macro outside of Arm-specific code.
> 

Fair enough. I will use 0xFFEul instead.

> > +    tsc_msr = tsc_msr | (uint64_t)maddr | 1 /* enabled */;
> 
> Why the cast? And maybe easier as "tsc_msr |= "?
> 
> > +    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;
> > +    struct ms_hyperv_tsc_page *tsc_page = &hyperv_tsc_page;
> 
> const?

Ack.

> 
> > +    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;
> > +        }
> > +
> > +        smp_rmb();
> > +
> > +        tsc = rdtsc_ordered();
> 
> This already includes at least a read fence.

OK. rdtsc() should be enough here.

> 
> > +        scale = tsc_page->tsc_scale;
> > +        offset = tsc_page->tsc_offset;
> > +
> > +        smp_rmb();
> > +
> > +    } while (tsc_page->tsc_sequence != seq);
> > +
> > +    /* x86 has ARCH_SUPPORTS_INT128 */
> > +    ret = (uint64_t)(((__uint128_t)tsc * scale) >> 64) + offset;
> 
> The final cast isn't really needed, is it? As to the multiplication
> - are you sure all compilers in all cases will avoid falling back
> to a library call here? In other similar places I think we use
> inline assembly instead.

What library call? A function in libgcc (or clang's equivalence)?
ISTR libgcc is always linked, but I could be wrong here.

I'm happy to change it to inline assembly though.

> 
> > +    return ret;
> > +}
> > +
> > +static struct platform_timesource __initdata plt_hyperv_timer =
> > +{
> > +    .id = "hyperv",
> > +    .name = "HYPER-V REFERENCE TSC",
> > +    .read_counter = read_hyperv_timer,
> > +    .init = init_hyperv_timer,
> > +    .counter_bits = 63,
> 
> Why 63? The calculation above is a uint64_t one. If there are
> wrapping concerns like for the TSC source, please add a
> respective comment (which may be as brief as a reference to
> the other one, if that's appropriate).

OK. I will add a comment to reference the previous comment.

Wei.

> 
> Jan

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