Subject: Re: [PATCH] KVM: Hyper-V: do not do hypercall userspace exits if
 SynIC is disabled
To: Roman Kagan <rkagan@virtuozzo.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org, Andrey Smetanin <asmetanin@virtuozzo.com>
References: <1459244096-7213-1-git-send-email-pbonzini@redhat.com>
 <20160329104803.GA15614@rkaganb.sw.ru>
From: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <56FA5DDE.9030005@redhat.com>
Date: Tue, 29 Mar 2016 12:50:06 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.6.0
MIME-Version: 1.0
In-Reply-To: <20160329104803.GA15614@rkaganb.sw.ru>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2663
Lines: 77


On 29/03/2016 12:48, Roman Kagan wrote:
> On Tue, Mar 29, 2016 at 11:34:56AM +0200, Paolo Bonzini wrote:
>> If SynIC is disabled, there is nothing that userspace can do to
>> handle these exits; on the other hand, userspace probably will
>> not know about KVM_EXIT_HYPERV_HCALL and complain about it or
>> even exit.  Just prevent anything bad from happening by handling
>> the hypercall in KVM and returning an "invalid hypercall" code.
> 
> I wonder if this has been encountered in real life or just found by code
> inspection?

Code inspection; I was looking for an excus^Wreason to submit your patch
for hypercall stubs during QEMU hard freeze, and it occurred to me that
you'd need the stubs even on older QEMU releases.  This patch avoids
that regression.

Paolo

>> Fixes: 83326e43f27e9a8a501427a0060f8af519a39bb2
>> Cc: Andrey Smetanin <asmetanin@virtuozzo.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  arch/x86/kvm/hyperv.c | 17 ++++++++++-------
>>  1 file changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
>> index b960d9ea171f..9e2becd20a59 100644
>> --- a/arch/x86/kvm/hyperv.c
>> +++ b/arch/x86/kvm/hyperv.c
>> @@ -1237,14 +1237,17 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
>>  		break;
>>  	case HVCALL_POST_MESSAGE:
>>  	case HVCALL_SIGNAL_EVENT:
>> -		vcpu->run->exit_reason = KVM_EXIT_HYPERV;
>> -		vcpu->run->hyperv.type = KVM_EXIT_HYPERV_HCALL;
>> -		vcpu->run->hyperv.u.hcall.input = param;
>> -		vcpu->run->hyperv.u.hcall.params[0] = ingpa;
>> -		vcpu->run->hyperv.u.hcall.params[1] = outgpa;
>> -		vcpu->arch.complete_userspace_io =
>> +		if (vcpu_to_synic(vcpu)->active) {
>> +			vcpu->run->exit_reason = KVM_EXIT_HYPERV;
>> +			vcpu->run->hyperv.type = KVM_EXIT_HYPERV_HCALL;
>> +			vcpu->run->hyperv.u.hcall.input = param;
>> +			vcpu->run->hyperv.u.hcall.params[0] = ingpa;
>> +			vcpu->run->hyperv.u.hcall.params[1] = outgpa;
>> +			vcpu->arch.complete_userspace_io =
>>  				kvm_hv_hypercall_complete_userspace;
>> -		return 0;
>> +			return 0;
>> +		}
>> +		/* fall through */
> 
> I'd rather put it the other way around, with explicit error path and a
> comment:
> 
> 
> 		/* don't bother userspace if it has no way to handle it */
> 		if (!vcpu_to_synic(vcpu)->active) {
> 			res = HV_STATUS_INVALID_HYPERCALL_CODE;
> 			break;
> 		}
> 
> but that's just nitpicking, so
> 
> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>

Your version is better.

> [ Andrey has left the company so I'm not sure he even receives the
> messages sent to this address ]

He noticed and informed me.  I'll Cc both you and his gmail address in
the future.

Paolo