[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2] x86/hvm/viridian: flush remote tlbs by hypercall
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@xxxxxxxx] > Sent: 19 November 2015 12:55 > To: Paul Durrant > Cc: Andrew Cooper; Ian Campbell; Wei Liu; Ian Jackson; Stefano Stabellini; > xen-devel@xxxxxxxxxxxxxxxxxxxx; Keir (Xen.org) > Subject: Re: [PATCH v2] x86/hvm/viridian: flush remote tlbs by hypercall > > >>> On 19.11.15 at 13:33, <paul.durrant@xxxxxxxxxx> wrote: > > + case HvFlushVirtualAddressSpace: > > + case HvFlushVirtualAddressList: > > + { > > + cpumask_var_t pcpu_mask; > > cpumask_t * (or else ... [skip next comment] > > > + struct vcpu *v; > > + > > + struct { > > Stray blank line. > I was going with style precedent within the same function but I can get rid of it. > > + 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; > > ... you pointlessly copy the contents here with low NR_CPUS. > cpumask_var_t is defined such that you can use it as rvalue > of an assignment to cpumask_t *. > Ok. Clearly I misunderstood the point of cpumask_var_t. > > + 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 ( !(input_params.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); > > Repeating my question you ignored on v1: Is this correct when > scheduling happens between calculating pcpu_mask and actually > using it? > I thought the comment above the loop explained it. The ASIDs are flushed first. The only requirement is that any running vcpu in the set is then pre-empted. The flush ensures this but by the time it is done it is possible that some of the IPIs have become unnecessary. > > + status = HV_STATUS_SUCCESS; > > + break; > > + } > > default: > > Perhaps time to insert blank lines between case blocks? > Ok. Paul > Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |