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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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