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

Marc Zyngier
2021-07-30T09:41:29Z

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.