Skip to contentSkip to conversation

arm64/x86: KVM: Introduce KVM_CAP_STEAL_TIME

PATCH In review Andrew Jones 3 participants 13 messages Jun 19, 2020 – Jun 22, 2020

Subsystems (7)
Documentation x86 KVM Kselftest +3 more subsystems
lore
Share & tools
lore

Andrew Jones

author
#0 lore raw

cover arm64/x86: KVM: Introduce KVM_CAP_STEAL_TIME

This series introduces a KVM CAP for steal time to arm64 and x86. For arm64 the cap resolves a couple issues described in the second patch's commit message. The cap isn't necessary for x86, but is added for consistency.

Thanks,
drew

Andrew Jones (4):
KVM: Documentation minor fixups
arm64/x86: KVM: Introduce steal time cap
tools headers kvm: Sync linux/kvm.h with the kernel sources
KVM: selftests: Use KVM_CAP_STEAL_TIME

 Documentation/virt/kvm/api.rst           | 20 ++++++++++++++++----
 arch/arm64/kvm/arm.c                     |  3 +++
 arch/x86/kvm/x86.c                       |  3 +++
 include/uapi/linux/kvm.h                 |  1 +
 tools/include/uapi/linux/kvm.h           | 15 +++++++++++++++
 tools/testing/selftests/kvm/steal_time.c |  8 ++++++++
 6 files changed, 46 insertions(+), 4 deletions(-)
Signature · 1 line
2.25.4
author
#1 lore raw

PATCH 1/4 KVM: Documentation minor fixups

Signed-off-by Andrew Jones <[email protected]>
---
 Documentation/virt/kvm/api.rst | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
Documentation/virt/kvm/api.rst +5 -4
index 426f94582b7a..9a12ea498dbb 100644
@@ -6121,7 +6121,7 @@ HvCallSendSyntheticClusterIpi, HvCallSendSyntheticClusterIpiEx.
 8.21 KVM_CAP_HYPERV_DIRECT_TLBFLUSH
 -----------------------------------
 
-:Architecture: x86
+:Architectures: x86
 
 This capability indicates that KVM running on top of Hyper-V hypervisor
 enables Direct TLB flush for its guests meaning that TLB flush

@@ -6134,16 +6134,17 @@ in CPUID and only exposes Hyper-V identification. In this case, guest
 thinks it's running on Hyper-V and only use Hyper-V hypercalls.
 
 8.22 KVM_CAP_S390_VCPU_RESETS
+-----------------------------
 
-Architectures: s390
+:Architectures: s390
 
 This capability indicates that the KVM_S390_NORMAL_RESET and
 KVM_S390_CLEAR_RESET ioctls are available.
 
 8.23 KVM_CAP_S390_PROTECTED
+---------------------------
 
-Architecture: s390
-
+:Architectures: s390
 
 This capability indicates that the Ultravisor has been initialized and
 KVM can therefore start protected VMs.
Signature · 1 line
2.25.4
author
#2 lore raw

PATCH 2/4 arm64/x86: KVM: Introduce steal time cap

In reply to Andrew Jones

arm64 requires a vcpu fd (KVM_HAS_DEVICE_ATTR vcpu ioctl) to probe support for steal time. However this is unnecessary and complicates userspace (userspace may prefer delaying vcpu creation until after feature probing). Since probing steal time only requires a KVM fd, we introduce a cap that can be checked. Additionally, when probing steal time we should check delayacct_on, because even though CONFIG_KVM selects TASK_DELAY_ACCT, it's possible for the host kernel to have delay accounting disabled with the 'nodelayacct' command line option. x86 already determines support for steal time by checking delayacct_on and can already probe steal time support with a kvm fd (KVM_GET_SUPPORTED_CPUID), but we add the cap there too for consistency.

Signed-off-by Andrew Jones <[email protected]>
---
 Documentation/virt/kvm/api.rst | 11 +++++++++++
 arch/arm64/kvm/arm.c           |  3 +++
 arch/x86/kvm/x86.c             |  3 +++
 include/uapi/linux/kvm.h       |  1 +
 4 files changed, 18 insertions(+)
Documentation/virt/kvm/api.rst +11 -0
index 9a12ea498dbb..05b1fdb88383 100644
@@ -6151,3 +6151,14 @@ KVM can therefore start protected VMs.
 This capability governs the KVM_S390_PV_COMMAND ioctl and the
 KVM_MP_STATE_LOAD MP_STATE. KVM_SET_MP_STATE can fail for protected
 guests when the state change is invalid.
+
+8.24 KVM_CAP_STEAL_TIME
+-----------------------
+
+:Architectures: arm64, x86
+
+This capability indicates that KVM supports steal time accounting.
+When steal time accounting is supported it may be enabled with
+architecture-specific interfaces.  For x86 see
+Documentation/virt/kvm/msr.rst "MSR_KVM_STEAL_TIME".  For arm64 see
+Documentation/virt/kvm/devices/vcpu.rst "KVM_ARM_VCPU_PVTIME_CTRL"
arch/arm64/kvm/arm.c +3 -0
index 90cb90561446..f6dca6d09952 100644
@@ -222,6 +222,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 		 */
 		r = 1;
 		break;
+	case KVM_CAP_STEAL_TIME:
+		r = sched_info_on();
+		break;
 	default:
 		r = kvm_arch_vm_ioctl_check_extension(kvm, ext);
 		break;
arch/x86/kvm/x86.c +3 -0
index 00c88c2f34e4..ced6335e403e 100644
@@ -3533,6 +3533,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_HYPERV_ENLIGHTENED_VMCS:
 		r = kvm_x86_ops.nested_ops->enable_evmcs != NULL;
 		break;
+	case KVM_CAP_STEAL_TIME:
+		r = sched_info_on();
+		break;
 	default:
 		break;
 	}
include/uapi/linux/kvm.h +1 -0
index 4fdf30316582..121fb29ac004 100644
@@ -1031,6 +1031,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_PPC_SECURE_GUEST 181
 #define KVM_CAP_HALT_POLL 182
 #define KVM_CAP_ASYNC_PF_INT 183
+#define KVM_CAP_STEAL_TIME 184
 
 #ifdef KVM_CAP_IRQ_ROUTING
Signature · 1 line
2.25.4
maintainer
#3 lore raw

Re: [PATCH 2/4] arm64/x86: KVM: Introduce steal time cap

Hi Andrew,

Quoted 5 lines · Andrew Jones

arm64 requires a vcpu fd (KVM_HAS_DEVICE_ATTR vcpu ioctl) to probe support for steal time. However this is unnecessary and complicates userspace (userspace may prefer delaying vcpu creation until after feature probing). Since probing steal time only requires a KVM fd, we introduce a cap that can be checked.

So this is purely an API convenience, right? You want a way to identify the presence of steal time accounting without having to create a vcpu? It would have been nice to have this requirement before we merged this code :-(.

Quoted 71 lines

Additionally, when probing
steal time we should check delayacct_on, because even though CONFIG_KVM selects TASK_DELAY_ACCT, it's possible for the host kernel to have delay accounting disabled with the 'nodelayacct' command line option. x86 already determines support for steal time by checking delayacct_on and can already probe steal time support with a kvm fd (KVM_GET_SUPPORTED_CPUID), but we add the cap there too for consistency.

Signed-off-by Andrew Jones <[email protected]>
---
 Documentation/virt/kvm/api.rst | 11 +++++++++++
 arch/arm64/kvm/arm.c           |  3 +++
 arch/x86/kvm/x86.c             |  3 +++
 include/uapi/linux/kvm.h       |  1 +
 4 files changed, 18 insertions(+)
Documentation/virt/kvm/api.rst +11 -0
index 9a12ea498dbb..05b1fdb88383 100644
@@ -6151,3 +6151,14 @@ KVM can therefore start protected VMs.
 This capability governs the KVM_S390_PV_COMMAND ioctl and the
 KVM_MP_STATE_LOAD MP_STATE. KVM_SET_MP_STATE can fail for protected
 guests when the state change is invalid.
+
+8.24 KVM_CAP_STEAL_TIME
+-----------------------
+
+:Architectures: arm64, x86
+
+This capability indicates that KVM supports steal time accounting.
+When steal time accounting is supported it may be enabled with
+architecture-specific interfaces.  For x86 see
+Documentation/virt/kvm/msr.rst "MSR_KVM_STEAL_TIME".  For arm64 see
+Documentation/virt/kvm/devices/vcpu.rst "KVM_ARM_VCPU_PVTIME_CTRL"
arch/arm64/kvm/arm.c +3 -0
index 90cb90561446..f6dca6d09952 100644
@@ -222,6 +222,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, 
 long ext)
 		 */
 		r = 1;
 		break;
+	case KVM_CAP_STEAL_TIME:
+		r = sched_info_on();
+		break;
 	default:
 		r = kvm_arch_vm_ioctl_check_extension(kvm, ext);
arch/arm64/kvm/arm.c +0 -0
 		break;
arch/x86/kvm/x86.c +3 -0
index 00c88c2f34e4..ced6335e403e 100644
@@ -3533,6 +3533,9 @@ int kvm_vm_ioctl_check_extension(struct kvm
 *kvm, long ext)
 	case KVM_CAP_HYPERV_ENLIGHTENED_VMCS:
 		r = kvm_x86_ops.nested_ops->enable_evmcs != NULL;
 		break;
+	case KVM_CAP_STEAL_TIME:
+		r = sched_info_on();
+		break;
 	default:
 		break;
arch/x86/kvm/x86.c +0 -0
 	}
include/uapi/linux/kvm.h +0 -0
index 4fdf30316582..121fb29ac004 100644

include/uapi/linux/kvm.h struct kvm_ppc_resize_hpt { · line 1036

Quoted 6 lines
include/uapi/linux/kvm.h +1 -0
@@ -1031,6 +1031,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_PPC_SECURE_GUEST 181
 #define KVM_CAP_HALT_POLL 182
 #define KVM_CAP_ASYNC_PF_INT 183
+#define KVM_CAP_STEAL_TIME 184
 
 #ifdef KVM_CAP_IRQ_ROUTING

Shouldn't you also add the same check of sched_info_on() to the various pvtime attribute handling functions? It feels odd that the capability can say "no", and yet we'd accept userspace messing with the steal time parameters...

Thanks,

        M.
Signature · 1 line
Jazz is not dead. It just smells funny...
#4 lore raw

Re: [PATCH 2/4] arm64/x86: KVM: Introduce steal time cap

Quoted 3 lines · Marc Zyngier

Hi Andrew,

On 2020-06-19 19:46, Andrew Jones wrote:

[...]

Quoted 10 lines
Quoted 10 lines
include/uapi/linux/kvm.h +1 -0
index 4fdf30316582..121fb29ac004 100644
@@ -1031,6 +1031,7 @@ struct kvm_ppc_resize_hpt {
  #define KVM_CAP_PPC_SECURE_GUEST 181
  #define KVM_CAP_HALT_POLL 182
  #define KVM_CAP_ASYNC_PF_INT 183
+#define KVM_CAP_STEAL_TIME 184
Quoted 6 lines

include/uapi/linux/kvm.h struct kvm_ppc_resize_hpt { · line 1036

Quoted 1 line
include/uapi/linux/kvm.h +0 -0
  #ifdef KVM_CAP_IRQ_ROUTING

Shouldn't you also add the same check of sched_info_on() to the various pvtime attribute handling functions? It feels odd that the capability can say "no", and yet we'd accept userspace messing with the steal time parameters...

My thought was that to some extent the two are separate. KVM_CAP_STEAL_TIME is "we have stolen time _AND_ it returns meaningful numbers", KVM_HAS_DEVICE_ATTR(KVM_ARM_VCPU_PVTIME_IPA) is (unfortunately) "we have the stolen interface, but it might not show how much time is stolen". Restoring a VM on a host with VCPU_PVTIME_IPA but without KVM_CAP_STEAL_TIME is possible just won't provide any stolen time data (the SMC calls will still work and the data format is valid).

Obviously with hindsight I would have done this differently...

Steve

author
#5 lore raw

Re: [PATCH 2/4] arm64/x86: KVM: Introduce steal time cap

In reply to Marc Zyngier
Quoted 9 lines · Marc Zyngier

Hi Andrew,

Quoted 5 lines · Andrew Jones

arm64 requires a vcpu fd (KVM_HAS_DEVICE_ATTR vcpu ioctl) to probe support for steal time. However this is unnecessary and complicates userspace (userspace may prefer delaying vcpu creation until after feature probing). Since probing steal time only requires a KVM fd, we introduce a cap that can be checked.

Quoted 4 lines

So this is purely an API convenience, right? You want a way to identify the presence of steal time accounting without having to create a vcpu? It would have been nice to have this requirement before we merged this code :-(.

Yes. I wish I had considered it more closely when I was reviewing the patches. And, I believe we have yet another user interface issue that I'm looking at now. Without the VCPU feature bit I'm not sure how easy it will be for a migration to fail when attempting to migrate from a host with steal-time enabled to one that does not support steal-time. So it's starting to look like steal-time should have followed the pmu pattern completely, not just the vcpu device ioctl part.

Quoted 78 lines
Quoted 78 lines

Additionally, when probing
steal time we should check delayacct_on, because even though CONFIG_KVM selects TASK_DELAY_ACCT, it's possible for the host kernel to have delay accounting disabled with the 'nodelayacct' command line option. x86 already determines support for steal time by checking delayacct_on and can already probe steal time support with a kvm fd (KVM_GET_SUPPORTED_CPUID), but we add the cap there too for consistency.

Signed-off-by Andrew Jones <[email protected]>
---
 Documentation/virt/kvm/api.rst | 11 +++++++++++
 arch/arm64/kvm/arm.c           |  3 +++
 arch/x86/kvm/x86.c             |  3 +++
 include/uapi/linux/kvm.h       |  1 +
 4 files changed, 18 insertions(+)
Documentation/virt/kvm/api.rst +11 -0
index 9a12ea498dbb..05b1fdb88383 100644
@@ -6151,3 +6151,14 @@ KVM can therefore start protected VMs.
 This capability governs the KVM_S390_PV_COMMAND ioctl and the
 KVM_MP_STATE_LOAD MP_STATE. KVM_SET_MP_STATE can fail for protected
 guests when the state change is invalid.
+
+8.24 KVM_CAP_STEAL_TIME
+-----------------------
+
+:Architectures: arm64, x86
+
+This capability indicates that KVM supports steal time accounting.
+When steal time accounting is supported it may be enabled with
+architecture-specific interfaces.  For x86 see
+Documentation/virt/kvm/msr.rst "MSR_KVM_STEAL_TIME".  For arm64 see
+Documentation/virt/kvm/devices/vcpu.rst "KVM_ARM_VCPU_PVTIME_CTRL"
arch/arm64/kvm/arm.c +3 -0
index 90cb90561446..f6dca6d09952 100644
@@ -222,6 +222,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm,
 long ext)
 		 */
 		r = 1;
 		break;
+	case KVM_CAP_STEAL_TIME:
+		r = sched_info_on();
+		break;
 	default:
 		r = kvm_arch_vm_ioctl_check_extension(kvm, ext);
arch/arm64/kvm/arm.c +0 -0
 		break;
arch/x86/kvm/x86.c +3 -0
index 00c88c2f34e4..ced6335e403e 100644
@@ -3533,6 +3533,9 @@ int kvm_vm_ioctl_check_extension(struct kvm
 *kvm, long ext)
 	case KVM_CAP_HYPERV_ENLIGHTENED_VMCS:
 		r = kvm_x86_ops.nested_ops->enable_evmcs != NULL;
 		break;
+	case KVM_CAP_STEAL_TIME:
+		r = sched_info_on();
+		break;
 	default:
 		break;
arch/x86/kvm/x86.c +0 -0
 	}
include/uapi/linux/kvm.h +1 -0
index 4fdf30316582..121fb29ac004 100644
@@ -1031,6 +1031,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_PPC_SECURE_GUEST 181
 #define KVM_CAP_HALT_POLL 182
 #define KVM_CAP_ASYNC_PF_INT 183
+#define KVM_CAP_STEAL_TIME 184
Quoted 6 lines

include/uapi/linux/kvm.h struct kvm_ppc_resize_hpt { · line 1036

Quoted 1 line
include/uapi/linux/kvm.h +0 -0
 #ifdef KVM_CAP_IRQ_ROUTING

Shouldn't you also add the same check of sched_info_on() to the various pvtime attribute handling functions? It feels odd that the capability can say "no", and yet we'd accept userspace messing with the steal time parameters...

I considered that, but the 'has attr' interface is really only asking if the interface exists, not if it should be used. I'm not sure what we should do about it other than document that the cap needs to say it's usable, rather than just the attr presence. But, since we've had the attr merged quite a while without the cap, then maybe we can't rely on a doc change alone?

Thanks,
drew

maintainer
#6 lore raw

Re: [PATCH 2/4] arm64/x86: KVM: Introduce steal time cap

Quoted 15 lines · Andrew Jones
Quoted 9 lines · Marc Zyngier

Hi Andrew,

Quoted 5 lines · Andrew Jones

arm64 requires a vcpu fd (KVM_HAS_DEVICE_ATTR vcpu ioctl) to probe support for steal time. However this is unnecessary and complicates userspace (userspace may prefer delaying vcpu creation until after feature probing). Since probing steal time only requires a KVM fd, we introduce a cap that can be checked.

Quoted 4 lines

So this is purely an API convenience, right? You want a way to identify the presence of steal time accounting without having to create a vcpu? It would have been nice to have this requirement before we merged this code :-(.

Quoted 7 lines

Yes. I wish I had considered it more closely when I was reviewing the patches. And, I believe we have yet another user interface issue that I'm looking at now. Without the VCPU feature bit I'm not sure how easy it will be for a migration to fail when attempting to migrate from a host with steal-time enabled to one that does not support steal-time. So it's starting to look like steal-time should have followed the pmu pattern completely, not just the vcpu device ioctl part.

Should we consider disabling steal time altogether until this is worked out?

Quoted 85 lines
Quoted 78 lines
Quoted 78 lines

Additionally, when probing
steal time we should check delayacct_on, because even though CONFIG_KVM selects TASK_DELAY_ACCT, it's possible for the host kernel to have delay accounting disabled with the 'nodelayacct' command line option. x86 already determines support for steal time by checking delayacct_on and can already probe steal time support with a kvm fd (KVM_GET_SUPPORTED_CPUID), but we add the cap there too for consistency.

Signed-off-by Andrew Jones <[email protected]>
---
 Documentation/virt/kvm/api.rst | 11 +++++++++++
 arch/arm64/kvm/arm.c           |  3 +++
 arch/x86/kvm/x86.c             |  3 +++
 include/uapi/linux/kvm.h       |  1 +
 4 files changed, 18 insertions(+)
Documentation/virt/kvm/api.rst +11 -0
index 9a12ea498dbb..05b1fdb88383 100644
@@ -6151,3 +6151,14 @@ KVM can therefore start protected VMs.
 This capability governs the KVM_S390_PV_COMMAND ioctl and the
 KVM_MP_STATE_LOAD MP_STATE. KVM_SET_MP_STATE can fail for protected
 guests when the state change is invalid.
+
+8.24 KVM_CAP_STEAL_TIME
+-----------------------
+
+:Architectures: arm64, x86
+
+This capability indicates that KVM supports steal time accounting.
+When steal time accounting is supported it may be enabled with
+architecture-specific interfaces.  For x86 see
+Documentation/virt/kvm/msr.rst "MSR_KVM_STEAL_TIME".  For arm64 see
+Documentation/virt/kvm/devices/vcpu.rst "KVM_ARM_VCPU_PVTIME_CTRL"
arch/arm64/kvm/arm.c +3 -0
index 90cb90561446..f6dca6d09952 100644
@@ -222,6 +222,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm,
 long ext)
 		 */
 		r = 1;
 		break;
+	case KVM_CAP_STEAL_TIME:
+		r = sched_info_on();
+		break;
 	default:
 		r = kvm_arch_vm_ioctl_check_extension(kvm, ext);
arch/arm64/kvm/arm.c +0 -0
 		break;
arch/x86/kvm/x86.c +3 -0
index 00c88c2f34e4..ced6335e403e 100644
@@ -3533,6 +3533,9 @@ int kvm_vm_ioctl_check_extension(struct kvm
 *kvm, long ext)
 	case KVM_CAP_HYPERV_ENLIGHTENED_VMCS:
 		r = kvm_x86_ops.nested_ops->enable_evmcs != NULL;
 		break;
+	case KVM_CAP_STEAL_TIME:
+		r = sched_info_on();
+		break;
 	default:
 		break;
arch/x86/kvm/x86.c +0 -0
 	}
include/uapi/linux/kvm.h +1 -0
index 4fdf30316582..121fb29ac004 100644
@@ -1031,6 +1031,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_PPC_SECURE_GUEST 181
 #define KVM_CAP_HALT_POLL 182
 #define KVM_CAP_ASYNC_PF_INT 183
+#define KVM_CAP_STEAL_TIME 184
Quoted 6 lines

include/uapi/linux/kvm.h struct kvm_ppc_resize_hpt { · line 1036

Quoted 1 line
include/uapi/linux/kvm.h +0 -0
 #ifdef KVM_CAP_IRQ_ROUTING

Shouldn't you also add the same check of sched_info_on() to the various pvtime attribute handling functions? It feels odd that the capability can say "no", and yet we'd accept userspace messing with the steal time parameters...

Quoted 6 lines

I considered that, but the 'has attr' interface is really only asking if the interface exists, not if it should be used. I'm not sure what we should do about it other than document that the cap needs to say it's usable, rather than just the attr presence. But, since we've had the attr merged quite a while without the cap, then maybe we can't rely on a doc change alone?

Accepting the pvtime attributes (setting up the per-vcpu area) has two
effects: we promise both the guest and userspace that we will provide the guest with steal time. By not checking sched_info_on(), we lie to both, with potential consequences. It really feels like a bug.

Thanks,

         M.
Signature · 1 line
Jazz is not dead. It just smells funny...
author
#7 lore raw

Re: [PATCH 2/4] arm64/x86: KVM: Introduce steal time cap

Quoted 25 lines · Marc Zyngier
Quoted 15 lines · Andrew Jones
Quoted 9 lines · Marc Zyngier

Hi Andrew,

Quoted 5 lines · Andrew Jones

arm64 requires a vcpu fd (KVM_HAS_DEVICE_ATTR vcpu ioctl) to probe support for steal time. However this is unnecessary and complicates userspace (userspace may prefer delaying vcpu creation until after feature probing). Since probing steal time only requires a KVM fd, we introduce a cap that can be checked.

Quoted 4 lines

So this is purely an API convenience, right? You want a way to identify the presence of steal time accounting without having to create a vcpu? It would have been nice to have this requirement before we merged this code :-(.

Quoted 8 lines

Yes. I wish I had considered it more closely when I was reviewing the patches. And, I believe we have yet another user interface issue that I'm looking at now. Without the VCPU feature bit I'm not sure how easy it will be for a migration to fail when attempting to migrate from a host
with steal-time enabled to one that does not support steal-time. So it's starting to look like steal-time should have followed the pmu pattern completely, not just the vcpu device ioctl part.

Quoted 1 line

Should we consider disabling steal time altogether until this is worked out?

I think we can leave it alone and just try to resolve it before merging QEMU patches (which I'm working on now). It doesn't look like kvmtool or rust-vmm (the only other two KVM userspaces I'm paying some attention to) do anything with steal-time yet, so they won't notice. And, I'm not sure disabling steal-time for any other userspaces is better than just trying to keep them working the best we can while improving the uapi.

Quoted 93 lines
Quoted 86 lines
Quoted 79 lines
Quoted 79 lines

Additionally, when probing
steal time we should check delayacct_on, because even though CONFIG_KVM selects TASK_DELAY_ACCT, it's possible for the host kernel to have delay accounting disabled with the 'nodelayacct' command line option. x86 already determines support for steal time by checking delayacct_on and can already probe steal time support with a kvm fd (KVM_GET_SUPPORTED_CPUID), but we add the cap there too for consistency.

Signed-off-by Andrew Jones <[email protected]>
---
 Documentation/virt/kvm/api.rst | 11 +++++++++++
 arch/arm64/kvm/arm.c           |  3 +++
 arch/x86/kvm/x86.c             |  3 +++
 include/uapi/linux/kvm.h       |  1 +
 4 files changed, 18 insertions(+)
Documentation/virt/kvm/api.rst +11 -0
index 9a12ea498dbb..05b1fdb88383 100644
@@ -6151,3 +6151,14 @@ KVM can therefore start protected VMs.
 This capability governs the KVM_S390_PV_COMMAND ioctl and the
 KVM_MP_STATE_LOAD MP_STATE. KVM_SET_MP_STATE can fail for protected
 guests when the state change is invalid.
+
+8.24 KVM_CAP_STEAL_TIME
+-----------------------
+
+:Architectures: arm64, x86
+
+This capability indicates that KVM supports steal time accounting.
+When steal time accounting is supported it may be enabled with
+architecture-specific interfaces.  For x86 see
+Documentation/virt/kvm/msr.rst "MSR_KVM_STEAL_TIME".  For arm64 see
+Documentation/virt/kvm/devices/vcpu.rst "KVM_ARM_VCPU_PVTIME_CTRL"
arch/arm64/kvm/arm.c +3 -0
index 90cb90561446..f6dca6d09952 100644
@@ -222,6 +222,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm,
 long ext)
 		 */
 		r = 1;
 		break;
+	case KVM_CAP_STEAL_TIME:
+		r = sched_info_on();
+		break;
 	default:
 		r = kvm_arch_vm_ioctl_check_extension(kvm, ext);
arch/arm64/kvm/arm.c +0 -0
 		break;
arch/x86/kvm/x86.c +3 -0
index 00c88c2f34e4..ced6335e403e 100644
@@ -3533,6 +3533,9 @@ int kvm_vm_ioctl_check_extension(struct kvm
 *kvm, long ext)
 	case KVM_CAP_HYPERV_ENLIGHTENED_VMCS:
 		r = kvm_x86_ops.nested_ops->enable_evmcs != NULL;
 		break;
+	case KVM_CAP_STEAL_TIME:
+		r = sched_info_on();
+		break;
 	default:
 		break;
arch/x86/kvm/x86.c +0 -0
 	}
include/uapi/linux/kvm.h +1 -0
index 4fdf30316582..121fb29ac004 100644
@@ -1031,6 +1031,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_PPC_SECURE_GUEST 181
 #define KVM_CAP_HALT_POLL 182
 #define KVM_CAP_ASYNC_PF_INT 183
+#define KVM_CAP_STEAL_TIME 184
Quoted 6 lines

include/uapi/linux/kvm.h struct kvm_ppc_resize_hpt { · line 1036

Quoted 1 line
include/uapi/linux/kvm.h +0 -0
 #ifdef KVM_CAP_IRQ_ROUTING

Shouldn't you also add the same check of sched_info_on() to the various pvtime attribute handling functions? It feels odd that the capability can say "no", and yet we'd accept userspace messing with the steal time parameters...

Quoted 6 lines

I considered that, but the 'has attr' interface is really only asking if the interface exists, not if it should be used. I'm not sure what we should do about it other than document that the cap needs to say it's usable, rather than just the attr presence. But, since we've had the attr merged quite a while without the cap, then maybe we can't rely on a doc change alone?

Quoted 4 lines

Accepting the pvtime attributes (setting up the per-vcpu area) has two
effects: we promise both the guest and userspace that we will provide the guest with steal time. By not checking sched_info_on(), we lie to both, with potential consequences. It really feels like a bug.

Yes, I agree now. Again, following the pmu pattern looks best here. The pmu will report that it doesn't have the attr support when its underlying kernel support (perf counters) doesn't exist. That's a direct analogy with steal-time relying on sched_info_on().

I'll work up another version of this series doing that, but before posting I'll look at the migration issue a bit more and likely post something for that as well.

Thanks,
drew

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

maintainer
#8 lore raw

Re: [PATCH 2/4] arm64/x86: KVM: Introduce steal time cap

Quoted 26 lines · Andrew Jones
Quoted 25 lines · Marc Zyngier
Quoted 23 lines · Andrew Jones
Quoted 9 lines · Marc Zyngier

Hi Andrew,

Quoted 5 lines · Andrew Jones

arm64 requires a vcpu fd (KVM_HAS_DEVICE_ATTR vcpu ioctl) to probe support for steal time. However this is unnecessary and complicates userspace (userspace may prefer delaying vcpu creation until after feature probing). Since probing steal time only requires a KVM fd, we introduce a cap that can be checked.

Quoted 4 lines

So this is purely an API convenience, right? You want a way to identify the presence of steal time accounting without having to create a vcpu? It would have been nice to have this requirement before we merged this code :-(.

Yes. I wish I had considered it more closely when I was reviewing the patches. And, I believe we have yet another user interface issue that I'm looking at now. Without the VCPU feature bit I'm not sure how easy it will be for a migration to fail when attempting to migrate from a host
with steal-time enabled to one that does not support steal-time. So it's starting to look like steal-time should have followed the pmu pattern completely, not just the vcpu device ioctl part.

Quoted 8 lines
Quoted 1 line

Should we consider disabling steal time altogether until this is worked out?

I think we can leave it alone and just try to resolve it before merging QEMU patches (which I'm working on now). It doesn't look like kvmtool or rust-vmm (the only other two KVM userspaces I'm paying some attention to) do anything with steal-time yet, so they won't notice. And, I'm not sure disabling steal-time for any other userspaces is better than just trying to keep them working the best we can while improving the uapi.

Is it only migration that is affected? Or do you see issues that would affect non-migrating userspace?

[...]

Quoted 9 lines
Quoted 4 lines

Accepting the pvtime attributes (setting up the per-vcpu area) has two
effects: we promise both the guest and userspace that we will provide the guest with steal time. By not checking sched_info_on(), we lie to both, with potential consequences. It really feels like a bug.

Yes, I agree now. Again, following the pmu pattern looks best here. The pmu will report that it doesn't have the attr support when its underlying kernel support (perf counters) doesn't exist. That's a direct analogy with steal-time relying on sched_info_on().

Indeed. I'd be happy to take a fix early if you can spin one.

Quoted 3 lines

I'll work up another version of this series doing that, but before posting I'll look at the migration issue a bit more and likely post something for that as well.

OK. I'll park this series for now.

Thanks,

        M.
Signature · 1 line
Jazz is not dead. It just smells funny...
author
#9 lore raw

Re: [PATCH 2/4] arm64/x86: KVM: Introduce steal time cap

Quoted 38 lines · Marc Zyngier
Quoted 29 lines · Andrew Jones
Quoted 25 lines · Marc Zyngier
Quoted 15 lines · Andrew Jones
Quoted 9 lines · Marc Zyngier

Hi Andrew,

Quoted 5 lines · Andrew Jones

arm64 requires a vcpu fd (KVM_HAS_DEVICE_ATTR vcpu ioctl) to probe support for steal time. However this is unnecessary and complicates userspace (userspace may prefer delaying vcpu creation until after feature probing). Since probing steal time only requires a KVM fd, we introduce a cap that can be checked.

Quoted 4 lines

So this is purely an API convenience, right? You want a way to identify the presence of steal time accounting without having to create a vcpu? It would have been nice to have this requirement before we merged this code :-(.

Quoted 8 lines

Yes. I wish I had considered it more closely when I was reviewing the patches. And, I believe we have yet another user interface issue that I'm looking at now. Without the VCPU feature bit I'm not sure how easy it will be for a migration to fail when attempting to migrate from a host
with steal-time enabled to one that does not support steal-time. So it's starting to look like steal-time should have followed the pmu pattern completely, not just the vcpu device ioctl part.

Quoted 2 lines

Should we consider disabling steal time altogether until this is worked out?

Quoted 7 lines

I think we can leave it alone and just try to resolve it before merging QEMU patches (which I'm working on now). It doesn't look like kvmtool or rust-vmm (the only other two KVM userspaces I'm paying some attention to)
do anything with steal-time yet, so they won't notice. And, I'm not sure disabling steal-time for any other userspaces is better than just trying to keep them working the best we can while improving the uapi.

Quoted 2 lines

Is it only migration that is affected? Or do you see issues that would affect non-migrating userspace?

If we don't consider the need to check sched_info_on(), then other than the probing requiring a vcpu fd (which isn't a show stopper, just not super convenient), then I don't see any other issues for non-migrating userspace.

Quoted 8 lines

[...]

Quoted 5 lines
Quoted 4 lines

Accepting the pvtime attributes (setting up the per-vcpu area) has two
effects: we promise both the guest and userspace that we will provide the guest with steal time. By not checking sched_info_on(), we lie to both, with potential consequences. It really feels like a bug.

Quoted 8 lines
Quoted 6 lines

Yes, I agree now. Again, following the pmu pattern looks best here. The pmu will report that it doesn't have the attr support when its underlying
kernel support (perf counters) doesn't exist. That's a direct analogy with
steal-time relying on sched_info_on().

Indeed. I'd be happy to take a fix early if you can spin one.

I'll post just that patch now then.

Quoted 8 lines
Quoted 6 lines

I'll work up another version of this series doing that, but before posting
I'll look at the migration issue a bit more and likely post something for
that as well.

OK. I'll park this series for now.

Thanks,
drew

author
#10 lore raw

PATCH 3/4 tools headers kvm: Sync linux/kvm.h with the kernel sources

In reply to Andrew Jones
Signed-off-by Andrew Jones <[email protected]>
---
 tools/include/uapi/linux/kvm.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
tools/include/uapi/linux/kvm.h +15 -0
index fdd632c833b4..121fb29ac004 100644
@@ -188,10 +188,13 @@ struct kvm_s390_cmma_log {
 struct kvm_hyperv_exit {
 #define KVM_EXIT_HYPERV_SYNIC          1
 #define KVM_EXIT_HYPERV_HCALL          2
+#define KVM_EXIT_HYPERV_SYNDBG         3
 	__u32 type;
+	__u32 pad1;
 	union {
 		struct {
 			__u32 msr;
+			__u32 pad2;
 			__u64 control;
 			__u64 evt_page;
 			__u64 msg_page;

@@ -201,6 +204,15 @@ struct kvm_hyperv_exit {
 			__u64 result;
 			__u64 params[2];
 		} hcall;
+		struct {
+			__u32 msr;
+			__u32 pad2;
+			__u64 control;
+			__u64 status;
+			__u64 send_page;
+			__u64 recv_page;
+			__u64 pending_page;
+		} syndbg;
 	} u;
 };
 

@@ -1017,6 +1029,9 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_S390_VCPU_RESETS 179
 #define KVM_CAP_S390_PROTECTED 180
 #define KVM_CAP_PPC_SECURE_GUEST 181
+#define KVM_CAP_HALT_POLL 182
+#define KVM_CAP_ASYNC_PF_INT 183
+#define KVM_CAP_STEAL_TIME 184
 
 #ifdef KVM_CAP_IRQ_ROUTING
Signature · 6 lines
2.25.4

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
author
#11 lore raw

PATCH 4/4 KVM: selftests: Use KVM_CAP_STEAL_TIME

In reply to Andrew Jones
Signed-off-by Andrew Jones <[email protected]>
---
 tools/testing/selftests/kvm/steal_time.c | 8 ++++++++
 1 file changed, 8 insertions(+)
tools/testing/selftests/kvm/steal_time.c +8 -0
index fcc840088c91..704bd3ee799a 100644
@@ -70,6 +70,10 @@ static void steal_time_init(struct kvm_vm *vm)
 		exit(KSFT_SKIP);
 	}
 
+#ifdef KVM_CAP_STEAL_TIME
+	TEST_ASSERT(kvm_check_cap(KVM_CAP_STEAL_TIME), "CAP doesn't match CPUID feature");
+#endif
+
 	for (i = 0; i < NR_VCPUS; ++i) {
 		int ret;
 

@@ -177,8 +181,12 @@ static void steal_time_init(struct kvm_vm *vm)
 	};
 	int i, ret;
 
+#ifndef KVM_CAP_STEAL_TIME
 	ret = _vcpu_ioctl(vm, 0, KVM_HAS_DEVICE_ATTR, &dev);
 	if (ret != 0 && errno == ENXIO) {
+#else
+	if (!kvm_check_cap(KVM_CAP_STEAL_TIME)) {
+#endif
 		print_skip("steal-time not supported");
 		exit(KSFT_SKIP);
 	}
Signature · 6 lines
2.25.4

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Reviewed-by on the v1 series
#12 lore raw
In reply to Andrew Jones
Quoted 7 lines · Andrew Jones

This series introduces a KVM CAP for steal time to arm64 and x86. For arm64 the cap resolves a couple issues described in the second patch's commit message. The cap isn't necessary for x86, but is added for consistency.

Thanks,
drew

LGTM, being able to probe whether nodelayacct has been specified makes sense and having it available before VCPU creation is even better. FWIW feel free to add:

Reviewed-by Steven Price <[email protected]>

Thanks,

Steve

Quoted 6 lines

Andrew Jones (4):
KVM: Documentation minor fixups
arm64/x86: KVM: Introduce steal time cap
tools headers kvm: Sync linux/kvm.h with the kernel sources
KVM: selftests: Use KVM_CAP_STEAL_TIME

Quoted 8 lines
  Documentation/virt/kvm/api.rst           | 20 ++++++++++++++++----
  arch/arm64/kvm/arm.c                     |  3 +++
  arch/x86/kvm/x86.c                       |  3 +++
  include/uapi/linux/kvm.h                 |  1 +
  tools/include/uapi/linux/kvm.h           | 15 +++++++++++++++
  tools/testing/selftests/kvm/steal_time.c |  8 ++++++++
  6 files changed, 46 insertions(+), 4 deletions(-)

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm