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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.)

> + */
> +
> +static struct ms_hyperv_tsc_page hyperv_tsc_page __aligned(PAGE_SIZE);

Does this need to be a statically allocated page?

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

paddr_t ?

> +    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?

> +        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.

> +    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?

> +    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.

> +        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.

> +    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).

Jan

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