Re: [PATCH v2 2/2] KVM: arm64: Commit pending PC adjustemnts before returning to userspace

[Marc Zyngier <maz@kernel.org>]

On 2021-05-14 15:08, Alexandru Elisei wrote:
> Hi Marc,
> 
> On 5/14/21 11:40 AM, Marc Zyngier wrote:
>> KVM currently updates PC (and the corresponding exception state)
>> using a two phase approach: first by setting a set of flags,
>> then by converting these flags into a state update when the vcpu
>> is about to enter the guest.
>> 
>> However, this creates a disconnect with userspace if the vcpu thread
>> returns there with any exception/PC flag set. In this case, the 
>> exposed
>> context is wrong, as userpsace doesn't have access to these flags
> 
> Nitpick: s/userpsace/userspace

Ah, forgot that one.

> 
>> (they aren't architectural). It also means that these flags are
>> preserved across a reset, which isn't expected.
>> 
>> To solve this problem, force an explicit synchronisation of the
>> exception state on vcpu exit to userspace. As an optimisation
>> for nVHE systems, only perform this when there is something pending.
>> 
>> Reported-by: Zenghui Yu <yuzenghui@huawei.com>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> Cc: stable@vger.kernel.org # 5.11
>> ---
>>  arch/arm64/include/asm/kvm_asm.h   |  1 +
>>  arch/arm64/kvm/arm.c               | 10 ++++++++++
>>  arch/arm64/kvm/hyp/exception.c     |  4 ++--
>>  arch/arm64/kvm/hyp/nvhe/hyp-main.c |  8 ++++++++
>>  4 files changed, 21 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/arm64/include/asm/kvm_asm.h 
>> b/arch/arm64/include/asm/kvm_asm.h
>> index d5b11037401d..5e9b33cbac51 100644
>> --- a/arch/arm64/include/asm/kvm_asm.h
>> +++ b/arch/arm64/include/asm/kvm_asm.h
>> @@ -63,6 +63,7 @@
>>  #define __KVM_HOST_SMCCC_FUNC___pkvm_cpu_set_vector		18
>>  #define __KVM_HOST_SMCCC_FUNC___pkvm_prot_finalize		19
>>  #define __KVM_HOST_SMCCC_FUNC___pkvm_mark_hyp			20
>> +#define __KVM_HOST_SMCCC_FUNC___kvm_adjust_pc			21
>> 
>>  #ifndef __ASSEMBLY__
>> 
>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> index 1cb39c0803a4..c4fe2b71f429 100644
>> --- a/arch/arm64/kvm/arm.c
>> +++ b/arch/arm64/kvm/arm.c
>> @@ -897,6 +897,16 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu 
>> *vcpu)
>> 
>>  	kvm_sigset_deactivate(vcpu);
>> 
>> +	/*
>> +	 * In the unlikely event that we are returning to userspace
>> +	 * with pending exceptions or PC adjustment, commit these
>> +	 * adjustments in order to give userspace a consistent view of
>> +	 * the vcpu state.

I didn't managed to commit this:

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index c4fe2b71f429..1126eae27400 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -901,7 +901,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
          * In the unlikely event that we are returning to userspace
          * with pending exceptions or PC adjustment, commit these
          * adjustments in order to give userspace a consistent view of
-        * the vcpu state.
+        * the vcpu state. Note that this relies on __kvm_adjust_pc()
+        * being preempt-safe on VHE.
          */
         if (unlikely(vcpu->arch.flags & (KVM_ARM64_PENDING_EXCEPTION |
                                          KVM_ARM64_INCREMENT_PC)))

I promise I'll fold that in!

>> +	 */
>> +	if (unlikely(vcpu->arch.flags & (KVM_ARM64_PENDING_EXCEPTION |
>> +					 KVM_ARM64_INCREMENT_PC)))
>> +		kvm_call_hyp(__kvm_adjust_pc, vcpu);
>> +
>>  	vcpu_put(vcpu);
>>  	return ret;
>>  }
>> diff --git a/arch/arm64/kvm/hyp/exception.c 
>> b/arch/arm64/kvm/hyp/exception.c
>> index 0812a496725f..11541b94b328 100644
>> --- a/arch/arm64/kvm/hyp/exception.c
>> +++ b/arch/arm64/kvm/hyp/exception.c
>> @@ -331,8 +331,8 @@ static void kvm_inject_exception(struct kvm_vcpu 
>> *vcpu)
>>  }
>> 
>>  /*
>> - * Adjust the guest PC on entry, depending on flags provided by EL1
>> - * for the purpose of emulation (MMIO, sysreg) or exception 
>> injection.
>> + * Adjust the guest PC (and potentially exception state) depending on
>> + * flags provided by the emulation code.
>>   */
>>  void __kvm_adjust_pc(struct kvm_vcpu *vcpu)
>>  {
>> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c 
>> b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
>> index f36420a80474..1632f001f4ed 100644
>> --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
>> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
>> @@ -28,6 +28,13 @@ static void handle___kvm_vcpu_run(struct 
>> kvm_cpu_context *host_ctxt)
>>  	cpu_reg(host_ctxt, 1) =  __kvm_vcpu_run(kern_hyp_va(vcpu));
>>  }
>> 
>> +static void handle___kvm_adjust_pc(struct kvm_cpu_context *host_ctxt)
>> +{
>> +	DECLARE_REG(struct kvm_vcpu *, vcpu, host_ctxt, 1);
>> +
>> +	__kvm_adjust_pc(kern_hyp_va(vcpu));
>> +}
>> +
>>  static void handle___kvm_flush_vm_context(struct kvm_cpu_context 
>> *host_ctxt)
>>  {
>>  	__kvm_flush_vm_context();
>> @@ -170,6 +177,7 @@ typedef void (*hcall_t)(struct kvm_cpu_context *);
>> 
>>  static const hcall_t host_hcall[] = {
>>  	HANDLE_FUNC(__kvm_vcpu_run),
>> +	HANDLE_FUNC(__kvm_adjust_pc),
>>  	HANDLE_FUNC(__kvm_flush_vm_context),
>>  	HANDLE_FUNC(__kvm_tlb_flush_vmid_ipa),
>>  	HANDLE_FUNC(__kvm_tlb_flush_vmid),
> 
> I'm guessing that the comment mentioned in the cover letter should be 
> in this
> patch, right? Or is the changelog from the cover letter stale?
> 
> Regardless, I trust your judgement regarding the comment and the patch
> looks correct:
> 
> Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel