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

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@zte.com.cn>
> Reviewed-by: Liu Yi <liu.yi24@zte.com.cn>

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)
> 

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

[Peng Hao]

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@zte.com.cn>
Reviewed-by: Liu Yi <liu.yi24@zte.com.cn>
---
 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)
-- 
1.8.3.1

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

[Paolo Bonzini]

On 02/08/2017 11:02, peng.hao2@zte.com.cn wrote:
> 
>> On 02/08/2017 10:45, peng.hao2@zte.com.cn wrote:
>> >>On 02/08/2017 17:24, Peng Hao wrote:
>> > coalesced MMIO is async and io port should be sync .  I don't need to
>> > coalese io port access.
> 
>> Coalesced MMIO access is resolved on the next read from a coalesced MMIO
>> address or the next write to a non-coalescecd MMIO address.  So it would
>> work perfectly for port 0x70.  You would have to modify QEMU to split
>> the RTC I/O ports into two MemoryRegions.
>
> ok , it sounds simpler. I will try it.

My suggestion is to repurpose the "pad" field of "struct
kvm_coalesced_mmio_zone" to be 0 for memory and -1 for I/O.  It should
be enough for an RFC.

Later we can look into how to handle backwards compatibility.

Thanks,

Paolo

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

[Paolo Bonzini]

On 02/08/2017 10:45, peng.hao2@zte.com.cn wrote:
>>On 02/08/2017 17:24, Peng Hao wrote:
> coalesced MMIO is async and io port should be sync .  I don't need to
> coalese io port access.

Coalesced MMIO access is resolved on the next read from a coalesced MMIO
address or the next write to a non-coalescecd MMIO address.  So it would
work perfectly for port 0x70.  You would have to modify QEMU to split
the RTC I/O ports into two MemoryRegions.

Paolo