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

[Jan Beulich <jbeulich@xxxxxxxx>]

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>

Jan

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