From mboxrd@z Thu Jan  1 00:00:00 1970
From: Joel Schopp <joel.schopp@amd.com>
Subject: Re: [PATCH v2] x86: svm: use kvm_fast_pio_in()
Date: Mon, 2 Mar 2015 14:57:50 -0600
Message-ID: <54F4CECE.7070806@amd.com>
References: <20150302164004.17276.77376.stgit@joelvmguard2.amd.com>
 <20150302184953.GB25123@potion.brq.redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Cc: Gleb Natapov <gleb@kernel.org>,
	Paolo Bonzini <pbonzini@redhat.com>, <kvm@vger.kernel.org>,
	David Kaplan <David.Kaplan@amd.com>,
	Joerg Roedel <joro@8bytes.org>, <linux-kernel@vger.kernel.org>,
	Borislav Petkov <bp@alien8.de>
To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= <rkrcmar@redhat.com>
Return-path: <linux-kernel-owner@vger.kernel.org>
In-Reply-To: <20150302184953.GB25123@potion.brq.redhat.com>
Sender: linux-kernel-owner@vger.kernel.org
List-Id: kvm.vger.kernel.org


>
>>   		return emulate_instruction(vcpu, 0) == EMULATE_DONE;
>>   
>>   	port = io_info >> 16;
>> @@ -1907,6 +1907,8 @@ static int io_interception(struct vcpu_svm *svm)
>>   	svm->next_rip = svm->vmcb->control.exit_info_2;
>>   	skip_emulated_instruction(&svm->vcpu);
>>   
>> +	if (in)
>> +		return kvm_fast_pio_in(vcpu, size, port);
>>   	return kvm_fast_pio_out(vcpu, size, port);
> (kvm_fast_pio() comes to mind.)
If you combined them you'd have to have an extra argument to say if it 
was in or out. You'd then have to have code to parse that.  I prefer 
this way.

>
>>   }
>>   
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index bd7a70b..089247c 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -5463,6 +5463,39 @@ int kvm_fast_pio_out(struct kvm_vcpu *vcpu, int size, unsigned short port)
>>   }
>>   EXPORT_SYMBOL_GPL(kvm_fast_pio_out);
>>   
>> +static int complete_fast_pio(struct kvm_vcpu *vcpu)
>> +{
>> +	u32 new_rax = kvm_register_read(vcpu, VCPU_REGS_RAX);
> u64.
Good call.  I'll use unsigned long like kvm_fast_pio_out() uses.

> arch/x86/kvm/x86.c
>
>> +
>> +	if (!vcpu->arch.pio.count)
>> +		return 0;
>> +	if (vcpu->arch.pio.count * vcpu->arch.pio.size > 8)
>> +		return 0;
> sizeof(new_rax).  (safer and easier to understand)
>
> Both should never happen in KVM code, BUG_ON().
Agreed on both counts.

>
>> +
>> +	memcpy(&new_rax, vcpu->arch.pio_data,
>> +	       vcpu->arch.pio.count * vcpu->arch.pio.size);
> Use emulator_pio_in_emulated() here, for code sharing.
> (We want to trace the read here too;  it could be better to split
>   the path from emulator_pio_in_emulated() first.)
I looked at pulling this out, it was a painful.  I'll add the trace hook.

>
>> +	kvm_register_write(vcpu, VCPU_REGS_RAX, new_rax);
>> +
>> +	vcpu->arch.pio.count = 0;
>> +	return 1;
>> +}
>> +
>> +int kvm_fast_pio_in(struct kvm_vcpu *vcpu, int size, unsigned short port)
>> +{
>> +	unsigned long val;
>> +	int ret = emulator_pio_in_emulated(&vcpu->arch.emulate_ctxt, size,
>> +					   port, &val, 1);
>> +
>> +	if (ret) {
>> +		kvm_register_write(vcpu, VCPU_REGS_RAX, val);
>> +		vcpu->arch.pio.count = 0;
> (emulator_pio_in_emulated() sets count to zero if it returns true.)
will remove = 0 line