Re: [PATCH v2 3/3] KVM: arm64: Use generic KVM xfer to guest work function

From: Marc Zyngier <maz@kernel.org>
To: Oliver Upton <oupton@google.com>
Cc: kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
	James Morse <james.morse@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Peter Zijlstra <peterz@infradead.org>,
	Andy Lutomirski <luto@kernel.org>,
	linux-arm-kernel@lists.infradead.org,
	Peter Shier <pshier@google.com>,
	Shakeel Butt <shakeelb@google.com>,
	Guangyu Shi <guangyus@google.com>
Subject: Re: [PATCH v2 3/3] KVM: arm64: Use generic KVM xfer to guest work function
Date: Fri, 30 Jul 2021 10:41:24 +0100	[thread overview]
Message-ID: <878s1o2l6j.wl-maz@kernel.org> (raw)
In-Reply-To: <20210729220916.1672875-4-oupton@google.com>

Hi Oliver,

On Thu, 29 Jul 2021 23:09:16 +0100,
Oliver Upton <oupton@google.com> wrote:
> 
> Clean up handling of checks for pending work by switching to the generic
> infrastructure to do so.
> 
> We pick up handling for TIF_NOTIFY_RESUME from this switch, meaning that
> task work will be correctly handled.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/kvm/Kconfig |  1 +
>  arch/arm64/kvm/arm.c   | 27 ++++++++++++++-------------
>  2 files changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
> index a4eba0908bfa..8bc1fac5fa26 100644
> --- a/arch/arm64/kvm/Kconfig
> +++ b/arch/arm64/kvm/Kconfig
> @@ -26,6 +26,7 @@ menuconfig KVM
>  	select HAVE_KVM_ARCH_TLB_FLUSH_ALL
>  	select KVM_MMIO
>  	select KVM_GENERIC_DIRTYLOG_READ_PROTECT
> +	select KVM_XFER_TO_GUEST_WORK
>  	select SRCU
>  	select KVM_VFIO
>  	select HAVE_KVM_EVENTFD
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 60d0a546d7fd..9762e2129813 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -6,6 +6,7 @@
>  
>  #include <linux/bug.h>
>  #include <linux/cpu_pm.h>
> +#include <linux/entry-kvm.h>
>  #include <linux/errno.h>
>  #include <linux/err.h>
>  #include <linux/kvm_host.h>
> @@ -714,6 +715,13 @@ static bool vcpu_mode_is_bad_32bit(struct kvm_vcpu *vcpu)
>  		static_branch_unlikely(&arm64_mismatched_32bit_el0);
>  }
>  
> +static bool kvm_vcpu_exit_request(struct kvm_vcpu *vcpu)
> +{
> +	return kvm_request_pending(vcpu) ||
> +			need_new_vmid_gen(&vcpu->arch.hw_mmu->vmid) ||
> +			xfer_to_guest_mode_work_pending();

Here's what xfer_to_guest_mode_work_pending() says:

<quote>
 * Has to be invoked with interrupts disabled before the transition to
 * guest mode.
</quote>

At the point where you call this, we already are in guest mode, at
least in the KVM sense.

> +}
> +
>  /**
>   * kvm_arch_vcpu_ioctl_run - the main VCPU run function to execute guest code
>   * @vcpu:	The VCPU pointer
> @@ -757,7 +765,11 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>  		/*
>  		 * Check conditions before entering the guest
>  		 */
> -		cond_resched();
> +		if (__xfer_to_guest_mode_work_pending()) {
> +			ret = xfer_to_guest_mode_handle_work(vcpu);

xfer_to_guest_mode_handle_work() already does the exact equivalent of
__xfer_to_guest_mode_work_pending(). Why do we need to do it twice?

> +			if (!ret)
> +				ret = 1;
> +		}
>  
>  		update_vmid(&vcpu->arch.hw_mmu->vmid);
>  
> @@ -776,16 +788,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>  
>  		kvm_vgic_flush_hwstate(vcpu);
>  
> -		/*
> -		 * Exit if we have a signal pending so that we can deliver the
> -		 * signal to user space.
> -		 */
> -		if (signal_pending(current)) {
> -			ret = -EINTR;
> -			run->exit_reason = KVM_EXIT_INTR;
> -			++vcpu->stat.signal_exits;
> -		}
> -
>  		/*
>  		 * If we're using a userspace irqchip, then check if we need
>  		 * to tell a userspace irqchip about timer or PMU level
> @@ -809,8 +811,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>  		 */
>  		smp_store_mb(vcpu->mode, IN_GUEST_MODE);
>  
> -		if (ret <= 0 || need_new_vmid_gen(&vcpu->arch.hw_mmu->vmid) ||
> -		    kvm_request_pending(vcpu)) {
> +		if (ret <= 0 || kvm_vcpu_exit_request(vcpu)) {

If you are doing this, please move the userspace irqchip handling into
the helper as well, so that we have a single function dealing with
collecting exit reasons.

>  			vcpu->mode = OUTSIDE_GUEST_MODE;
>  			isb(); /* Ensure work in x_flush_hwstate is committed */
>  			kvm_pmu_sync_hwstate(vcpu);

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.