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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 19 November 2015 10:18
> To: Paul Durrant
> Cc: Andrew Cooper; Ian Campbell; Wei Liu; Ian Jackson; Stefano Stabellini;
> xen-devel@xxxxxxxxxxxxxxxxxxxx; Keir (Xen.org)
> Subject: Re: [PATCH v1] x86/hvm/viridian: flush remote tlbs by hypercall
> 
> >>> On 19.11.15 at 10:43, <paul.durrant@xxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -2452,6 +2452,13 @@ int hvm_vcpu_initialise(struct vcpu *v)
> >      if ( rc != 0 )
> >          goto fail6;
> >
> > +    if ( is_viridian_domain(d) )
> > +    {
> > +        rc = viridian_init(v);
> 
> I think these would better be viridian_vcpu_{,de}init().
> 

Sure.

> > @@ -512,9 +521,25 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
> >      return 1;
> >  }
> >
> > +int viridian_init(struct vcpu *v)
> > +{
> > +    v->arch.hvm_vcpu.viridian.flush_cpumask = xmalloc(cpumask_t);
> 
> Why not alloc_cpumask_var()?
> 

Because I didn't know about it. Now I do :-)

> > +    case HvFlushVirtualAddressSpace:
> > +    case HvFlushVirtualAddressList:
> > +    {
> > +        struct page_info *page;
> > +        unsigned int page_off;
> > +        uint8_t *input_params;
> > +        uint64_t vcpu_mask;
> 
> Is there a built in limit to 64 CPUs in the Hyper-V spec?
> 

Yes. The spec. includes the following (in section 12.4.2):

"Future versions of the hypervisor may support more than 64 virtual processors 
per partition. In that case, a new field will be added to the flags value that 
allows the caller to define the "processor bank" to which the processor mask 
applies."

> > +        uint64_t flags;
> > +        cpumask_t *pcpu_mask = curr-
> >arch.hvm_vcpu.viridian.flush_cpumask;
> > +        struct vcpu *v;
> > +
> > +        /*
> > +         * See Microsoft Hypervisor Top Level Spec. sections 12.4.2
> > +         * and 12.4.3.
> > +         */
> > +        perfc_incr(mshv_flush);
> > +
> > +        /* These hypercalls should never use the fast-call convention. */
> > +        status = HV_STATUS_INVALID_PARAMETER;
> > +        if ( input.fast )
> > +            break;
> > +
> > +        /* Get input parameters. */
> > +        page = get_page_from_gfn(currd, paddr_to_pfn(input_params_gpa),
> > +                                 NULL, P2M_ALLOC);
> 
> Does this really need to be P2M_ALLOC?

I guess not. It won't be zero-filled so there's no chance of it being swept 
from under us.

> 
> > +        if ( !page )
> > +            break;
> > +
> > +        page_off = input_params_gpa & (PAGE_SIZE - 1);
> > +
> > +        input_params = __map_domain_page(page);
> > +        input_params += page_off;
> > +
> > +        /*
> > +         * Address space is ignored since we cannot restrict the flush
> > +         * to a particular guest CR3 value.
> > +         */
> > +
> > +        flags = *(uint64_t *)(input_params + 8);
> > +        vcpu_mask = *(uint64_t *)(input_params + 16);
> 
> What if one of these crosses the page boundary?
> 

Good point. I'd assumed from the spec. that they would not cross a boundary but 
reading again I can't see why I assumed that. I'll look at using 
copy_to/from_guest() instead.

> > +        input_params -= page_off;
> > +        unmap_domain_page(input_params);
> > +
> > +        put_page(page);
> > +
> > +        /*
> > +         * It is not clear from the spec. if we are supposed to
> > +         * include current virtual CPU in the set or not in this case,
> > +         * so err on the safe side.
> > +         */
> > +        if ( flags & HV_FLUSH_ALL_PROCESSORS )
> > +            vcpu_mask = ~0ul;
> > +
> > +        cpumask_clear(pcpu_mask);
> > +
> > +        /*
> > +         * For each specified virtual CPU flush all ASIDs to invalidate
> > +         * TLB entries the next time it is scheduled and then, if it
> > +         * is currently running, add its physical CPU to a mask of
> > +         * those which need to be interrupted to force a flush. (Since
> > +         * ASIDs have already been flushed they actually only need
> > +         * forcing out of non-root mode, but this is as good a way as
> > +         * any).
> > +         */
> > +        for_each_vcpu ( currd, v )
> > +        {
> > +            if ( !(vcpu_mask & (1ul << v->vcpu_id)) )
> > +                continue;
> > +
> > +            hvm_asid_flush_vcpu(v);
> > +            if ( v->is_running )
> > +                cpumask_set_cpu(v->processor, pcpu_mask);
> > +        }
> > +
> > +        if ( !cpumask_empty(pcpu_mask) )
> > +            flush_tlb_mask(pcpu_mask);
> 
> Is this correct when scheduling happens between calculating
> pcpu_mask and actually using it?
> 
> > --- a/xen/include/asm-x86/hvm/viridian.h
> > +++ b/xen/include/asm-x86/hvm/viridian.h
> > @@ -22,6 +22,7 @@ union viridian_apic_assist
> >  struct viridian_vcpu
> >  {
> >      union viridian_apic_assist apic_assist;
> > +    cpumask_var_t flush_cpumask;
> 
> This suggests you even introduce a build problem without using
> alloc_cpumask_var() above.
> 

Ok.

  Paul

> Jan


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