| Message ID | 20191025091618.10153-8-liuwe@microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Implement Hyper-V reference TSC based clock source | expand |
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
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
On 18.12.2019 13:38, Wei Liu wrote: > On Tue, Dec 10, 2019 at 05:59:04PM +0100, Jan Beulich wrote: >> On 25.10.2019 11:16, Wei Liu wrote: >>> +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; >>> + >>> + 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. Are you sure? My comment was rather towards the dropping of smp_rmb() (maybe replacing by a comment). >>> + 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. No, the hypervisor (at least the x86 one) doesn't link libgcc afaik. Jan
On 18/12/2019 12:51, Jan Beulich wrote: >>>> + 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. > No, the hypervisor (at least the x86 one) doesn't link libgcc afaik. x86 can multiply uint128_t by uint64_t without a library call. If this compiles, then it should be fine. ~Andrew
On Wed, Dec 18, 2019 at 12:56:41PM +0000, Andrew Cooper wrote: > On 18/12/2019 12:51, Jan Beulich wrote: > >>>> + 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. > > No, the hypervisor (at least the x86 one) doesn't link libgcc afaik. > > x86 can multiply uint128_t by uint64_t without a library call. If this > compiles, then it should be fine. This obviously compiles fine for me. But Jan's comment was more about making sure all possible x86 compilers can deal with it. I don't have a definitive answer to that. Wei. > > ~Andrew
On Wed, Dec 18, 2019 at 01:51:54PM +0100, Jan Beulich wrote: > On 18.12.2019 13:38, Wei Liu wrote: > > On Tue, Dec 10, 2019 at 05:59:04PM +0100, Jan Beulich wrote: > >> On 25.10.2019 11:16, Wei Liu wrote: > >>> +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; > >>> + > >>> + 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. > > Are you sure? My comment was rather towards the dropping of smp_rmb() > (maybe replacing by a comment). I do mean to keep smp_rmb() before it. Is that not enough? Wei.
On 18.12.2019 14:18, Wei Liu wrote: > On Wed, Dec 18, 2019 at 01:51:54PM +0100, Jan Beulich wrote: >> On 18.12.2019 13:38, Wei Liu wrote: >>> On Tue, Dec 10, 2019 at 05:59:04PM +0100, Jan Beulich wrote: >>>> On 25.10.2019 11:16, Wei Liu wrote: >>>>> +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; >>>>> + >>>>> + 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. >> >> Are you sure? My comment was rather towards the dropping of smp_rmb() >> (maybe replacing by a comment). > > I do mean to keep smp_rmb() before it. Is that not enough? With #define smp_rmb() barrier() it isn't - it's merely a compiler barrier, but for the ordering you want you need a fence. Jan
On 18.12.2019 13:56, Andrew Cooper wrote: > On 18/12/2019 12:51, Jan Beulich wrote: >>>>> + 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. >> No, the hypervisor (at least the x86 one) doesn't link libgcc afaik. > > x86 can multiply uint128_t by uint64_t without a library call. If this > compiles, then it should be fine. Hmm, a quick test proves what you say, but I'm uncertain relying on it is a good idea. Especially with -Os the compiler _should_ really emit a library call. Jan
On Wed, Dec 18, 2019 at 02:24:33PM +0100, Jan Beulich wrote: > On 18.12.2019 14:18, Wei Liu wrote: > > On Wed, Dec 18, 2019 at 01:51:54PM +0100, Jan Beulich wrote: > >> On 18.12.2019 13:38, Wei Liu wrote: > >>> On Tue, Dec 10, 2019 at 05:59:04PM +0100, Jan Beulich wrote: > >>>> On 25.10.2019 11:16, Wei Liu wrote: > >>>>> +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; > >>>>> + > >>>>> + 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. > >> > >> Are you sure? My comment was rather towards the dropping of smp_rmb() > >> (maybe replacing by a comment). > > > > I do mean to keep smp_rmb() before it. Is that not enough? > > With > > #define smp_rmb() barrier() > > it isn't - it's merely a compiler barrier, but for the ordering > you want you need a fence. Ah, I see. Thank you. Wei. > > Jan
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c index d8242295ef..f7e93b8a1f 100644 --- a/xen/arch/x86/time.c +++ b/xen/arch/x86/time.c @@ -30,6 +30,7 @@ #include <asm/processor.h> #include <asm/fixmap.h> #include <asm/guest.h> +#include <asm/guest/hyperv-tlfs.h> #include <asm/mc146818rtc.h> #include <asm/div64.h> #include <asm/acpi.h> @@ -614,6 +615,89 @@ static struct platform_timesource __initdata plt_xen_timer = }; #endif +#ifdef CONFIG_HYPERV_GUEST +/************************************************************ + * PLATFORM TIMER 6: HYPER-V REFERENCE TSC + */ + +static struct ms_hyperv_tsc_page hyperv_tsc_page __aligned(PAGE_SIZE); + +static int64_t __init init_hyperv_timer(struct platform_timesource *pts) +{ + unsigned long maddr; + uint64_t tsc_msr, freq; + + if ( !hyperv_guest || + !(ms_hyperv.features & HV_MSR_REFERENCE_TSC_AVAILABLE) ) + 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); + tsc_msr = tsc_msr | (uint64_t)maddr | 1 /* enabled */; + 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; + + 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(); + 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; + + 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, +}; +#endif + /************************************************************ * GENERIC PLATFORM TIMER INFRASTRUCTURE */ @@ -763,6 +847,9 @@ static u64 __init init_platform_timer(void) static struct platform_timesource * __initdata plt_timers[] = { #ifdef CONFIG_XEN_GUEST &plt_xen_timer, +#endif +#ifdef CONFIG_HYPERV_GUEST + &plt_hyperv_timer, #endif &plt_hpet, &plt_pmtimer, &plt_pit };
Implement a clock source using Hyper-V's reference TSC page. Signed-off-by: Wei Liu <liuwe@microsoft.com> --- Relevant spec: https://github.com/MicrosoftDocs/Virtualization-Documentation/raw/live/tlfs/Hypervisor%20Top%20Level%20Functional%20Specification%20v5.0C.pdf Section 12.6. --- xen/arch/x86/time.c | 87 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+)