* [PATCH 1/5] KVM: arm64: pvtime: steal-time is only supported when configured
2020-07-11 10:04 [PATCH 0/5] KVM: arm64: pvtime: Fixes and a new cap Andrew Jones
@ 2020-07-11 10:04 ` Andrew Jones
2020-07-27 17:25 ` Marc Zyngier
2020-07-11 10:04 ` [PATCH 2/5] KVM: arm64: pvtime: Fix potential loss of stolen time Andrew Jones
` (7 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Andrew Jones @ 2020-07-11 10:04 UTC (permalink / raw)
To: kvm, kvmarm; +Cc: pbonzini, steven.price, maz
Don't confuse the guest by saying steal-time is supported when
it hasn't been configured by userspace and won't work.
Signed-off-by: Andrew Jones <drjones@redhat.com>
---
arch/arm64/kvm/pvtime.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/kvm/pvtime.c b/arch/arm64/kvm/pvtime.c
index f7b52ce1557e..2b22214909be 100644
--- a/arch/arm64/kvm/pvtime.c
+++ b/arch/arm64/kvm/pvtime.c
@@ -42,9 +42,12 @@ long kvm_hypercall_pv_features(struct kvm_vcpu *vcpu)
switch (feature) {
case ARM_SMCCC_HV_PV_TIME_FEATURES:
- case ARM_SMCCC_HV_PV_TIME_ST:
val = SMCCC_RET_SUCCESS;
break;
+ case ARM_SMCCC_HV_PV_TIME_ST:
+ if (vcpu->arch.steal.base != GPA_INVALID)
+ val = SMCCC_RET_SUCCESS;
+ break;
}
return val;
--
2.25.4
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] KVM: arm64: pvtime: steal-time is only supported when configured
2020-07-11 10:04 ` [PATCH 1/5] KVM: arm64: pvtime: steal-time is only supported when configured Andrew Jones
@ 2020-07-27 17:25 ` Marc Zyngier
2020-07-28 12:55 ` Andrew Jones
0 siblings, 1 reply; 20+ messages in thread
From: Marc Zyngier @ 2020-07-27 17:25 UTC (permalink / raw)
To: Andrew Jones; +Cc: pbonzini, kvmarm, kvm, steven.price
Hi Andrew,
On 2020-07-11 11:04, Andrew Jones wrote:
> Don't confuse the guest by saying steal-time is supported when
> it hasn't been configured by userspace and won't work.
>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
> arch/arm64/kvm/pvtime.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kvm/pvtime.c b/arch/arm64/kvm/pvtime.c
> index f7b52ce1557e..2b22214909be 100644
> --- a/arch/arm64/kvm/pvtime.c
> +++ b/arch/arm64/kvm/pvtime.c
> @@ -42,9 +42,12 @@ long kvm_hypercall_pv_features(struct kvm_vcpu
> *vcpu)
>
> switch (feature) {
> case ARM_SMCCC_HV_PV_TIME_FEATURES:
> - case ARM_SMCCC_HV_PV_TIME_ST:
> val = SMCCC_RET_SUCCESS;
> break;
> + case ARM_SMCCC_HV_PV_TIME_ST:
> + if (vcpu->arch.steal.base != GPA_INVALID)
> + val = SMCCC_RET_SUCCESS;
> + break;
> }
>
> return val;
I'm not so sure about this. I have always considered the
discovery interface to be "do you know about this SMCCC
function". And if you look at the spec, it says (4.2,
PV_TIME_FEATURES):
<quote>
If PV_call_id identifies PV_TIME_FEATURES, this call returns:
• NOT_SUPPORTED (-1) to indicate that all
paravirtualized time functions in this specification are not
supported.
• SUCCESS (0) to indicate that all the paravirtualized time
functions in this specification are supported.
</quote>
So the way I understand it, you cannot return "supported"
for PV_TIME_FEATURES, and yet return NOT_SUPPORTED for
PV_TIME_ST. It applies to *all* features.
Yes, this is very bizarre. But I don't think we can deviate
from it.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] KVM: arm64: pvtime: steal-time is only supported when configured
2020-07-27 17:25 ` Marc Zyngier
@ 2020-07-28 12:55 ` Andrew Jones
2020-07-28 13:13 ` Marc Zyngier
0 siblings, 1 reply; 20+ messages in thread
From: Andrew Jones @ 2020-07-28 12:55 UTC (permalink / raw)
To: Marc Zyngier; +Cc: pbonzini, kvmarm, kvm, steven.price
On Mon, Jul 27, 2020 at 06:25:50PM +0100, Marc Zyngier wrote:
> Hi Andrew,
>
> On 2020-07-11 11:04, Andrew Jones wrote:
> > Don't confuse the guest by saying steal-time is supported when
> > it hasn't been configured by userspace and won't work.
> >
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> > arch/arm64/kvm/pvtime.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/kvm/pvtime.c b/arch/arm64/kvm/pvtime.c
> > index f7b52ce1557e..2b22214909be 100644
> > --- a/arch/arm64/kvm/pvtime.c
> > +++ b/arch/arm64/kvm/pvtime.c
> > @@ -42,9 +42,12 @@ long kvm_hypercall_pv_features(struct kvm_vcpu *vcpu)
> >
> > switch (feature) {
> > case ARM_SMCCC_HV_PV_TIME_FEATURES:
> > - case ARM_SMCCC_HV_PV_TIME_ST:
> > val = SMCCC_RET_SUCCESS;
> > break;
> > + case ARM_SMCCC_HV_PV_TIME_ST:
> > + if (vcpu->arch.steal.base != GPA_INVALID)
> > + val = SMCCC_RET_SUCCESS;
> > + break;
> > }
> >
> > return val;
>
> I'm not so sure about this. I have always considered the
> discovery interface to be "do you know about this SMCCC
> function". And if you look at the spec, it says (4.2,
> PV_TIME_FEATURES):
>
> <quote>
> If PV_call_id identifies PV_TIME_FEATURES, this call returns:
> • NOT_SUPPORTED (-1) to indicate that all
> paravirtualized time functions in this specification are not
> supported.
> • SUCCESS (0) to indicate that all the paravirtualized time
> functions in this specification are supported.
> </quote>
>
> So the way I understand it, you cannot return "supported"
> for PV_TIME_FEATURES, and yet return NOT_SUPPORTED for
> PV_TIME_ST. It applies to *all* features.
>
> Yes, this is very bizarre. But I don't think we can deviate
> from it.
Ah, I see your point. But I wonder if we should drop this patch
or if we should change the return of ARM_SMCCC_HV_PV_TIME_FEATURES
to be dependant on all the pv calls?
Discovery would look like this
IF (SMCCC_ARCH_FEATURES, PV_TIME_FEATURES) == 0; THEN
IF (PV_TIME_FEATURES, PV_TIME_FEATURES) == 0; THEN
PV_TIME_ST is supported, as well as all other PV calls
ELIF (PV_TIME_FEATURES, PV_TIME_ST) == 0; THEN
PV_TIME_ST is supported
ELIF (PV_TIME_FEATURES, <another-pv-call>) == 0; THEN
<another-pv-call> is supported
...
ENDIF
ELSE
No PV calls are supported
ENDIF
I believe the above implements a reasonable interpretation of the
specification, but the all feature (PV_TIME_FEATURES, PV_TIME_FEATURES)
thing is indeed bizarre no matter how you look at it.
Thanks,
drew
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] KVM: arm64: pvtime: steal-time is only supported when configured
2020-07-28 12:55 ` Andrew Jones
@ 2020-07-28 13:13 ` Marc Zyngier
2020-07-28 13:29 ` Andrew Jones
0 siblings, 1 reply; 20+ messages in thread
From: Marc Zyngier @ 2020-07-28 13:13 UTC (permalink / raw)
To: Andrew Jones; +Cc: pbonzini, kvmarm, kvm, steven.price
On 2020-07-28 13:55, Andrew Jones wrote:
> On Mon, Jul 27, 2020 at 06:25:50PM +0100, Marc Zyngier wrote:
>> Hi Andrew,
>>
>> On 2020-07-11 11:04, Andrew Jones wrote:
>> > Don't confuse the guest by saying steal-time is supported when
>> > it hasn't been configured by userspace and won't work.
>> >
>> > Signed-off-by: Andrew Jones <drjones@redhat.com>
>> > ---
>> > arch/arm64/kvm/pvtime.c | 5 ++++-
>> > 1 file changed, 4 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/arch/arm64/kvm/pvtime.c b/arch/arm64/kvm/pvtime.c
>> > index f7b52ce1557e..2b22214909be 100644
>> > --- a/arch/arm64/kvm/pvtime.c
>> > +++ b/arch/arm64/kvm/pvtime.c
>> > @@ -42,9 +42,12 @@ long kvm_hypercall_pv_features(struct kvm_vcpu *vcpu)
>> >
>> > switch (feature) {
>> > case ARM_SMCCC_HV_PV_TIME_FEATURES:
>> > - case ARM_SMCCC_HV_PV_TIME_ST:
>> > val = SMCCC_RET_SUCCESS;
>> > break;
>> > + case ARM_SMCCC_HV_PV_TIME_ST:
>> > + if (vcpu->arch.steal.base != GPA_INVALID)
>> > + val = SMCCC_RET_SUCCESS;
>> > + break;
>> > }
>> >
>> > return val;
>>
>> I'm not so sure about this. I have always considered the
>> discovery interface to be "do you know about this SMCCC
>> function". And if you look at the spec, it says (4.2,
>> PV_TIME_FEATURES):
>>
>> <quote>
>> If PV_call_id identifies PV_TIME_FEATURES, this call returns:
>> • NOT_SUPPORTED (-1) to indicate that all
>> paravirtualized time functions in this specification are not
>> supported.
>> • SUCCESS (0) to indicate that all the paravirtualized time
>> functions in this specification are supported.
>> </quote>
>>
>> So the way I understand it, you cannot return "supported"
>> for PV_TIME_FEATURES, and yet return NOT_SUPPORTED for
>> PV_TIME_ST. It applies to *all* features.
>>
>> Yes, this is very bizarre. But I don't think we can deviate
>> from it.
>
> Ah, I see your point. But I wonder if we should drop this patch
> or if we should change the return of ARM_SMCCC_HV_PV_TIME_FEATURES
> to be dependant on all the pv calls?
>
> Discovery would look like this
>
> IF (SMCCC_ARCH_FEATURES, PV_TIME_FEATURES) == 0; THEN
> IF (PV_TIME_FEATURES, PV_TIME_FEATURES) == 0; THEN
> PV_TIME_ST is supported, as well as all other PV calls
> ELIF (PV_TIME_FEATURES, PV_TIME_ST) == 0; THEN
> PV_TIME_ST is supported
> ELIF (PV_TIME_FEATURES, <another-pv-call>) == 0; THEN
> <another-pv-call> is supported
> ...
> ENDIF
> ELSE
> No PV calls are supported
> ENDIF
>
> I believe the above implements a reasonable interpretation of the
> specification, but the all feature (PV_TIME_FEATURES, PV_TIME_FEATURES)
> thing is indeed bizarre no matter how you look at it.
It it indeed true to the spec. Thankfully we only support PV_TIME
as a feature for now, so we are (sort of) immune to the braindead
aspect of the discovery protocol.
I think returning a failure when PV_TIME isn't setup is a valid thing
to do, as long as it applies to all functions (i.e. something like
the below patch).
Thanks,
M.
diff --git a/arch/arm64/kvm/pvtime.c b/arch/arm64/kvm/pvtime.c
index f7b52ce1557e..c3ef4ebd6846 100644
--- a/arch/arm64/kvm/pvtime.c
+++ b/arch/arm64/kvm/pvtime.c
@@ -43,7 +43,8 @@ long kvm_hypercall_pv_features(struct kvm_vcpu *vcpu)
switch (feature) {
case ARM_SMCCC_HV_PV_TIME_FEATURES:
case ARM_SMCCC_HV_PV_TIME_ST:
- val = SMCCC_RET_SUCCESS;
+ if (vcpu->arch.steal.base != GPA_INVALID)
+ val = SMCCC_RET_SUCCESS;
break;
}
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] KVM: arm64: pvtime: steal-time is only supported when configured
2020-07-28 13:13 ` Marc Zyngier
@ 2020-07-28 13:29 ` Andrew Jones
0 siblings, 0 replies; 20+ messages in thread
From: Andrew Jones @ 2020-07-28 13:29 UTC (permalink / raw)
To: Marc Zyngier; +Cc: pbonzini, kvmarm, kvm, steven.price
On Tue, Jul 28, 2020 at 02:13:54PM +0100, Marc Zyngier wrote:
> On 2020-07-28 13:55, Andrew Jones wrote:
> > On Mon, Jul 27, 2020 at 06:25:50PM +0100, Marc Zyngier wrote:
> > > Hi Andrew,
> > >
> > > On 2020-07-11 11:04, Andrew Jones wrote:
> > > > Don't confuse the guest by saying steal-time is supported when
> > > > it hasn't been configured by userspace and won't work.
> > > >
> > > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > > ---
> > > > arch/arm64/kvm/pvtime.c | 5 ++++-
> > > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/arm64/kvm/pvtime.c b/arch/arm64/kvm/pvtime.c
> > > > index f7b52ce1557e..2b22214909be 100644
> > > > --- a/arch/arm64/kvm/pvtime.c
> > > > +++ b/arch/arm64/kvm/pvtime.c
> > > > @@ -42,9 +42,12 @@ long kvm_hypercall_pv_features(struct kvm_vcpu *vcpu)
> > > >
> > > > switch (feature) {
> > > > case ARM_SMCCC_HV_PV_TIME_FEATURES:
> > > > - case ARM_SMCCC_HV_PV_TIME_ST:
> > > > val = SMCCC_RET_SUCCESS;
> > > > break;
> > > > + case ARM_SMCCC_HV_PV_TIME_ST:
> > > > + if (vcpu->arch.steal.base != GPA_INVALID)
> > > > + val = SMCCC_RET_SUCCESS;
> > > > + break;
> > > > }
> > > >
> > > > return val;
> > >
> > > I'm not so sure about this. I have always considered the
> > > discovery interface to be "do you know about this SMCCC
> > > function". And if you look at the spec, it says (4.2,
> > > PV_TIME_FEATURES):
> > >
> > > <quote>
> > > If PV_call_id identifies PV_TIME_FEATURES, this call returns:
> > > • NOT_SUPPORTED (-1) to indicate that all
> > > paravirtualized time functions in this specification are not
> > > supported.
> > > • SUCCESS (0) to indicate that all the paravirtualized time
> > > functions in this specification are supported.
> > > </quote>
> > >
> > > So the way I understand it, you cannot return "supported"
> > > for PV_TIME_FEATURES, and yet return NOT_SUPPORTED for
> > > PV_TIME_ST. It applies to *all* features.
> > >
> > > Yes, this is very bizarre. But I don't think we can deviate
> > > from it.
> >
> > Ah, I see your point. But I wonder if we should drop this patch
> > or if we should change the return of ARM_SMCCC_HV_PV_TIME_FEATURES
> > to be dependant on all the pv calls?
> >
> > Discovery would look like this
> >
> > IF (SMCCC_ARCH_FEATURES, PV_TIME_FEATURES) == 0; THEN
> > IF (PV_TIME_FEATURES, PV_TIME_FEATURES) == 0; THEN
> > PV_TIME_ST is supported, as well as all other PV calls
> > ELIF (PV_TIME_FEATURES, PV_TIME_ST) == 0; THEN
> > PV_TIME_ST is supported
> > ELIF (PV_TIME_FEATURES, <another-pv-call>) == 0; THEN
> > <another-pv-call> is supported
> > ...
> > ENDIF
> > ELSE
> > No PV calls are supported
> > ENDIF
> >
> > I believe the above implements a reasonable interpretation of the
> > specification, but the all feature (PV_TIME_FEATURES, PV_TIME_FEATURES)
> > thing is indeed bizarre no matter how you look at it.
>
> It it indeed true to the spec. Thankfully we only support PV_TIME
> as a feature for now, so we are (sort of) immune to the braindead
> aspect of the discovery protocol.
>
> I think returning a failure when PV_TIME isn't setup is a valid thing
> to do, as long as it applies to all functions (i.e. something like
> the below patch).
>
> Thanks,
>
> M.
>
> diff --git a/arch/arm64/kvm/pvtime.c b/arch/arm64/kvm/pvtime.c
> index f7b52ce1557e..c3ef4ebd6846 100644
> --- a/arch/arm64/kvm/pvtime.c
> +++ b/arch/arm64/kvm/pvtime.c
> @@ -43,7 +43,8 @@ long kvm_hypercall_pv_features(struct kvm_vcpu *vcpu)
> switch (feature) {
> case ARM_SMCCC_HV_PV_TIME_FEATURES:
> case ARM_SMCCC_HV_PV_TIME_ST:
> - val = SMCCC_RET_SUCCESS;
> + if (vcpu->arch.steal.base != GPA_INVALID)
> + val = SMCCC_RET_SUCCESS;
> break;
> }
Looks good to me. I'll do that for v2.
Thanks,
drew
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/5] KVM: arm64: pvtime: Fix potential loss of stolen time
2020-07-11 10:04 [PATCH 0/5] KVM: arm64: pvtime: Fixes and a new cap Andrew Jones
2020-07-11 10:04 ` [PATCH 1/5] KVM: arm64: pvtime: steal-time is only supported when configured Andrew Jones
@ 2020-07-11 10:04 ` Andrew Jones
2020-07-27 17:29 ` Marc Zyngier
2020-07-11 10:04 ` [PATCH 3/5] KVM: arm64: pvtime: Fix stolen time accounting across migration Andrew Jones
` (6 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Andrew Jones @ 2020-07-11 10:04 UTC (permalink / raw)
To: kvm, kvmarm; +Cc: pbonzini, steven.price, maz
We should only check current->sched_info.run_delay once when
updating stolen time. Otherwise there's a chance there could
be a change between checks that we miss (preemption disabling
comes after vcpu request checks).
Signed-off-by: Andrew Jones <drjones@redhat.com>
---
arch/arm64/kvm/pvtime.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/kvm/pvtime.c b/arch/arm64/kvm/pvtime.c
index 2b22214909be..db5ef097a166 100644
--- a/arch/arm64/kvm/pvtime.c
+++ b/arch/arm64/kvm/pvtime.c
@@ -13,6 +13,7 @@
void kvm_update_stolen_time(struct kvm_vcpu *vcpu)
{
struct kvm *kvm = vcpu->kvm;
+ u64 last_steal = vcpu->arch.steal.last_steal;
u64 steal;
__le64 steal_le;
u64 offset;
@@ -24,8 +25,8 @@ void kvm_update_stolen_time(struct kvm_vcpu *vcpu)
/* Let's do the local bookkeeping */
steal = vcpu->arch.steal.steal;
- steal += current->sched_info.run_delay - vcpu->arch.steal.last_steal;
vcpu->arch.steal.last_steal = current->sched_info.run_delay;
+ steal += vcpu->arch.steal.last_steal - last_steal;
vcpu->arch.steal.steal = steal;
steal_le = cpu_to_le64(steal);
--
2.25.4
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/5] KVM: arm64: pvtime: Fix potential loss of stolen time
2020-07-11 10:04 ` [PATCH 2/5] KVM: arm64: pvtime: Fix potential loss of stolen time Andrew Jones
@ 2020-07-27 17:29 ` Marc Zyngier
0 siblings, 0 replies; 20+ messages in thread
From: Marc Zyngier @ 2020-07-27 17:29 UTC (permalink / raw)
To: Andrew Jones; +Cc: pbonzini, kvmarm, kvm, steven.price
On 2020-07-11 11:04, Andrew Jones wrote:
> We should only check current->sched_info.run_delay once when
> updating stolen time. Otherwise there's a chance there could
> be a change between checks that we miss (preemption disabling
> comes after vcpu request checks).
>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
> arch/arm64/kvm/pvtime.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kvm/pvtime.c b/arch/arm64/kvm/pvtime.c
> index 2b22214909be..db5ef097a166 100644
> --- a/arch/arm64/kvm/pvtime.c
> +++ b/arch/arm64/kvm/pvtime.c
> @@ -13,6 +13,7 @@
> void kvm_update_stolen_time(struct kvm_vcpu *vcpu)
> {
> struct kvm *kvm = vcpu->kvm;
> + u64 last_steal = vcpu->arch.steal.last_steal;
> u64 steal;
> __le64 steal_le;
> u64 offset;
> @@ -24,8 +25,8 @@ void kvm_update_stolen_time(struct kvm_vcpu *vcpu)
>
> /* Let's do the local bookkeeping */
> steal = vcpu->arch.steal.steal;
> - steal += current->sched_info.run_delay - vcpu->arch.steal.last_steal;
> vcpu->arch.steal.last_steal = current->sched_info.run_delay;
> + steal += vcpu->arch.steal.last_steal - last_steal;
> vcpu->arch.steal.steal = steal;
>
> steal_le = cpu_to_le64(steal);
Unless you read current->sched_info.run_delay using READ_ONCE,
there is no guarantee that this will do what you want. The
compiler is free to rejig this anyway it wants.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/5] KVM: arm64: pvtime: Fix stolen time accounting across migration
2020-07-11 10:04 [PATCH 0/5] KVM: arm64: pvtime: Fixes and a new cap Andrew Jones
2020-07-11 10:04 ` [PATCH 1/5] KVM: arm64: pvtime: steal-time is only supported when configured Andrew Jones
2020-07-11 10:04 ` [PATCH 2/5] KVM: arm64: pvtime: Fix potential loss of stolen time Andrew Jones
@ 2020-07-11 10:04 ` Andrew Jones
2020-07-27 17:54 ` Marc Zyngier
2020-07-11 10:04 ` [PATCH 4/5] KVM: Documentation minor fixups Andrew Jones
` (5 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Andrew Jones @ 2020-07-11 10:04 UTC (permalink / raw)
To: kvm, kvmarm; +Cc: pbonzini, steven.price, maz
When updating the stolen time we should always read the current
stolen time from the user provided memory, not from a kernel
cache. If we use a cache then we'll end up resetting stolen time
to zero on the first update after migration.
Signed-off-by: Andrew Jones <drjones@redhat.com>
---
arch/arm64/include/asm/kvm_host.h | 1 -
arch/arm64/kvm/pvtime.c | 23 +++++++++--------------
include/linux/kvm_host.h | 19 +++++++++++++++++++
3 files changed, 28 insertions(+), 15 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index c3e6fcc664b1..b01f52b61572 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -343,7 +343,6 @@ struct kvm_vcpu_arch {
/* Guest PV state */
struct {
- u64 steal;
u64 last_steal;
gpa_t base;
} steal;
diff --git a/arch/arm64/kvm/pvtime.c b/arch/arm64/kvm/pvtime.c
index db5ef097a166..025b5f3a97ef 100644
--- a/arch/arm64/kvm/pvtime.c
+++ b/arch/arm64/kvm/pvtime.c
@@ -13,26 +13,22 @@
void kvm_update_stolen_time(struct kvm_vcpu *vcpu)
{
struct kvm *kvm = vcpu->kvm;
+ u64 base = vcpu->arch.steal.base;
u64 last_steal = vcpu->arch.steal.last_steal;
- u64 steal;
- __le64 steal_le;
- u64 offset;
+ u64 offset = offsetof(struct pvclock_vcpu_stolen_time, stolen_time);
+ u64 steal = 0;
int idx;
- u64 base = vcpu->arch.steal.base;
if (base == GPA_INVALID)
return;
- /* Let's do the local bookkeeping */
- steal = vcpu->arch.steal.steal;
- vcpu->arch.steal.last_steal = current->sched_info.run_delay;
- steal += vcpu->arch.steal.last_steal - last_steal;
- vcpu->arch.steal.steal = steal;
-
- steal_le = cpu_to_le64(steal);
idx = srcu_read_lock(&kvm->srcu);
- offset = offsetof(struct pvclock_vcpu_stolen_time, stolen_time);
- kvm_put_guest(kvm, base + offset, steal_le, u64);
+ if (!kvm_get_guest(kvm, base + offset, steal, u64)) {
+ steal = le64_to_cpu(steal);
+ vcpu->arch.steal.last_steal = current->sched_info.run_delay;
+ steal += vcpu->arch.steal.last_steal - last_steal;
+ kvm_put_guest(kvm, base + offset, cpu_to_le64(steal), u64);
+ }
srcu_read_unlock(&kvm->srcu, idx);
}
@@ -68,7 +64,6 @@ gpa_t kvm_init_stolen_time(struct kvm_vcpu *vcpu)
* Start counting stolen time from the time the guest requests
* the feature enabled.
*/
- vcpu->arch.steal.steal = 0;
vcpu->arch.steal.last_steal = current->sched_info.run_delay;
idx = srcu_read_lock(&kvm->srcu);
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index d564855243d8..e2fc655f0b5b 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -749,6 +749,25 @@ int kvm_write_guest_offset_cached(struct kvm *kvm, struct gfn_to_hva_cache *ghc,
int kvm_gfn_to_hva_cache_init(struct kvm *kvm, struct gfn_to_hva_cache *ghc,
gpa_t gpa, unsigned long len);
+#define __kvm_get_guest(kvm, gfn, offset, x, type) \
+({ \
+ unsigned long __addr = gfn_to_hva(kvm, gfn); \
+ type __user *__uaddr = (type __user *)(__addr + offset); \
+ int __ret = -EFAULT; \
+ \
+ if (!kvm_is_error_hva(__addr)) \
+ __ret = get_user(x, __uaddr); \
+ __ret; \
+})
+
+#define kvm_get_guest(kvm, gpa, x, type) \
+({ \
+ gpa_t __gpa = gpa; \
+ struct kvm *__kvm = kvm; \
+ __kvm_get_guest(__kvm, __gpa >> PAGE_SHIFT, \
+ offset_in_page(__gpa), x, type); \
+})
+
#define __kvm_put_guest(kvm, gfn, offset, value, type) \
({ \
unsigned long __addr = gfn_to_hva(kvm, gfn); \
--
2.25.4
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 3/5] KVM: arm64: pvtime: Fix stolen time accounting across migration
2020-07-11 10:04 ` [PATCH 3/5] KVM: arm64: pvtime: Fix stolen time accounting across migration Andrew Jones
@ 2020-07-27 17:54 ` Marc Zyngier
0 siblings, 0 replies; 20+ messages in thread
From: Marc Zyngier @ 2020-07-27 17:54 UTC (permalink / raw)
To: Andrew Jones; +Cc: pbonzini, kvmarm, kvm, steven.price
On 2020-07-11 11:04, Andrew Jones wrote:
> When updating the stolen time we should always read the current
> stolen time from the user provided memory, not from a kernel
> cache. If we use a cache then we'll end up resetting stolen time
> to zero on the first update after migration.
>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
> arch/arm64/include/asm/kvm_host.h | 1 -
> arch/arm64/kvm/pvtime.c | 23 +++++++++--------------
> include/linux/kvm_host.h | 19 +++++++++++++++++++
> 3 files changed, 28 insertions(+), 15 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h
> b/arch/arm64/include/asm/kvm_host.h
> index c3e6fcc664b1..b01f52b61572 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -343,7 +343,6 @@ struct kvm_vcpu_arch {
>
> /* Guest PV state */
> struct {
> - u64 steal;
> u64 last_steal;
> gpa_t base;
> } steal;
> diff --git a/arch/arm64/kvm/pvtime.c b/arch/arm64/kvm/pvtime.c
> index db5ef097a166..025b5f3a97ef 100644
> --- a/arch/arm64/kvm/pvtime.c
> +++ b/arch/arm64/kvm/pvtime.c
> @@ -13,26 +13,22 @@
> void kvm_update_stolen_time(struct kvm_vcpu *vcpu)
> {
> struct kvm *kvm = vcpu->kvm;
> + u64 base = vcpu->arch.steal.base;
> u64 last_steal = vcpu->arch.steal.last_steal;
> - u64 steal;
> - __le64 steal_le;
> - u64 offset;
> + u64 offset = offsetof(struct pvclock_vcpu_stolen_time, stolen_time);
> + u64 steal = 0;
> int idx;
> - u64 base = vcpu->arch.steal.base;
>
> if (base == GPA_INVALID)
> return;
>
> - /* Let's do the local bookkeeping */
> - steal = vcpu->arch.steal.steal;
> - vcpu->arch.steal.last_steal = current->sched_info.run_delay;
> - steal += vcpu->arch.steal.last_steal - last_steal;
> - vcpu->arch.steal.steal = steal;
> -
> - steal_le = cpu_to_le64(steal);
> idx = srcu_read_lock(&kvm->srcu);
> - offset = offsetof(struct pvclock_vcpu_stolen_time, stolen_time);
> - kvm_put_guest(kvm, base + offset, steal_le, u64);
> + if (!kvm_get_guest(kvm, base + offset, steal, u64)) {
> + steal = le64_to_cpu(steal);
> + vcpu->arch.steal.last_steal = current->sched_info.run_delay;
> + steal += vcpu->arch.steal.last_steal - last_steal;
> + kvm_put_guest(kvm, base + offset, cpu_to_le64(steal), u64);
> + }
> srcu_read_unlock(&kvm->srcu, idx);
> }
>
> @@ -68,7 +64,6 @@ gpa_t kvm_init_stolen_time(struct kvm_vcpu *vcpu)
> * Start counting stolen time from the time the guest requests
> * the feature enabled.
> */
> - vcpu->arch.steal.steal = 0;
> vcpu->arch.steal.last_steal = current->sched_info.run_delay;
>
> idx = srcu_read_lock(&kvm->srcu);
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index d564855243d8..e2fc655f0b5b 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -749,6 +749,25 @@ int kvm_write_guest_offset_cached(struct kvm
> *kvm, struct gfn_to_hva_cache *ghc,
> int kvm_gfn_to_hva_cache_init(struct kvm *kvm, struct gfn_to_hva_cache
> *ghc,
> gpa_t gpa, unsigned long len);
>
> +#define __kvm_get_guest(kvm, gfn, offset, x, type) \
> +({ \
> + unsigned long __addr = gfn_to_hva(kvm, gfn); \
> + type __user *__uaddr = (type __user *)(__addr + offset); \
Passing the type around is pretty ugly. Can't you use something like:
typeof(x) __user *__uaddr = (typeof(__uaddr))(__addr + offset);
which would avoid passing this type around? kvm_put_guest could
use the same treatment.
Yes, it forces the caller to rigorously type the inputs to the
macro. But they should do that anyway.
> + int __ret = -EFAULT; \
> + \
> + if (!kvm_is_error_hva(__addr)) \
> + __ret = get_user(x, __uaddr); \
> + __ret; \
> +})
> +
> +#define kvm_get_guest(kvm, gpa, x, type) \
> +({ \
> + gpa_t __gpa = gpa; \
> + struct kvm *__kvm = kvm; \
> + __kvm_get_guest(__kvm, __gpa >> PAGE_SHIFT, \
> + offset_in_page(__gpa), x, type); \
> +})
> +
> #define __kvm_put_guest(kvm, gfn, offset, value, type) \
> ({ \
> unsigned long __addr = gfn_to_hva(kvm, gfn); \
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/5] KVM: Documentation minor fixups
2020-07-11 10:04 [PATCH 0/5] KVM: arm64: pvtime: Fixes and a new cap Andrew Jones
` (2 preceding siblings ...)
2020-07-11 10:04 ` [PATCH 3/5] KVM: arm64: pvtime: Fix stolen time accounting across migration Andrew Jones
@ 2020-07-11 10:04 ` Andrew Jones
2020-07-27 17:55 ` Marc Zyngier
2020-07-11 10:04 ` [PATCH 5/5] arm64/x86: KVM: Introduce steal-time cap Andrew Jones
` (4 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Andrew Jones @ 2020-07-11 10:04 UTC (permalink / raw)
To: kvm, kvmarm; +Cc: pbonzini, steven.price, maz
Reviewed-by: Steven Price <steven.price@arm.com>
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 320788f81a05..3bd96c1a3962 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -6122,7 +6122,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
@@ -6135,16 +6135,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
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 4/5] KVM: Documentation minor fixups
2020-07-11 10:04 ` [PATCH 4/5] KVM: Documentation minor fixups Andrew Jones
@ 2020-07-27 17:55 ` Marc Zyngier
0 siblings, 0 replies; 20+ messages in thread
From: Marc Zyngier @ 2020-07-27 17:55 UTC (permalink / raw)
To: Andrew Jones; +Cc: pbonzini, kvmarm, kvm, steven.price
On 2020-07-11 11:04, Andrew Jones wrote:
> Reviewed-by: Steven Price <steven.price@arm.com>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
It'd be good to have an actual commit message.
> ---
> 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 320788f81a05..3bd96c1a3962 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -6122,7 +6122,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
> @@ -6135,16 +6135,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.
But this seems to be an otherwise unrelated patch.
I'm happy to take it, but it seems odd here.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 5/5] arm64/x86: KVM: Introduce steal-time cap
2020-07-11 10:04 [PATCH 0/5] KVM: arm64: pvtime: Fixes and a new cap Andrew Jones
` (3 preceding siblings ...)
2020-07-11 10:04 ` [PATCH 4/5] KVM: Documentation minor fixups Andrew Jones
@ 2020-07-11 10:04 ` Andrew Jones
2020-07-27 17:58 ` Marc Zyngier
2020-07-11 10:20 ` [PATCH 0/5] KVM: arm64: pvtime: Fixes and a new cap Andrew Jones
` (3 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Andrew Jones @ 2020-07-11 10:04 UTC (permalink / raw)
To: kvm, kvmarm; +Cc: pbonzini, steven.price, maz
arm64 requires a vcpu fd (KVM_HAS_DEVICE_ATTR vcpu ioctl) to probe
support for steal-time. However this is unnecessary, as only a KVM
fd is required, and it complicates userspace (userspace may prefer
delaying vcpu creation until after feature probing). Introduce a cap
that can be checked instead. While x86 can already probe steal-time
support with a kvm fd (KVM_GET_SUPPORTED_CPUID), we add the cap there
too for consistency.
Reviewed-by: Steven Price <steven.price@arm.com>
Signed-off-by: Andrew Jones <drjones@redhat.com>
---
Documentation/virt/kvm/api.rst | 11 +++++++++++
arch/arm64/include/asm/kvm_host.h | 1 +
arch/arm64/kvm/arm.c | 3 +++
arch/arm64/kvm/pvtime.c | 2 +-
arch/x86/kvm/x86.c | 3 +++
include/uapi/linux/kvm.h | 1 +
6 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index 3bd96c1a3962..3716d1e29771 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -6152,3 +6152,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/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index b01f52b61572..8909c840ea91 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -500,6 +500,7 @@ long kvm_hypercall_pv_features(struct kvm_vcpu *vcpu);
gpa_t kvm_init_stolen_time(struct kvm_vcpu *vcpu);
void kvm_update_stolen_time(struct kvm_vcpu *vcpu);
+bool kvm_arm_pvtime_supported(void);
int kvm_arm_pvtime_set_attr(struct kvm_vcpu *vcpu,
struct kvm_device_attr *attr);
int kvm_arm_pvtime_get_attr(struct kvm_vcpu *vcpu,
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 90cb90561446..0f7f8cd2f341 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 = kvm_arm_pvtime_supported();
+ break;
default:
r = kvm_arch_vm_ioctl_check_extension(kvm, ext);
break;
diff --git a/arch/arm64/kvm/pvtime.c b/arch/arm64/kvm/pvtime.c
index 025b5f3a97ef..468bd1ef9c7e 100644
--- a/arch/arm64/kvm/pvtime.c
+++ b/arch/arm64/kvm/pvtime.c
@@ -73,7 +73,7 @@ gpa_t kvm_init_stolen_time(struct kvm_vcpu *vcpu)
return base;
}
-static bool kvm_arm_pvtime_supported(void)
+bool kvm_arm_pvtime_supported(void)
{
return !!sched_info_on();
}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 88c593f83b28..27fc86bb28c4 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3538,6 +3538,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
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 5/5] arm64/x86: KVM: Introduce steal-time cap
2020-07-11 10:04 ` [PATCH 5/5] arm64/x86: KVM: Introduce steal-time cap Andrew Jones
@ 2020-07-27 17:58 ` Marc Zyngier
0 siblings, 0 replies; 20+ messages in thread
From: Marc Zyngier @ 2020-07-27 17:58 UTC (permalink / raw)
To: Andrew Jones; +Cc: pbonzini, kvmarm, kvm, steven.price
On 2020-07-11 11:04, Andrew Jones wrote:
> arm64 requires a vcpu fd (KVM_HAS_DEVICE_ATTR vcpu ioctl) to probe
> support for steal-time. However this is unnecessary, as only a KVM
> fd is required, and it complicates userspace (userspace may prefer
> delaying vcpu creation until after feature probing). Introduce a cap
> that can be checked instead. While x86 can already probe steal-time
> support with a kvm fd (KVM_GET_SUPPORTED_CPUID), we add the cap there
> too for consistency.
>
> Reviewed-by: Steven Price <steven.price@arm.com>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
> Documentation/virt/kvm/api.rst | 11 +++++++++++
> arch/arm64/include/asm/kvm_host.h | 1 +
> arch/arm64/kvm/arm.c | 3 +++
> arch/arm64/kvm/pvtime.c | 2 +-
> arch/x86/kvm/x86.c | 3 +++
> include/uapi/linux/kvm.h | 1 +
> 6 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/virt/kvm/api.rst
> b/Documentation/virt/kvm/api.rst
> index 3bd96c1a3962..3716d1e29771 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -6152,3 +6152,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".
Maybe add something that indicates that KVM_CAP_STEAL_TIME
and the architecture specific ioctl aren't allowed to disagree
about the support?
> diff --git a/arch/arm64/include/asm/kvm_host.h
> b/arch/arm64/include/asm/kvm_host.h
> index b01f52b61572..8909c840ea91 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -500,6 +500,7 @@ long kvm_hypercall_pv_features(struct kvm_vcpu
> *vcpu);
> gpa_t kvm_init_stolen_time(struct kvm_vcpu *vcpu);
> void kvm_update_stolen_time(struct kvm_vcpu *vcpu);
>
> +bool kvm_arm_pvtime_supported(void);
> int kvm_arm_pvtime_set_attr(struct kvm_vcpu *vcpu,
> struct kvm_device_attr *attr);
> int kvm_arm_pvtime_get_attr(struct kvm_vcpu *vcpu,
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 90cb90561446..0f7f8cd2f341 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 = kvm_arm_pvtime_supported();
> + break;
> default:
> r = kvm_arch_vm_ioctl_check_extension(kvm, ext);
> break;
> diff --git a/arch/arm64/kvm/pvtime.c b/arch/arm64/kvm/pvtime.c
> index 025b5f3a97ef..468bd1ef9c7e 100644
> --- a/arch/arm64/kvm/pvtime.c
> +++ b/arch/arm64/kvm/pvtime.c
> @@ -73,7 +73,7 @@ gpa_t kvm_init_stolen_time(struct kvm_vcpu *vcpu)
> return base;
> }
>
> -static bool kvm_arm_pvtime_supported(void)
> +bool kvm_arm_pvtime_supported(void)
> {
> return !!sched_info_on();
> }
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 88c593f83b28..27fc86bb28c4 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3538,6 +3538,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
Otherwise looks ok.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/5] KVM: arm64: pvtime: Fixes and a new cap
2020-07-11 10:04 [PATCH 0/5] KVM: arm64: pvtime: Fixes and a new cap Andrew Jones
` (4 preceding siblings ...)
2020-07-11 10:04 ` [PATCH 5/5] arm64/x86: KVM: Introduce steal-time cap Andrew Jones
@ 2020-07-11 10:20 ` Andrew Jones
2020-07-13 8:25 ` Steven Price
` (2 subsequent siblings)
8 siblings, 0 replies; 20+ messages in thread
From: Andrew Jones @ 2020-07-11 10:20 UTC (permalink / raw)
To: kvm, kvmarm; +Cc: pbonzini, steven.price, maz
On Sat, Jul 11, 2020 at 12:04:29PM +0200, Andrew Jones wrote:
> The first three patches in the series are fixes that come from testing
> and reviewing pvtime code while writing the QEMU support (I'll reply
> to this mail with a link to the QEMU patches after posting - which I'll
> do shortly). The last patch is only a convenience for userspace, and I
> wouldn't be heartbroken if it wasn't deemed worth it. The QEMU patches
> I'll be posting are currently written without the cap. However, if the
> cap is accepted, then I'll change the QEMU code to use it.
And the promised link to the QEMU patches
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg03856.html
Thanks,
drew
>
> Thanks,
> drew
>
> Andrew Jones (5):
> KVM: arm64: pvtime: steal-time is only supported when configured
> KVM: arm64: pvtime: Fix potential loss of stolen time
> KVM: arm64: pvtime: Fix stolen time accounting across migration
> KVM: Documentation minor fixups
> arm64/x86: KVM: Introduce steal-time cap
>
> Documentation/virt/kvm/api.rst | 20 ++++++++++++++++----
> arch/arm64/include/asm/kvm_host.h | 2 +-
> arch/arm64/kvm/arm.c | 3 +++
> arch/arm64/kvm/pvtime.c | 31 +++++++++++++++----------------
> arch/x86/kvm/x86.c | 3 +++
> include/linux/kvm_host.h | 19 +++++++++++++++++++
> include/uapi/linux/kvm.h | 1 +
> 7 files changed, 58 insertions(+), 21 deletions(-)
>
> --
> 2.25.4
>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/5] KVM: arm64: pvtime: Fixes and a new cap
2020-07-11 10:04 [PATCH 0/5] KVM: arm64: pvtime: Fixes and a new cap Andrew Jones
` (5 preceding siblings ...)
2020-07-11 10:20 ` [PATCH 0/5] KVM: arm64: pvtime: Fixes and a new cap Andrew Jones
@ 2020-07-13 8:25 ` Steven Price
2020-07-27 11:02 ` Andrew Jones
2020-07-27 18:01 ` Marc Zyngier
8 siblings, 0 replies; 20+ messages in thread
From: Steven Price @ 2020-07-13 8:25 UTC (permalink / raw)
To: Andrew Jones, kvm, kvmarm; +Cc: pbonzini, maz
On 11/07/2020 11:04, Andrew Jones wrote:
> The first three patches in the series are fixes that come from testing
> and reviewing pvtime code while writing the QEMU support (I'll reply
> to this mail with a link to the QEMU patches after posting - which I'll
> do shortly). The last patch is only a convenience for userspace, and I
> wouldn't be heartbroken if it wasn't deemed worth it. The QEMU patches
> I'll be posting are currently written without the cap. However, if the
> cap is accepted, then I'll change the QEMU code to use it.
Thanks for this, you've already got my r-b on the last two patches. For
the others:
Reviewed-by: Steven Price <steven.price@arm.com>
> Thanks,
> drew
>
> Andrew Jones (5):
> KVM: arm64: pvtime: steal-time is only supported when configured
> KVM: arm64: pvtime: Fix potential loss of stolen time
> KVM: arm64: pvtime: Fix stolen time accounting across migration
> KVM: Documentation minor fixups
> arm64/x86: KVM: Introduce steal-time cap
>
> Documentation/virt/kvm/api.rst | 20 ++++++++++++++++----
> arch/arm64/include/asm/kvm_host.h | 2 +-
> arch/arm64/kvm/arm.c | 3 +++
> arch/arm64/kvm/pvtime.c | 31 +++++++++++++++----------------
> arch/x86/kvm/x86.c | 3 +++
> include/linux/kvm_host.h | 19 +++++++++++++++++++
> include/uapi/linux/kvm.h | 1 +
> 7 files changed, 58 insertions(+), 21 deletions(-)
>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/5] KVM: arm64: pvtime: Fixes and a new cap
2020-07-11 10:04 [PATCH 0/5] KVM: arm64: pvtime: Fixes and a new cap Andrew Jones
` (6 preceding siblings ...)
2020-07-13 8:25 ` Steven Price
@ 2020-07-27 11:02 ` Andrew Jones
2020-07-27 11:15 ` Marc Zyngier
2020-07-27 18:01 ` Marc Zyngier
8 siblings, 1 reply; 20+ messages in thread
From: Andrew Jones @ 2020-07-27 11:02 UTC (permalink / raw)
To: kvm, kvmarm; +Cc: pbonzini, steven.price, maz
Hi Marc,
Ping?
Thanks,
drew
On Sat, Jul 11, 2020 at 12:04:29PM +0200, Andrew Jones wrote:
> The first three patches in the series are fixes that come from testing
> and reviewing pvtime code while writing the QEMU support (I'll reply
> to this mail with a link to the QEMU patches after posting - which I'll
> do shortly). The last patch is only a convenience for userspace, and I
> wouldn't be heartbroken if it wasn't deemed worth it. The QEMU patches
> I'll be posting are currently written without the cap. However, if the
> cap is accepted, then I'll change the QEMU code to use it.
>
> Thanks,
> drew
>
> Andrew Jones (5):
> KVM: arm64: pvtime: steal-time is only supported when configured
> KVM: arm64: pvtime: Fix potential loss of stolen time
> KVM: arm64: pvtime: Fix stolen time accounting across migration
> KVM: Documentation minor fixups
> arm64/x86: KVM: Introduce steal-time cap
>
> Documentation/virt/kvm/api.rst | 20 ++++++++++++++++----
> arch/arm64/include/asm/kvm_host.h | 2 +-
> arch/arm64/kvm/arm.c | 3 +++
> arch/arm64/kvm/pvtime.c | 31 +++++++++++++++----------------
> arch/x86/kvm/x86.c | 3 +++
> include/linux/kvm_host.h | 19 +++++++++++++++++++
> include/uapi/linux/kvm.h | 1 +
> 7 files changed, 58 insertions(+), 21 deletions(-)
>
> --
> 2.25.4
>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/5] KVM: arm64: pvtime: Fixes and a new cap
2020-07-11 10:04 [PATCH 0/5] KVM: arm64: pvtime: Fixes and a new cap Andrew Jones
` (7 preceding siblings ...)
2020-07-27 11:02 ` Andrew Jones
@ 2020-07-27 18:01 ` Marc Zyngier
2020-07-28 12:59 ` Andrew Jones
8 siblings, 1 reply; 20+ messages in thread
From: Marc Zyngier @ 2020-07-27 18:01 UTC (permalink / raw)
To: Andrew Jones; +Cc: pbonzini, kvmarm, kvm, steven.price
On 2020-07-11 11:04, Andrew Jones wrote:
> The first three patches in the series are fixes that come from testing
> and reviewing pvtime code while writing the QEMU support (I'll reply
> to this mail with a link to the QEMU patches after posting - which I'll
> do shortly). The last patch is only a convenience for userspace, and I
> wouldn't be heartbroken if it wasn't deemed worth it. The QEMU patches
> I'll be posting are currently written without the cap. However, if the
> cap is accepted, then I'll change the QEMU code to use it.
>
> Thanks,
> drew
>
> Andrew Jones (5):
> KVM: arm64: pvtime: steal-time is only supported when configured
> KVM: arm64: pvtime: Fix potential loss of stolen time
> KVM: arm64: pvtime: Fix stolen time accounting across migration
> KVM: Documentation minor fixups
> arm64/x86: KVM: Introduce steal-time cap
>
> Documentation/virt/kvm/api.rst | 20 ++++++++++++++++----
> arch/arm64/include/asm/kvm_host.h | 2 +-
> arch/arm64/kvm/arm.c | 3 +++
> arch/arm64/kvm/pvtime.c | 31 +++++++++++++++----------------
> arch/x86/kvm/x86.c | 3 +++
> include/linux/kvm_host.h | 19 +++++++++++++++++++
> include/uapi/linux/kvm.h | 1 +
> 7 files changed, 58 insertions(+), 21 deletions(-)
Hi Andrew,
Sorry about the time it took to get to this series.
Although I had a number of comments, they are all easy to
address, and you will hopefully be able to respin it quickly
(assuming we agree that patch #1 is unnecessary).
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/5] KVM: arm64: pvtime: Fixes and a new cap
2020-07-27 18:01 ` Marc Zyngier
@ 2020-07-28 12:59 ` Andrew Jones
0 siblings, 0 replies; 20+ messages in thread
From: Andrew Jones @ 2020-07-28 12:59 UTC (permalink / raw)
To: Marc Zyngier; +Cc: pbonzini, kvmarm, kvm, steven.price
On Mon, Jul 27, 2020 at 07:01:04PM +0100, Marc Zyngier wrote:
> On 2020-07-11 11:04, Andrew Jones wrote:
> > The first three patches in the series are fixes that come from testing
> > and reviewing pvtime code while writing the QEMU support (I'll reply
> > to this mail with a link to the QEMU patches after posting - which I'll
> > do shortly). The last patch is only a convenience for userspace, and I
> > wouldn't be heartbroken if it wasn't deemed worth it. The QEMU patches
> > I'll be posting are currently written without the cap. However, if the
> > cap is accepted, then I'll change the QEMU code to use it.
> >
> > Thanks,
> > drew
> >
> > Andrew Jones (5):
> > KVM: arm64: pvtime: steal-time is only supported when configured
> > KVM: arm64: pvtime: Fix potential loss of stolen time
> > KVM: arm64: pvtime: Fix stolen time accounting across migration
> > KVM: Documentation minor fixups
> > arm64/x86: KVM: Introduce steal-time cap
> >
> > Documentation/virt/kvm/api.rst | 20 ++++++++++++++++----
> > arch/arm64/include/asm/kvm_host.h | 2 +-
> > arch/arm64/kvm/arm.c | 3 +++
> > arch/arm64/kvm/pvtime.c | 31 +++++++++++++++----------------
> > arch/x86/kvm/x86.c | 3 +++
> > include/linux/kvm_host.h | 19 +++++++++++++++++++
> > include/uapi/linux/kvm.h | 1 +
> > 7 files changed, 58 insertions(+), 21 deletions(-)
>
> Hi Andrew,
>
> Sorry about the time it took to get to this series.
No problem.
> Although I had a number of comments, they are all easy to
> address, and you will hopefully be able to respin it quickly
I'll address all the comments and get it respun right away.
> (assuming we agree that patch #1 is unnecessary).
I'm not sure yet. I've suggested yet another interpretation
of the spec and will see what you say about that.
Thanks,
drew
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 20+ messages in thread