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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 25 September 2014 17:11
> To: Paul Durrant
> Cc: Christoph Egger; Ian Campbell; Ian Jackson; Stefano Stabellini; xen-
> devel@xxxxxxxxxxxxxxxxxxxx; Keir (Xen.org)
> Subject: Re: [PATCH] x86/viridian: Add Partition Reference Time
> enlightenment
> 
> >>> 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?

I guess that's debatable. It is possible to use this enlightenment to scale 
TSC, which means  that TSC emulation could be avoided even if the tsc_khz 
changes, but I still need to figure out how best to do that part. Nothing can 
be done in the case of migration to a non iTSC host though.
I could leave this out the default enlightenment set for now.

> 
> > +    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.

Ok.

> 
> > +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()

Indeed.

> 
> > @@ -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?
> 

Gah - don't know how that happened.

> > @@ -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()?
> 

Yes -  I guess I should dedup.

  Paul

> Jan


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