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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 19/11/15 16:57, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>> Sent: 19 November 2015 16:07
>> To: Paul Durrant; xen-devel@lists.xenproject.org
>> 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.

The compiler is free to do anything it wishes when encountering
undefined behaviour, including crash the hypervisor.

Any undefined-behaviour which can be triggered by a guest action
warrants an XSA, because there is no telling what might happen.

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

The dirty mask is a "currently scheduled on" mask.

~Andrew