[PATCH 0/4] arm64/x86: KVM: Introduce KVM_CAP_STEAL_TIME

[PATCH 0/4] arm64/x86: KVM: Introduce KVM_CAP_STEAL_TIME

[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

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

-- 
2.25.4

[PATCH 1/4] KVM: Documentation minor fixups

[Andrew Jones]

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 Documentation/virt/kvm/api.rst | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index 426f94582b7a..9a12ea498dbb 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -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.
-- 
2.25.4

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

[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 <drjones@redhat.com>
---
 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(+)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index 9a12ea498dbb..05b1fdb88383 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -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"
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 90cb90561446..f6dca6d09952 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -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;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 00c88c2f34e4..ced6335e403e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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;
 	}
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 4fdf30316582..121fb29ac004 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -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
 
-- 
2.25.4

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

[Marc Zyngier]

Hi Andrew,

On 2020-06-19 19:46, Andrew Jones wrote:
> 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 :-(.

> 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 <drjones@redhat.com>
> ---
>  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(+)
> 
> diff --git a/Documentation/virt/kvm/api.rst 
> b/Documentation/virt/kvm/api.rst
> index 9a12ea498dbb..05b1fdb88383 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -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"
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 90cb90561446..f6dca6d09952 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -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;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 00c88c2f34e4..ced6335e403e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -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;
>  	}
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 4fdf30316582..121fb29ac004 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -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.
-- 
Jazz is not dead. It just smells funny...

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

[Steven Price]

On 22/06/2020 09:20, Marc Zyngier wrote:
> Hi Andrew,
> 
> On 2020-06-19 19:46, Andrew Jones wrote:
[...]
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index 4fdf30316582..121fb29ac004 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -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...

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

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

[Andrew Jones]

On Mon, Jun 22, 2020 at 09:20:02AM +0100, Marc Zyngier wrote:
> Hi Andrew,
> 
> On 2020-06-19 19:46, Andrew Jones wrote:
> > 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 :-(.

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.

> 
> > 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 <drjones@redhat.com>
> > ---
> >  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(+)
> > 
> > diff --git a/Documentation/virt/kvm/api.rst
> > b/Documentation/virt/kvm/api.rst
> > index 9a12ea498dbb..05b1fdb88383 100644
> > --- a/Documentation/virt/kvm/api.rst
> > +++ b/Documentation/virt/kvm/api.rst
> > @@ -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"
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index 90cb90561446..f6dca6d09952 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -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;
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 00c88c2f34e4..ced6335e403e 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -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;
> >  	}
> > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > index 4fdf30316582..121fb29ac004 100644
> > --- a/include/uapi/linux/kvm.h
> > +++ b/include/uapi/linux/kvm.h
> > @@ -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...

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

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

[Marc Zyngier]

On 2020-06-22 09:41, Andrew Jones wrote:
> On Mon, Jun 22, 2020 at 09:20:02AM +0100, Marc Zyngier wrote:
>> Hi Andrew,
>> 
>> On 2020-06-19 19:46, Andrew Jones wrote:
>> > 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 :-(.
> 
> 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?

>> 
>> > 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 <drjones@redhat.com>
>> > ---
>> >  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(+)
>> >
>> > diff --git a/Documentation/virt/kvm/api.rst
>> > b/Documentation/virt/kvm/api.rst
>> > index 9a12ea498dbb..05b1fdb88383 100644
>> > --- a/Documentation/virt/kvm/api.rst
>> > +++ b/Documentation/virt/kvm/api.rst
>> > @@ -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"
>> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> > index 90cb90561446..f6dca6d09952 100644
>> > --- a/arch/arm64/kvm/arm.c
>> > +++ b/arch/arm64/kvm/arm.c
>> > @@ -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;
>> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> > index 00c88c2f34e4..ced6335e403e 100644
>> > --- a/arch/x86/kvm/x86.c
>> > +++ b/arch/x86/kvm/x86.c
>> > @@ -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;
>> >  	}
>> > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> > index 4fdf30316582..121fb29ac004 100644
>> > --- a/include/uapi/linux/kvm.h
>> > +++ b/include/uapi/linux/kvm.h
>> > @@ -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...
> 
> 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.
-- 
Jazz is not dead. It just smells funny...

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

[Andrew Jones]

On Mon, Jun 22, 2020 at 10:51:47AM +0100, Marc Zyngier wrote:
> On 2020-06-22 09:41, Andrew Jones wrote:
> > On Mon, Jun 22, 2020 at 09:20:02AM +0100, Marc Zyngier wrote:
> > > Hi Andrew,
> > > 
> > > On 2020-06-19 19:46, Andrew Jones wrote:
> > > > 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 :-(.
> > 
> > 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?

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.

> 
> > > 
> > > > 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 <drjones@redhat.com>
> > > > ---
> > > >  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(+)
> > > >
> > > > diff --git a/Documentation/virt/kvm/api.rst
> > > > b/Documentation/virt/kvm/api.rst
> > > > index 9a12ea498dbb..05b1fdb88383 100644
> > > > --- a/Documentation/virt/kvm/api.rst
> > > > +++ b/Documentation/virt/kvm/api.rst
> > > > @@ -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"
> > > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > > > index 90cb90561446..f6dca6d09952 100644
> > > > --- a/arch/arm64/kvm/arm.c
> > > > +++ b/arch/arm64/kvm/arm.c
> > > > @@ -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;
> > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > > index 00c88c2f34e4..ced6335e403e 100644
> > > > --- a/arch/x86/kvm/x86.c
> > > > +++ b/arch/x86/kvm/x86.c
> > > > @@ -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;
> > > >  	}
> > > > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > > > index 4fdf30316582..121fb29ac004 100644
> > > > --- a/include/uapi/linux/kvm.h
> > > > +++ b/include/uapi/linux/kvm.h
> > > > @@ -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...
> > 
> > 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.

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

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

[Marc Zyngier]

On 2020-06-22 11:31, Andrew Jones wrote:
> On Mon, Jun 22, 2020 at 10:51:47AM +0100, Marc Zyngier wrote:
>> On 2020-06-22 09:41, Andrew Jones wrote:
>> > On Mon, Jun 22, 2020 at 09:20:02AM +0100, Marc Zyngier wrote:
>> > > Hi Andrew,
>> > >
>> > > On 2020-06-19 19:46, Andrew Jones wrote:
>> > > > 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 :-(.
>> >
>> > 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?
> 
> 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?

[...]

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

> 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.
-- 
Jazz is not dead. It just smells funny...

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

[Andrew Jones]

On Mon, Jun 22, 2020 at 11:39:49AM +0100, Marc Zyngier wrote:
> On 2020-06-22 11:31, Andrew Jones wrote:
> > On Mon, Jun 22, 2020 at 10:51:47AM +0100, Marc Zyngier wrote:
> > > On 2020-06-22 09:41, Andrew Jones wrote:
> > > > On Mon, Jun 22, 2020 at 09:20:02AM +0100, Marc Zyngier wrote:
> > > > > Hi Andrew,
> > > > >
> > > > > On 2020-06-19 19:46, Andrew Jones wrote:
> > > > > > 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 :-(.
> > > >
> > > > 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?
> > 
> > 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?

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.

> 
> [...]
> 
> > > 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.

I'll post just that patch now then.

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

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

[Andrew Jones]

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 tools/include/uapi/linux/kvm.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/tools/include/uapi/linux/kvm.h b/tools/include/uapi/linux/kvm.h
index fdd632c833b4..121fb29ac004 100644
--- a/tools/include/uapi/linux/kvm.h
+++ b/tools/include/uapi/linux/kvm.h
@@ -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
 
-- 
2.25.4

[PATCH 4/4] KVM: selftests: Use KVM_CAP_STEAL_TIME

[Andrew Jones]

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 tools/testing/selftests/kvm/steal_time.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/testing/selftests/kvm/steal_time.c b/tools/testing/selftests/kvm/steal_time.c
index fcc840088c91..704bd3ee799a 100644
--- a/tools/testing/selftests/kvm/steal_time.c
+++ b/tools/testing/selftests/kvm/steal_time.c
@@ -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);
 	}
-- 
2.25.4

Re: [PATCH 0/4] arm64/x86: KVM: Introduce KVM_CAP_STEAL_TIME

[Steven Price]

On 19/06/2020 19:46, Andrew Jones wrote:
> 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 <steven.price@arm.com>

Thanks,

Steve

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