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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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

Hi Oliver,

On Thu, 29 Jul 2021 23:09:16 +0100,
Oliver Upton <oupton@...gle.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@...gle.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.