Re: [PATCH v2 3/6] KVM: arm64: Support pvlock preempted via shared structure

[yezengruan]

Hi Steve,

On 2020/1/9 23:02, Steven Price wrote:
> On 26/12/2019 13:58, Zengruan Ye wrote:
>> Implement the service call for configuring a shared structure between a
>> VCPU and the hypervisor in which the hypervisor can tell the VCPU is
>> running or not.
>>
>> The preempted field is zero if 1) some old KVM deos not support this filed.
> 
> NIT: s/deos/does/

Thanks for posting this.

> 
> However, I would hope that the service call will fail if it's an old KVM not simply return zero.

Sorry, I'm not sure what you mean. The service call will fail if it's an old KVM, and the Guest will use __native_vcpu_is_preempted.

> 
>> 2) the VCPU is not preempted. Other values means the VCPU has been preempted.
>>
>> Signed-off-by: Zengruan Ye <yezengruan@xxxxxxxxxx>
>> ---
>> Â arch/arm/include/asm/kvm_host.hÂÂ | 18 ++++++++++++
>> Â arch/arm64/include/asm/kvm_host.h | 19 +++++++++++++
>> Â arch/arm64/kvm/MakefileÂÂÂÂÂÂÂÂÂÂ |Â 1 +
>> Â virt/kvm/arm/arm.cÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |Â 8 ++++++
>> Â virt/kvm/arm/hypercalls.cÂÂÂÂÂÂÂÂ |Â 8 ++++++
>> Â virt/kvm/arm/pvlock.cÂÂÂÂÂÂÂÂÂÂÂÂ | 46 +++++++++++++++++++++++++++++++
>> Â 6 files changed, 100 insertions(+)
>> Â create mode 100644 virt/kvm/arm/pvlock.c
>>
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index 556cd818eccf..dfeaf9204875 100644
>> --- a/arch/arm/include/asm/kvm_host.h
>> +++ b/arch/arm/include/asm/kvm_host.h
>> @@ -356,6 +356,24 @@ static inline bool kvm_arm_is_pvtime_enabled(struct kvm_vcpu_arch *vcpu_arch)
>> ÂÂÂÂÂ return false;
>> Â }
>> Â +static inline void kvm_arm_pvlock_preempted_init(struct kvm_vcpu_arch *vcpu_arch)
>> +{
>> +}
>> +
>> +static inline bool kvm_arm_is_pvlock_preempted_ready(struct kvm_vcpu_arch *vcpu_arch)
>> +{
>> +ÂÂÂ return false;
>> +}
>> +
>> +static inline gpa_t kvm_init_pvlock(struct kvm_vcpu *vcpu)
>> +{
>> +ÂÂÂ return GPA_INVALID;
>> +}
>> +
>> +static inline void kvm_update_pvlock_preempted(struct kvm_vcpu *vcpu, u64 preempted)
>> +{
>> +}
>> +
>> Â void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot);
>> Â Â struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index c61260cf63c5..2818a2330f92 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -354,6 +354,12 @@ struct kvm_vcpu_arch {
>> ÂÂÂÂÂÂÂÂÂ u64 last_steal;
>> ÂÂÂÂÂÂÂÂÂ gpa_t base;
>> ÂÂÂÂÂ } steal;
>> +
>> +ÂÂÂ /* Guest PV lock state */
>> +ÂÂÂ struct {
>> +ÂÂÂÂÂÂÂ u64 preempted;
> 
> I'm not sure why the kernel needs to (separately) track this preempted state? It doesn't appear to be used from what I can tell.

Good point, the preempted state field is not actually used, I'll remove it.

> 
> Steve
> 
>> +ÂÂÂÂÂÂÂ gpa_t base;
>> +ÂÂÂ } pv;
>> Â };
>> Â Â /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
>> @@ -515,6 +521,19 @@ static inline bool kvm_arm_is_pvtime_enabled(struct kvm_vcpu_arch *vcpu_arch)
>> ÂÂÂÂÂ return (vcpu_arch->steal.base != GPA_INVALID);
>> Â }
>> Â +static inline void kvm_arm_pvlock_preempted_init(struct kvm_vcpu_arch *vcpu_arch)
>> +{
>> +ÂÂÂ vcpu_arch->pv.base = GPA_INVALID;
>> +}
>> +
>> +static inline bool kvm_arm_is_pvlock_preempted_ready(struct kvm_vcpu_arch *vcpu_arch)
>> +{
>> +ÂÂÂ return (vcpu_arch->pv.base != GPA_INVALID);
>> +}
>> +
>> +gpa_t kvm_init_pvlock(struct kvm_vcpu *vcpu);
>> +void kvm_update_pvlock_preempted(struct kvm_vcpu *vcpu, u64 preempted);
>> +
>> Â void kvm_set_sei_esr(struct kvm_vcpu *vcpu, u64 syndrome);
>> Â Â struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
>> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
>> index 5ffbdc39e780..e4591f56d5f1 100644
>> --- a/arch/arm64/kvm/Makefile
>> +++ b/arch/arm64/kvm/Makefile
>> @@ -15,6 +15,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arm.o $(KVM)/arm/mmu.o $(KVM)/arm/mmio.
>> Â kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/psci.o $(KVM)/arm/perf.o
>> Â kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/hypercalls.o
>> Â kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/pvtime.o
>> +kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/pvlock.o
>> Â Â kvm-$(CONFIG_KVM_ARM_HOST) += inject_fault.o regmap.o va_layout.o
>> Â kvm-$(CONFIG_KVM_ARM_HOST) += hyp.o hyp-init.o handle_exit.o
>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>> index 8de4daf25097..36d57e77d3c4 100644
>> --- a/virt/kvm/arm/arm.c
>> +++ b/virt/kvm/arm/arm.c
>> @@ -383,6 +383,8 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
>> Â ÂÂÂÂÂ kvm_arm_pvtime_vcpu_init(&vcpu->arch);
>> Â +ÂÂÂ kvm_arm_pvlock_preempted_init(&vcpu->arch);
>> +
>> ÂÂÂÂÂ return kvm_vgic_vcpu_init(vcpu);
>> Â }
>> Â @@ -421,6 +423,9 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>> ÂÂÂÂÂÂÂÂÂ vcpu_set_wfx_traps(vcpu);
>> Â ÂÂÂÂÂ vcpu_ptrauth_setup_lazy(vcpu);
>> +
>> +ÂÂÂ if (kvm_arm_is_pvlock_preempted_ready(&vcpu->arch))
>> +ÂÂÂÂÂÂÂ kvm_update_pvlock_preempted(vcpu, 0);
>> Â }
>> Â Â void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>> @@ -434,6 +439,9 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>> ÂÂÂÂÂ vcpu->cpu = -1;
>> Â ÂÂÂÂÂ kvm_arm_set_running_vcpu(NULL);
>> +
>> +ÂÂÂ if (kvm_arm_is_pvlock_preempted_ready(&vcpu->arch))
>> +ÂÂÂÂÂÂÂ kvm_update_pvlock_preempted(vcpu, 1);
>> Â }
>> Â Â static void vcpu_power_off(struct kvm_vcpu *vcpu)
>> diff --git a/virt/kvm/arm/hypercalls.c b/virt/kvm/arm/hypercalls.c
>> index 550dfa3e53cd..1c6a11f21bb4 100644
>> --- a/virt/kvm/arm/hypercalls.c
>> +++ b/virt/kvm/arm/hypercalls.c
>> @@ -52,6 +52,9 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
>> ÂÂÂÂÂÂÂÂÂ case ARM_SMCCC_HV_PV_TIME_FEATURES:
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ val = SMCCC_RET_SUCCESS;
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ break;
>> +ÂÂÂÂÂÂÂ case ARM_SMCCC_HV_PV_LOCK_FEATURES:
>> +ÂÂÂÂÂÂÂÂÂÂÂ val = SMCCC_RET_SUCCESS;
>> +ÂÂÂÂÂÂÂÂÂÂÂ break;
>> ÂÂÂÂÂÂÂÂÂ }
>> ÂÂÂÂÂÂÂÂÂ break;
>> ÂÂÂÂÂ case ARM_SMCCC_HV_PV_TIME_FEATURES:
>> @@ -62,6 +65,11 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
>> ÂÂÂÂÂÂÂÂÂ if (gpa != GPA_INVALID)
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ val = gpa;
>> ÂÂÂÂÂÂÂÂÂ break;
>> +ÂÂÂ case ARM_SMCCC_HV_PV_LOCK_PREEMPTED:
>> +ÂÂÂÂÂÂÂ gpa = kvm_init_pvlock(vcpu);
>> +ÂÂÂÂÂÂÂ if (gpa != GPA_INVALID)
>> +ÂÂÂÂÂÂÂÂÂÂÂ val = gpa;
>> +ÂÂÂÂÂÂÂ break;
>> ÂÂÂÂÂ default:
>> ÂÂÂÂÂÂÂÂÂ return kvm_psci_call(vcpu);
>> ÂÂÂÂÂ }
>> diff --git a/virt/kvm/arm/pvlock.c b/virt/kvm/arm/pvlock.c
>> new file mode 100644
>> index 000000000000..cdfd30a903b9
>> --- /dev/null
>> +++ b/virt/kvm/arm/pvlock.c
>> @@ -0,0 +1,46 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright(c) 2019 Huawei Technologies Co., Ltd
>> + * Author: Zengruan Ye <yezengruan@xxxxxxxxxx>
>> + */
>> +
>> +#include <linux/arm-smccc.h>
>> +#include <linux/kvm_host.h>
>> +
>> +#include <asm/pvlock-abi.h>
>> +
>> +#include <kvm/arm_hypercalls.h>
>> +
>> +gpa_t kvm_init_pvlock(struct kvm_vcpu *vcpu)
>> +{
>> +ÂÂÂ struct pvlock_vcpu_state init_values = {};
>> +ÂÂÂ struct kvm *kvm = vcpu->kvm;
>> +ÂÂÂ u64 base = vcpu->arch.pv.base;
>> +ÂÂÂ int idx;
>> +
>> +ÂÂÂ if (base == GPA_INVALID)
>> +ÂÂÂÂÂÂÂ return base;
>> +
>> +ÂÂÂ idx = srcu_read_lock(&kvm->srcu);
>> +ÂÂÂ kvm_write_guest(kvm, base, &init_values, sizeof(init_values));
>> +ÂÂÂ srcu_read_unlock(&kvm->srcu, idx);
>> +
>> +ÂÂÂ return base;
>> +}
>> +
>> +void kvm_update_pvlock_preempted(struct kvm_vcpu *vcpu, u64 preempted)
>> +{
>> +ÂÂÂ int idx;
>> +ÂÂÂ u64 offset;
>> +ÂÂÂ __le64 preempted_le;
>> +ÂÂÂ struct kvm *kvm = vcpu->kvm;
>> +ÂÂÂ u64 base = vcpu->arch.pv.base;
>> +
>> +ÂÂÂ vcpu->arch.pv.preempted = preempted;
>> +ÂÂÂ preempted_le = cpu_to_le64(preempted);
>> +
>> +ÂÂÂ idx = srcu_read_lock(&kvm->srcu);
>> +ÂÂÂ offset = offsetof(struct pvlock_vcpu_state, preempted);
>> +ÂÂÂ kvm_put_guest(kvm, base + offset, preempted_le, u64);
>> +ÂÂÂ srcu_read_unlock(&kvm->srcu, idx);
>> +}
>>
> 
> 
> .

Thanks,

Zengruan