Re: [PATCH] kvm: x86: reduce rtc 0x70 access vm-exit time

[Paolo Bonzini]

On 02/08/2017 17:24, Peng Hao wrote:
> some versions of windows guest access rtc frequently because of 
> rtc as system tick.guest access rtc like this: write register index 
> to 0x70, then write or read data from 0x71. writing 0x70 port is 
> just as index and do nothing else. So writing 0x70 is not necessory 
> to exit to userspace every time and caching rtc register index in kvm 
> can reduce VM-EXIT time.
> without my patch, get the vm-exit time of accessing rtc 0x70 using
> perf tools: (guest OS : windows 7 64bit)
> IO Port Access  Samples Samples%  Time%  Min Time  Max Time  Avg time
> 0x70:POUT        86     30.99%    74.59%   9us      29us    10.75us (+- 3.41%)
> 
> with my patch
> IO Port Access  Samples Samples%  Time%   Min Time  Max Time   Avg time
>  0x70:POUT       106    32.02%    29.47%    0us      10us     1.57us (+- 7.38%)
> 
> the patch is a part of optimizing rtc 0x70 port access.another is in
> qemu.
> 
> Signed-off-by: Peng Hao <peng.hao2@xxxxxxxxxx>
> Reviewed-by: Liu Yi <liu.yi24@xxxxxxxxxx>

Technically, bit 7 of port 0x70 disables NMI, but QEMU doesn't implement
that part so I guess that's fine.

How would userspace read the index?  Could you instead extend the
existing coalesced MMIO support to I/O ports?

Paolo

> ---
>  arch/x86/include/asm/kvm_host.h |   1 +
>  arch/x86/kvm/Makefile           |   2 +-
>  arch/x86/kvm/vrtc.c             | 101 ++++++++++++++++++++++++++++++++++++++++
>  arch/x86/kvm/vrtc.h             |  19 ++++++++
>  arch/x86/kvm/x86.c              |  15 ++++++
>  include/uapi/linux/kvm.h        |   2 +
>  6 files changed, 139 insertions(+), 1 deletion(-)
>  create mode 100644 arch/x86/kvm/vrtc.c
>  create mode 100644 arch/x86/kvm/vrtc.h
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 1588e9e..41fde27 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -761,6 +761,7 @@ struct kvm_arch {
>  	struct kvm_pic *vpic;
>  	struct kvm_ioapic *vioapic;
>  	struct kvm_pit *vpit;
> +	struct kvm_vrtc *vrtc;
>  	atomic_t vapics_in_nmi_mode;
>  	struct mutex apic_map_lock;
>  	struct kvm_apic_map *apic_map;
> diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile
> index 09d4b17..dfd4364 100644
> --- a/arch/x86/kvm/Makefile
> +++ b/arch/x86/kvm/Makefile
> @@ -13,7 +13,7 @@ kvm-$(CONFIG_KVM_ASYNC_PF)	+= $(KVM)/async_pf.o
>  
>  kvm-y			+= x86.o mmu.o emulate.o i8259.o irq.o lapic.o \
>  			   i8254.o ioapic.o irq_comm.o cpuid.o pmu.o mtrr.o \
> -			   hyperv.o page_track.o debugfs.o
> +			   hyperv.o page_track.o debugfs.o vrtc.o
>  
>  kvm-intel-y		+= vmx.o pmu_intel.o
>  kvm-amd-y		+= svm.o pmu_amd.o
> diff --git a/arch/x86/kvm/vrtc.c b/arch/x86/kvm/vrtc.c
> new file mode 100644
> index 0000000..c8b45c0
> --- /dev/null
> +++ b/arch/x86/kvm/vrtc.c
> @@ -0,0 +1,101 @@
> +#include <linux/kvm_host.h>
> +#include <linux/slab.h>
> +
> +#include "x86.h"
> +#include "vrtc.h"
> +
> +static inline struct kvm_vrtc *dev_to_vrtc(struct kvm_io_device *dev)
> +{
> +	return container_of(dev, struct kvm_vrtc, dev);
> +}
> +
> +static int vrtc_ioport_read(struct kvm_vcpu *vcpu,
> +			struct kvm_io_device *dev,
> +			gpa_t addr, int len, void *data)
> +{
> +	return 0xff;
> +}
> +
> +static int vrtc_ioport_write(struct kvm_vcpu *vcpu,
> +			struct kvm_io_device *dev,
> +			gpa_t addr, int len, const void *val)
> +{
> +	struct kvm_vrtc *vrtc = dev_to_vrtc(dev);
> +	unsigned char data = *(unsigned char *)val;
> +	int ret = -EOPNOTSUPP;
> +
> +	if (addr != KVM_VRTC_BASE_ADDRESS)
> +		return -EOPNOTSUPP;
> +
> +	spin_lock(&vrtc->lock);
> +	if (vrtc->prev_reg_index == 0xff) {
> +		vrtc->prev_reg_index = data;
> +		goto out;
> +	}
> +
> +	if (data == vrtc->prev_reg_index) {
> +		ret = 0;
> +		goto out;
> +	} else {
> +		vrtc->prev_reg_index = data;
> +		goto out;
> +	}
> +
> +out:
> +	spin_unlock(&vrtc->lock);
> +	return ret;
> +}
> +
> +static const struct kvm_io_device_ops vrtc_dev_ops = {
> +	.read     = vrtc_ioport_read,
> +	.write    = vrtc_ioport_write,
> +};
> +
> +struct kvm_vrtc *kvm_create_vrtc(struct kvm *kvm)
> +{
> +	struct kvm_vrtc *vrtc;
> +	int ret;
> +
> +	vrtc = kzalloc(sizeof(struct kvm_vrtc), GFP_KERNEL);
> +	if (!vrtc)
> +		return NULL;
> +
> +	spin_lock_init(&vrtc->lock);
> +	vrtc->kvm = kvm;
> +	kvm_vrtc_reset(vrtc);
> +	mutex_lock(&kvm->slots_lock);
> +
> +	kvm_iodevice_init(&vrtc->dev, &vrtc_dev_ops);
> +	ret = kvm_io_bus_register_dev(kvm, KVM_PIO_BUS, KVM_VRTC_BASE_ADDRESS,
> +				      1, &vrtc->dev);
> +	if (ret < 0)
> +		goto fail_register_vrtc;
> +
> +	mutex_unlock(&kvm->slots_lock);
> +	return vrtc;
> +
> +fail_register_vrtc:
> +	mutex_unlock(&kvm->slots_lock);
> +	kfree(vrtc);
> +	return NULL;
> +}
> +
> +void kvm_vrtc_reset(struct kvm_vrtc *vrtc)
> +{
> +	vrtc->prev_reg_index = 0xff;
> +}
> +
> +void kvm_vrtc_destroy(struct kvm *kvm)
> +{
> +	struct kvm_vrtc *vrtc = kvm->arch.vrtc;
> +
> +	if (!vrtc)
> +		return;
> +	mutex_lock(&kvm->slots_lock);
> +	kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS, &vrtc->dev);
> +	mutex_unlock(&kvm->slots_lock);
> +
> +	kvm->arch.vrtc = NULL;
> +	kfree(vrtc);
> +}
> +
> diff --git a/arch/x86/kvm/vrtc.h b/arch/x86/kvm/vrtc.h
> new file mode 100644
> index 0000000..ea434ce
> --- /dev/null
> +++ b/arch/x86/kvm/vrtc.h
> @@ -0,0 +1,19 @@
> +#ifndef __VRTC_H
> +#define __VRTC_H
> +
> +#include <kvm/iodev.h>
> +#include <linux/spinlock.h>
> +
> +struct kvm_vrtc {
> +	spinlock_t lock;
> +	struct kvm_io_device dev;
> +	struct kvm *kvm;
> +	unsigned char prev_reg_index;
> +};
> +
> +
> +#define KVM_VRTC_BASE_ADDRESS	    0x70
> +struct kvm_vrtc *kvm_create_vrtc(struct kvm *kvm);
> +void kvm_vrtc_destroy(struct kvm *kvm);
> +void kvm_vrtc_reset(struct kvm_vrtc *vrtc);
> +#endif
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 6c7266f..426cf82 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -29,6 +29,7 @@
>  #include "cpuid.h"
>  #include "pmu.h"
>  #include "hyperv.h"
> +#include "vrtc.h"
>  
>  #include <linux/clocksource.h>
>  #include <linux/interrupt.h>
> @@ -4033,6 +4034,19 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  		mutex_unlock(&kvm->lock);
>  		break;
>  	}
> +	case KVM_CREATE_VRTC:
> +		mutex_lock(&kvm->lock);
> +		r = -EEXIST;
> +		if (kvm->arch.vrtc)
> +			goto create_vrtc_unlock;
> +
> +		r = -ENOMEM;
> +		kvm->arch.vrtc = kvm_create_vrtc(kvm);
> +		if (kvm->arch.vrtc)
> +			r = 0;
> +create_vrtc_unlock:
> +		mutex_unlock(&kvm->lock);
> +		break;
>  	case KVM_CREATE_PIT:
>  		u.pit_config.flags = KVM_PIT_SPEAKER_DUMMY;
>  		goto create_pit;
> @@ -8168,6 +8182,7 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>  	if (kvm_x86_ops->vm_destroy)
>  		kvm_x86_ops->vm_destroy(kvm);
>  	kvm_pic_destroy(kvm);
> +	kvm_vrtc_destroy(kvm);
>  	kvm_ioapic_destroy(kvm);
>  	kvm_free_vcpus(kvm);
>  	kvfree(rcu_dereference_check(kvm->arch.apic_map, 1));
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index c0b6dfe..ce503f35 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -927,6 +927,7 @@ struct kvm_ppc_resize_hpt {
>  #define KVM_CAP_S390_CMMA_MIGRATION 145
>  #define KVM_CAP_PPC_FWNMI 146
>  #define KVM_CAP_PPC_SMT_POSSIBLE 147
> +#define KVM_CAP_VRTC 150
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> @@ -1256,6 +1257,7 @@ struct kvm_s390_ucas_mapping {
>  #define KVM_PPC_CONFIGURE_V3_MMU  _IOW(KVMIO,  0xaf, struct kvm_ppc_mmuv3_cfg)
>  /* Available with KVM_CAP_PPC_RADIX_MMU */
>  #define KVM_PPC_GET_RMMU_INFO	  _IOW(KVMIO,  0xb0, struct kvm_ppc_rmmu_info)
> +#define KVM_CREATE_VRTC           _IO(KVMIO,  0xba)
>  
>  /* ioctl for vm fd */
>  #define KVM_CREATE_DEVICE	  _IOWR(KVMIO,  0xe0, struct kvm_create_device)
>