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

Radim Krčmář
2015-03-03T16:42:36Z

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;
}