Re: [Xen-devel] [PATCH v3] x86/hvm/viridian: flush remote tlbs by hypercall

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 20 November 2015 12:27
> To: Paul Durrant
> Cc: Andrew Cooper; Ian Campbell; Ian Jackson; Stefano Stabellini; Wei Liu;
> xen-devel@xxxxxxxxxxxxxxxxxxxx; Keir (Xen.org)
> Subject: RE: [PATCH v3] x86/hvm/viridian: flush remote tlbs by hypercall
> 
> >>> On 20.11.15 at 10:15, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> >> Sent: 19 November 2015 17:09
> >> On 19/11/15 16:57, Paul Durrant wrote:
> >> >> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> >> >> Sent: 19 November 2015 16:07
> >> >> On 19/11/15 13:19, Paul Durrant wrote:
> >> >>> +        /*
> >> >>> +         * Since ASIDs have now been flushed it just remains to
> >> >>> +         * force any CPUs currently running target vCPUs out of non-
> >> >>> +         * root mode. It's possible that re-scheduling has taken place
> >> >>> +         * so we may unnecessarily IPI some CPUs.
> >> >>> +         */
> >> >>> +        if ( !cpumask_empty(pcpu_mask) )
> >> >>> +            flush_tlb_mask(pcpu_mask);
> >> >> Wouldn't it be easier to simply and input_params.vcpu_mask with
> >> >> d->vcpu_dirty_mask ?
> >> >>
> >
> > Actually I realise your original statement makes no sense anyway. There is
> > no such mask as d->vcpu_dirty_mask.There is d->domain_dirty_cpumask
> which is a
> > mask of *physical* CPUs, but since (as the name implies) input_params.
> > vcpu_mask is a mask of *virtual* CPUs then ANDing the two together
> would just
> > yield garbage.
> >
> >> > No, that may yield much too big a mask. All we need here is a mask of
> >> where the vcpus are running *now*, not everywhere they've been.
> >>
> >> The dirty mask is a "currently scheduled on" mask.
> >
> > No it's not. The comment in sched.h clearly states that
> domain_dirty_cpumask
> > is a "Bitmask of CPUs which are holding onto this domain's state" which is,
> > as I said before, essentially everywhere the domains vcpus have been
> > scheduled since the last time state was flushed. Since, in this case, I have
> > already invalidated ASIDs for all targeted virtual CPUs I don't need to IPI
> > that many physical CPUs, I only need the mask of where the virtual CPUs
> are
> > *currently* running. If one of the them gets descheduled before the IPI
> then
> > the IPI was unnecessary (but there is no low-cost way of determining or
> > preventing that).
> 
> While you can't "and" that mask into input_params.vcpu_mask,
> wouldn't using it allow you to avoid the scratch pCPU mask
> variable?
> 

I'm not sure. After flushing the ASIDs I guess I could start with the 
domain_dirty_cpumask and, remove non-running vcpus from it, and then use it as 
the flush mask. If I do that, I suppose I ought to reset the 
vcpu_dirty_vcpumask values too. Something like...

        for_each_vcpu ( currd, v )
        {
            if ( v->vcpu_id >= (sizeof(input_params.vcpu_mask) * 8) )
                break;

            if ( !((input_params.vcpu_mask >> v->vcpu_id) & 1) )
                continue;

            hvm_asid_flush_vcpu(v);
            cpumask_clear(v->vcpu_dirty_vcpumask);
            
            if ( v->is_running )
                cpumask_set_cpu(v->processor, v->vcpu_dirty_vcpumask);
            else
                cpumask_clear_cpu(v->processor, d->domain_dirty_vcpumask);
        }

        if ( !cpumask_empty(d->domain_dirty_vcpumask) )
            flush_tlb_mask(d->domain_dirty_vcpumask);

Maybe?

  Paul

> Jan


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