Re: [PATCH v3] x86: svm: use kvm_fast_pio_in()

["Radim Krčmář" <rkrcmar@redhat.com>]

2015-03-02 15:02-0600, Joel Schopp:
> From: David Kaplan <David.Kaplan@amd.com>
> 
> We can make the in instruction go faster the same way the out instruction is
> already.

(How much faster do benchmarks run?)

> Changes from v2[Joel]:
> 	* changed rax from u32 to unsigned long
> 	* changed a couple return 0 to BUG_ON()
> 	* changed 8 to sizeof(new_rax)
> 	* added trace hook
> 	* removed redundant clearing of count
> Changes from v1[Joel]
> 	* Added kvm_fast_pio_in() implementation that was left out of v1
> 
> Signed-off-by: David Kaplan <David.Kaplan@amd.com>
> [extracted from larger unlrelated patch, forward ported, addressed reviews, tested]
> Signed-off-by: Joel Schopp <joel.schopp@amd.com>
> ---
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> @@ -5463,6 +5463,36 @@ 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)

(complete_fast_pio_in()?)

> +{
> +	unsigned long new_rax = kvm_register_read(vcpu, VCPU_REGS_RAX);

Shouldn't we handle writes in EAX differently than in AX and AL, because
of implicit zero extension.

> +
> +	BUG_ON(!vcpu->arch.pio.count);
> +	BUG_ON(vcpu->arch.pio.count * vcpu->arch.pio.size > sizeof(new_rax));

(Looking at it again, a check for 'vcpu->arch.pio.count == 1' would be
 sufficient.)

> +
> +	memcpy(&new_rax, vcpu, sizeof(new_rax));
> +	trace_kvm_pio(KVM_PIO_IN, vcpu->arch.pio.port, vcpu->arch.pio.size,
> +		      vcpu->arch.pio.count, vcpu->arch.pio_data);
> +	kvm_register_write(vcpu, VCPU_REGS_RAX, new_rax);
> +	vcpu->arch.pio.count = 0;

I think it is better to call emulator_pio_in_emulated directly, like

   	emulator_pio_in_out(&vcpu->arch.emulate_ctxt, vcpu->arch.pio.size,
   			vcpu->arch.pio.port, &new_rax, 1);
   	kvm_register_write(vcpu, VCPU_REGS_RAX, new_rax);

because we know that vcpu->arch.pio.count != 0.

Refactoring could avoid the weird vcpu->ctxt->vcpu conversion.
(A better name is always welcome.)

---
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 96a8333f3db0..d0e5b086f2e1 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4663,22 +4663,23 @@ static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
 	return 0;
 }
 
+static void emulator_complete_pio_in(struct kvm_vcpu *vcpu, int size,
+		unsigned short port, void *val, unsigned int count)
+{
+	memcpy(val, vcpu->arch.pio_data, size * count);
+	trace_kvm_pio(KVM_PIO_IN, port, size, count, vcpu->arch.pio_data);
+	vcpu->arch.pio.count = 0;
+}
+
 static int emulator_pio_in_emulated(struct x86_emulate_ctxt *ctxt,
 				    int size, unsigned short port, void *val,
 				    unsigned int count)
 {
 	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
-	int ret;
 
-	if (vcpu->arch.pio.count)
-		goto data_avail;
-
-	ret = emulator_pio_in_out(vcpu, size, port, val, count, true);
-	if (ret) {
-data_avail:
-		memcpy(val, vcpu->arch.pio_data, size * count);
-		trace_kvm_pio(KVM_PIO_IN, port, size, count, vcpu->arch.pio_data);
-		vcpu->arch.pio.count = 0;
+	if (vcpu->arch.pio.count ||
+	    emulator_pio_in_out(vcpu, size, port, val, count, true)) {
+		emulator_complete_pio_in(vcpu, size, port, val, count);
 		return 1;
 	}