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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> Sent: 19 November 2015 16:07
> To: Paul Durrant; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Ian Jackson; Stefano Stabellini; Ian Campbell; Wei Liu; Keir (Xen.org); 
> Jan
> Beulich
> Subject: Re: [PATCH v3] x86/hvm/viridian: flush remote tlbs by hypercall
> 
> On 19/11/15 13:19, Paul Durrant wrote:
> > @@ -561,10 +584,81 @@ int viridian_hypercall(struct cpu_user_regs *regs)
> >      switch ( input.call_code )
> >      {
> >      case HvNotifyLongSpinWait:
> > +        /*
> > +         * See Microsoft Hypervisor Top Level Spec. section 18.5.1.
> > +         */
> >          perfc_incr(mshv_call_long_wait);
> >          do_sched_op(SCHEDOP_yield, guest_handle_from_ptr(NULL, void));
> >          status = HV_STATUS_SUCCESS;
> >          break;
> > +
> > +    case HvFlushVirtualAddressSpace:
> > +    case HvFlushVirtualAddressList:
> > +    {
> > +        cpumask_t *pcpu_mask;
> > +        struct vcpu *v;
> > +        struct {
> > +            uint64_t address_space;
> > +            uint64_t flags;
> > +            uint64_t vcpu_mask;
> > +        } input_params;
> > +
> > +        /*
> > +         * 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. */
> > +        if ( hvm_copy_from_guest_phys(&input_params, input_params_gpa,
> > +                                      sizeof(input_params)) != 
> > HVMCOPY_okay )
> > +            break;
> > +
> > +        /*
> > +         * 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 ( input_params.flags & HV_FLUSH_ALL_PROCESSORS )
> > +            input_params.vcpu_mask = ~0ul;
> > +
> > +        pcpu_mask = curr->arch.hvm_vcpu.viridian.flush_cpumask;
> > +        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.
> > +         */
> > +        for_each_vcpu ( currd, v )
> > +        {
> > +            if ( !(input_params.vcpu_mask & (1ul << v->vcpu_id)) )
> > +                continue;
> 
> You need to cap this loop at a vcpu_id of 63, or the above conditional
> will become undefined.

The guest should not issue the hypercall if it has more than 64 vCPUs so to 
some extend I don't care what happens, as long as it is not harmful to the 
hypervisor in general, and I don't think that it is in this case.

> 
> It might also be wise to fail the vcpu_initialise() for a
> viridian-enabled domain having more than 64 vcpus.
> 
> > +
> > +            hvm_asid_flush_vcpu(v);
> > +            if ( v->is_running )
> > +                cpumask_set_cpu(v->processor, pcpu_mask);
> 
> __cpumask_set_cpu().  No need for atomic operations here.
> 

Ok.

> > +        }
> > +
> > +        /*
> > +         * 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 ?
> 

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.

> > +
> > +        status = HV_STATUS_SUCCESS;
> > +        break;
> > +    }
> > +
> >      default:
> >          status = HV_STATUS_INVALID_HYPERCALL_CODE;
> >          break;
> > diff --git a/xen/include/asm-x86/hvm/viridian.h b/xen/include/asm-
> x86/hvm/viridian.h
> > index c4319d7..2eec85e 100644
> > --- 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;
> >  };
> >
> >  union viridian_guest_os_id
> > @@ -117,6 +118,9 @@ viridian_hypercall(struct cpu_user_regs *regs);
> >  void viridian_time_ref_count_freeze(struct domain *d);
> >  void viridian_time_ref_count_thaw(struct domain *d);
> >
> > +int viridian_vcpu_init(struct vcpu *v);
> > +void viridian_vcpu_deinit(struct vcpu *v);
> > +
> >  #endif /* __ASM_X86_HVM_VIRIDIAN_H__ */
> >
> >  /*
> > diff --git a/xen/include/asm-x86/perfc_defn.h b/xen/include/asm-
> x86/perfc_defn.h
> > index 9ef092e..aac9331 100644
> > --- a/xen/include/asm-x86/perfc_defn.h
> > +++ b/xen/include/asm-x86/perfc_defn.h
> > @@ -115,6 +115,7 @@ PERFCOUNTER(mshv_call_sw_addr_space,    "MS
> Hv Switch Address Space")
> >  PERFCOUNTER(mshv_call_flush_tlb_list,   "MS Hv Flush TLB list")
> >  PERFCOUNTER(mshv_call_flush_tlb_all,    "MS Hv Flush TLB all")
> >  PERFCOUNTER(mshv_call_long_wait,        "MS Hv Notify long wait")
> > +PERFCOUNTER(mshv_call_flush,            "MS Hv Flush TLB")
> >  PERFCOUNTER(mshv_rdmsr_osid,            "MS Hv rdmsr Guest OS ID")
> >  PERFCOUNTER(mshv_rdmsr_hc_page,         "MS Hv rdmsr hypercall page")
> >  PERFCOUNTER(mshv_rdmsr_vp_index,        "MS Hv rdmsr vp index")
> > diff --git a/xen/include/public/hvm/params.h
> b/xen/include/public/hvm/params.h
> > index 356dfd3..5e54a84 100644
> > --- a/xen/include/public/hvm/params.h
> > +++ b/xen/include/public/hvm/params.h
> > @@ -1,3 +1,4 @@
> > +
> 
> Spurious change.

Yes, it is.

  Paul

> 
> ~Andrew

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