Re: [Xen-devel] [PATCH] x86/viridian: Add Partition Reference Time enlightenment

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.09.14 at 17:50, <paul.durrant@xxxxxxxxxx> wrote:
> The presence of the partition reference time enlightenment persuades newer
> versions of Windows to prefer the TSC as their primary time source. Hence,
> if rdtsc is not being emulated and is invariant then many vmexits (for
> alternative time sources such as the HPET or reference counter MSR) can
> be avoided.
> 
> If the VM is migrated to a host where TSC is no longer invariant and/or
> TSC is emulated then the enlightenment is disabled.

Which means performance of a guest post migration can change
dramatically. Is that really a good thing?

> +    do
> +        p->TscSequence++;
> +    while ( p->TscSequence == 0xFFFFFFFF ||
> +            p->TscSequence == 0 ); /* Avoid both 'invalid' values */

I don't think we ever use "naked" do/while constructs - please add
figure braces here.

> +static void initialize_reference_tsc(struct domain *d)
> +{
> +    unsigned long gmfn = 
> d->arch.hvm_domain.viridian.reference_tsc.fields.pfn;
> +    struct page_info *page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
> +    HV_REFERENCE_TSC_PAGE *p;
> +
> +    if ( !page || !get_page_type(page, PGT_writable_page) )
> +    {
> +        if ( page )
> +            put_page(page);
> +        gdprintk(XENLOG_WARNING, "Bad GMFN %lx (MFN %lx)\n", gmfn,
> +                 page ? page_to_mfn(page) : INVALID_MFN);
> +        return;
> +    }
> +
> +    p = __map_domain_page(page);
> +
> +    memset(p, 0, sizeof(*p));

clear_page()

> @@ -341,9 +433,12 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
>          *val = vlapic_get_reg(vcpu_vlapic(v), APIC_TASKPRI);
>          break;
>  
> -    case VIRIDIAN_MSR_APIC_ASSIST:
> -        perfc_incr(mshv_rdmsr_apic_msr);
> -        *val = v->arch.hvm_vcpu.viridian.apic_assist.raw;
> +    case VIRIDIAN_MSR_REFERENCE_TSC:
> +        if ( !(viridian_feature_mask(d) & HVMPV_reference_tsc) )
> +            return 0;
> +
> +        perfc_incr(mshv_rdmsr_tsc_msr);
> +        *val = d->arch.hvm_domain.viridian.reference_tsc.raw;
>          break;

Did you really mean to delete the VIRIDIAN_MSR_APIC_ASSIST code?

> @@ -460,11 +556,37 @@ static int viridian_load_domain_ctxt(struct domain *d, 
> hvm_domain_context_t *h)
>  {
>      struct hvm_viridian_domain_context ctxt;
>  
> -    if ( hvm_load_entry(VIRIDIAN_DOMAIN, h, &ctxt) != 0 )
> +    if ( hvm_load_entry_zeroextend(VIRIDIAN_DOMAIN, h, &ctxt) != 0 )
>          return -EINVAL;
>  
>      d->arch.hvm_domain.viridian.hypercall_gpa.raw = ctxt.hypercall_gpa;
>      d->arch.hvm_domain.viridian.guest_os_id.raw   = ctxt.guest_os_id;
> +    d->arch.hvm_domain.viridian.reference_tsc.raw = ctxt.reference_tsc;
> +
> +    if ( d->arch.hvm_domain.viridian.reference_tsc.fields.enabled )
> +    {
> +        unsigned long gmfn;
> +        struct page_info *page;
> +        HV_REFERENCE_TSC_PAGE *p;
> +
> +        gmfn = d->arch.hvm_domain.viridian.reference_tsc.fields.pfn;
> +        page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
> +
> +        if ( !page || !get_page_type(page, PGT_writable_page) )
> +        {
> +            if ( page )
> +                put_page(page);
> +            return -EINVAL;
> +        }
> +
> +        p = __map_domain_page(page);
> +
> +        update_reference_tsc(d, p);
> +
> +        unmap_domain_page(p);
> +
> +        put_page_and_type(page);
> +    }

Isn't this initialize_reference_tsc() without the memset()?

Jan


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