[PATCH 0/5] KVM: arm64: pvtime: Fixes and a new cap

[PATCH 0/5] KVM: arm64: pvtime: Fixes and a new cap

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

[PATCH 1/5] KVM: arm64: pvtime: steal-time is only supported when configured

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

[PATCH 2/5] KVM: arm64: pvtime: Fix potential loss of stolen time

[Andrew Jones]

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

[PATCH 3/5] KVM: arm64: pvtime: Fix stolen time accounting across migration

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

[PATCH 4/5] KVM: Documentation minor fixups

[Andrew Jones]

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

[PATCH 5/5] 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, 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

Re: [PATCH 0/5] KVM: arm64: pvtime: Fixes and a new cap

[Andrew Jones]

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

Re: [PATCH 0/5] KVM: arm64: pvtime: Fixes and a new cap

[Steven Price]

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

Re: [PATCH 0/5] KVM: arm64: pvtime: Fixes and a new cap

[Andrew Jones]

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

Re: [PATCH 0/5] KVM: arm64: pvtime: Fixes and a new cap

[Marc Zyngier]

On 2020-07-27 12:02, Andrew Jones wrote:
> Hi Marc,
> 
> Ping?

On it! ;-)

         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

Re: [PATCH 1/5] KVM: arm64: pvtime: steal-time is only supported when configured

[Marc Zyngier]

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

Re: [PATCH 2/5] KVM: arm64: pvtime: Fix potential loss of stolen time

[Marc Zyngier]

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

Re: [PATCH 3/5] KVM: arm64: pvtime: Fix stolen time accounting across migration

[Marc Zyngier]

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

Re: [PATCH 4/5] KVM: Documentation minor fixups

[Marc Zyngier]

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

Re: [PATCH 5/5] arm64/x86: KVM: Introduce steal-time cap

[Marc Zyngier]

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

Re: [PATCH 0/5] KVM: arm64: pvtime: Fixes and a new cap

[Marc Zyngier]

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

Re: [PATCH 1/5] KVM: arm64: pvtime: steal-time is only supported when configured

[Andrew Jones]

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

Re: [PATCH 0/5] KVM: arm64: pvtime: Fixes and a new cap

[Andrew Jones]

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

Re: [PATCH 1/5] KVM: arm64: pvtime: steal-time is only supported when configured

[Marc Zyngier]

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

Re: [PATCH 1/5] KVM: arm64: pvtime: steal-time is only supported when configured

[Andrew Jones]

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