KVM: arm64: pvtime: Fixes and a new cap
Subsystems (6)
cover KVM: arm64: pvtime: Fixes and a new cap
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(-)
Signature · 1 line
2.25.4
PATCH 1/5 KVM: arm64: pvtime: steal-time is only supported when configured
Don't confuse the guest by saying steal-time is supported when it hasn't been configured by userspace and won't work.
--- arch/arm64/kvm/pvtime.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
arch/arm64/kvm/pvtime.c +4 -1
@@ -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;
Signature · 1 line
2.25.4
Marc Zyngier
maintainerRe: [PATCH 1/5] KVM: arm64: pvtime: steal-time is only supported when configured
Hi Andrew,
Quoted 26 lines · Andrew Jones
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 <[email protected]>--- arch/arm64/kvm/pvtime.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)arch/arm64/kvm/pvtime.c +4
-1@@ -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 ; }
Quoted 1 line
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.
Signature · 1 line
Jazz is not dead. It just smells funny...
Re: [PATCH 1/5] KVM: arm64: pvtime: steal-time is only supported when configured
Quoted 44 lines · Marc Zyngier
Hi Andrew,
Quoted 16 lines · Andrew Jones
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 <[email protected]>--- arch/arm64/kvm/pvtime.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)arch/arm64/kvm/pvtime.c +0
-0@@ -42,9 +42,12 @@ long kvm_hypercall_pv_features(struct kvm_vcpu *vcpu)switch (feature) {case ARM_SMCCC_HV_PV_TIME_FEATURES :arch/arm64/kvm/pvtime.c long kvm_hypercall_pv_features(struct kvm_vcpu *vcpu) · line 53
Quoted 10 lines
arch/arm64/kvm/pvtime.c +4
-1-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>
IfPV_call_ididentifiesPV_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>
Quoted 6 lines
So the way I understand it, you cannot return "supported" for
PV_TIME_FEATURES, and yet returnNOT_SUPPORTEDforPV_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
Marc Zyngier
maintainerRe: [PATCH 1/5] KVM: arm64: pvtime: steal-time is only supported when configured
Quoted 71 lines · Andrew Jones
Quoted 44 lines · Marc Zyngier
Hi Andrew,
Quoted 16 lines · Andrew Jones
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 <[email protected]>--- arch/arm64/kvm/pvtime.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)arch/arm64/kvm/pvtime.c +0
-0@@ -42,9 +42,12 @@ long kvm_hypercall_pv_features(struct kvm_vcpu *vcpu)switch (feature) {case ARM_SMCCC_HV_PV_TIME_FEATURES :arch/arm64/kvm/pvtime.c long kvm_hypercall_pv_features(struct kvm_vcpu *vcpu) · line 53
Quoted 10 lines
arch/arm64/kvm/pvtime.c +4
-1-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>
IfPV_call_ididentifiesPV_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>Quoted 6 lines
So the way I understand it, you cannot return "supported" for
PV_TIME_FEATURES, and yet returnNOT_SUPPORTEDforPV_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_FEATURESto 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_STis supported, as well as all other PV calls
ELIF (PV_TIME_FEATURES,PV_TIME_ST) == 0; THEN
PV_TIME_STis supported
ELIF (PV_TIME_FEATURES, <another-pv-call>) == 0; THEN
<another-pv-call> is supported
...
ENDIF
ELSE
No PV calls are supported
ENDIF
Quoted 3 lines
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.
arch/arm64/kvm/pvtime.c +2 -1
@@ -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 ; }
Signature · 1 line
Jazz is not dead. It just smells funny...
Re: [PATCH 1/5] KVM: arm64: pvtime: steal-time is only supported when configured
Quoted 93 lines · Marc Zyngier
Quoted 71 lines · Andrew Jones
Quoted 44 lines · Marc Zyngier
Hi Andrew,
Quoted 16 lines · Andrew Jones
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 <[email protected]>--- arch/arm64/kvm/pvtime.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)arch/arm64/kvm/pvtime.c +0
-0@@ -42,9 +42,12 @@ long kvm_hypercall_pv_features(struct kvm_vcpu *vcpu)switch (feature) {case ARM_SMCCC_HV_PV_TIME_FEATURES :arch/arm64/kvm/pvtime.c long kvm_hypercall_pv_features(struct kvm_vcpu *vcpu) · line 53
Quoted 10 lines
arch/arm64/kvm/pvtime.c +4
-1-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>
IfPV_call_ididentifiesPV_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>Quoted 6 lines
So the way I understand it, you cannot return "supported" for
PV_TIME_FEATURES, and yet returnNOT_SUPPORTEDforPV_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_FEATURESto 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_STis supported, as well as all other PV calls
ELIF (PV_TIME_FEATURES,PV_TIME_ST) == 0; THEN
PV_TIME_STis supported
ELIF (PV_TIME_FEATURES, <another-pv-call>) == 0; THEN
<another-pv-call> is supported
...
ENDIF
ELSE
No PV calls are supported
ENDIFQuoted 3 lines
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_TIMEas 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_TIMEisn'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.arch/arm64/kvm/pvtime.c +0
-0
arch/arm64/kvm/pvtime.c long kvm_hypercall_pv_features(struct kvm_vcpu *vcpu) · line 49
Quoted 8 lines
arch/arm64/kvm/pvtime.c +2
-1@@ -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
PATCH 2/5 KVM: arm64: pvtime: Fix potential loss of stolen time
In reply to Andrew JonesWe 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).
--- arch/arm64/kvm/pvtime.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
arch/arm64/kvm/pvtime.c +2 -1
@@ -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);
Signature · 1 line
2.25.4
Marc Zyngier
maintainerRe: [PATCH 2/5] KVM: arm64: pvtime: Fix potential loss of stolen time
Quoted 23 lines · Andrew Jones
We should only check
current->sched_info.run_delayonce 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 <[email protected]>--- arch/arm64/kvm/pvtime.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)arch/arm64/kvm/pvtime.c +1
-0@@ -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;
arch/arm64/kvm/pvtime.c void kvm_update_stolen_time(struct kvm_vcpu *vcpu) · line 32
Quoted 9 lines
arch/arm64/kvm/pvtime.c +1
-1@@ -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.
Signature · 1 line
Jazz is not dead. It just smells funny...
PATCH 3/5 KVM: arm64: pvtime: Fix stolen time accounting across migration
In reply to Andrew JonesWhen 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.
--- 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(-)
arch/arm64/include/asm/kvm_host.h +0 -1
@@ -343,7 +343,6 @@ struct kvm_vcpu_arch {/* Guest PV state */ struct { -u64 steal;u64 last_steal;gpa_t base; } steal;
arch/arm64/kvm/pvtime.c +9 -14
@@ -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);
include/linux/kvm_host.h +19 -0
@@ -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); \
Signature · 1 line
2.25.4
Marc Zyngier
maintainerRe: [PATCH 3/5] KVM: arm64: pvtime: Fix stolen time accounting across migration
Quoted 78 lines · Andrew Jones
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 <[email protected]>--- 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(-)arch/arm64/include/asm/kvm_host.h +0
-1@@ -343,7 +343,6 @@ struct kvm_vcpu_arch {/* Guest PV state */ struct { -u64 steal;u64 last_steal;gpa_t base; } steal;arch/arm64/kvm/pvtime.c +9
-14@@ -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);include/linux/kvm_host.h +0
-0
include/linux/kvm_host.h int kvm_write_guest_offset_cached(struct kvm · line 757
Quoted 9 lines
include/linux/kvm_host.h +4
-0@@ -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.
Quoted 16 lines
include/linux/kvm_host.h +15
-0+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) \
Quoted 2 lines
({ \
unsigned long__addr=gfn_to_hva(kvm, gfn); \
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
PATCH 4/5 KVM: Documentation minor fixups
In reply to Andrew Jones--- Documentation/virt/kvm/api.rst | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
Documentation/virt/kvm/api.rst +5 -4
@@ -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.
Signature · 6 lines
2.25.4 _______________________________________________ kvmarm mailing list [email protected] https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Marc Zyngier
maintainerRe: [PATCH 4/5] KVM: Documentation minor fixups
Quoted 2 lines · Andrew Jones
Reviewed-by Steven Price <[email protected]>Signed-off-by Andrew Jones <[email protected]>
It'd be good to have an actual commit message.
Quoted 31 lines
--- Documentation/virt/kvm/api.rst | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)Documentation/virt/kvm/api.rst +0
-0@@ -6122,7 +6122,7 @@ HvCallSendSyntheticClusterIpi,HvCallSendSyntheticClusterIpiEx.
8.21KVM_CAP_HYPERV_DIRECT_TLBFLUSH
------------------------------------:Architecture: x86
+:Architectures: x86This capability indicates that KVM running on top of Hyper-V
hypervisor
enables Direct TLB flush for its guests meaning that TLB flushDocumentation/virt/kvm/api.rst +0
-0@@ -6135,16 +6135,17 @@ in CPUID and only exposes Hyper-Videntification. In this case, guest
thinks it's running on Hyper-V and only use Hyper-V hypercalls.8.22
KVM_CAP_S390_VCPU_RESETS
+-----------------------------Documentation/virt/kvm/api.rst +1
-1-Architectures: s390 +:Architectures: s390 This capability indicates that the KVM_S390_NORMAL_RESET and KVM_S390_CLEAR_RESET ioctls are available.
Documentation/virt/kvm/api.rst in CPUID and only exposes Hyper-V identification. In this case, guest · line 6151
Quoted 10 lines
Documentation/virt/kvm/api.rst +2
-28.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.
Signature · 1 line
Jazz is not dead. It just smells funny...
PATCH 5/5 arm64/x86: KVM: Introduce steal-time cap
In reply to Andrew Jonesarm64 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.
--- 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(-)
Documentation/virt/kvm/api.rst +11 -0
@@ -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".
arch/arm64/include/asm/kvm_host.h +1 -0
@@ -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,
arch/arm64/kvm/arm.c +3 -0
@@ -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 ;
arch/arm64/kvm/pvtime.c +1 -1
@@ -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 (); }
arch/x86/kvm/x86.c +3 -0
@@ -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 ; }
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
Signature · 1 line
2.25.4
Marc Zyngier
maintainerRe: [PATCH 5/5] arm64/x86: KVM: Introduce steal-time cap
Quoted 28 lines · Andrew Jones
arm64 requires a vcpu fd (
KVM_HAS_DEVICE_ATTRvcpu 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 <[email protected]>Signed-off-by Andrew Jones <[email protected]>--- 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(-)Documentation/virt/kvm/api.rst +1
-0@@ -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. +
Documentation/virt/kvm/api.rst KVM can therefore start protected VMs. · line 6165
Quoted 10 lines
Documentation/virt/kvm/api.rst +10
-0+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?
Quoted 62 lines
arch/arm64/include/asm/kvm_host.h +1
-0@@ -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,arch/arm64/kvm/arm.c +3
-0@@ -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);arch/arm64/kvm/arm.c +0
-0break ;arch/arm64/kvm/pvtime.c +1
-1@@ -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 (); }arch/x86/kvm/x86.c +3
-0@@ -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 ;arch/x86/kvm/x86.c +0
-0}include/uapi/linux/kvm.h +0
-0
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
Otherwise looks ok.
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Quoted 7 lines · Andrew Jones
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
Quoted 23 lines
Thanks,
drewAndrew 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 capDocumentation/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(-)Signature · 1 line
2.25.4
Quoted 7 lines · Andrew Jones
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:
Quoted 19 lines
Thanks,
drewAndrew 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 capDocumentation/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 Marc,
Ping?
Thanks,
drew
Quoted 30 lines · Andrew Jones
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,
drewAndrew 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 capDocumentation/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(-)Signature · 1 line
2.25.4
Marc Zyngier
maintainerQuoted 3 lines · Andrew Jones
Hi Marc,
Ping?
On it! ;-)
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Marc Zyngier
maintainerQuoted 18 lines · Andrew Jones
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,
drewAndrew 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
Quoted 8 lines
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.
Signature · 1 line
Jazz is not dead. It just smells funny...
Quoted 28 lines · Marc Zyngier
Quoted 18 lines · Andrew Jones
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,
drewAndrew 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 capQuoted 8 lines
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(-)
Quoted 3 lines
Hi Andrew,
Sorry about the time it took to get to this series.
No problem.
Quoted 2 lines
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.
Quoted 1 line
(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
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm