Re: [Xen-devel] [PATCH v2 3/3] x86/viridian: Add partition time reference counter MSR support

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.08.14 at 15:12, <paul.durrant@xxxxxxxxxx> wrote:
> +static s_time_t get_time_ref_count(struct domain *d)
> +{
> +    s_time_t now = get_s_time() / 100;
> +    struct viridian_time_ref_count *trc;
> +
> +    trc = &d->arch.hvm_domain.viridian.time_ref_count;
> +
> +    spin_lock(&trc->lock);

How well do you think this will scale, taking into consideration our
recent observations with certain Windows versions having all its
CPUs race for time stamps over apparently extended periods of
time? It would seem to me that a cmpxchg() based approach
might be preferable here.

> +
> +    if ( trc->last == 0 )
> +        gdprintk(XENLOG_G_INFO, "TIME_REFERENCE_COUNTER accessed\n");

As per the comments on patch 2, this needs to be either printk() or
the _G_ in the middle converted to just _.

> +
> +    if ( (now - trc->last) > 0 )
> +        trc->last = now;
> +    else
> +        now = ++trc->last;

And how well is that going to work (and match Windows'
expectations) when the migration destination host's NOW() is
significantly smaller than the source host's?

> @@ -344,6 +383,14 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
>          *val = v->arch.hvm_vcpu.viridian.apic_assist.raw;
>          break;
>  
> +    case VIRIDIAN_MSR_TIME_REF_COUNT:
> +        if ( !(viridian_feature_mask(d) & HVMPV_time_ref_count) )
> +            return 0;
> +
> +        perfc_incr(mshv_rdmsr_time_ref_count);
> +        *val = (uint64_t)get_time_ref_count(d);

Pointless cast.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel