Received-SPF: pass (google.com: domain of linux-kernel+bounces-154060-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Mon, 22 Apr 2024 14:43:32 -0700
From: Oliver Upton <oliver.upton@linux.dev>
To: Colton Lewis <coltonlewis@google.com>
Cc: kvm@vger.kernel.org, Jonathan Corbet <corbet@lwn.net>,
	Marc Zyngier <maz@kernel.org>, James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Zenghui Yu <yuzenghui@huawei.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>, linux-doc@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	kvmarm@lists.linux.dev
Subject: Re: [PATCH v4] KVM: arm64: Add early_param to control WFx trapping
Message-ID: <ZibaBKCFMz-dJNM4@linux.dev>
References: <20240422181716.237284-1-coltonlewis@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240422181716.237284-1-coltonlewis@google.com>

Hi Colton,

On Mon, Apr 22, 2024 at 06:17:16PM +0000, Colton Lewis wrote:
> Add an early_params to control WFI and WFE trapping. This is to
> control the degree guests can wait for interrupts on their own without
> being trapped by KVM. Options for each param are trap, notrap, and
> default. trap enables the trap. notrap disables the trap. default
> preserves current behavior, disabling the trap if only a single task
> is running and enabling otherwise.
> 
> Signed-off-by: Colton Lewis <coltonlewis@google.com>
> ---
> v4:
> 
> * Fixed inaccurate names that incorrectly implied this controls interrupts
>   themselves instead of instructions waiting for interrupts and events
> * Split into two separate params as interrupts (WFI) and events (WFE) do
>   different things and may warrant separate controls.
> * Document new params in Documentation/admin-guide/kernel-parameters.txt
> 
> 
> v3:
> https://lore.kernel.org/kvmarm/20240410175437.793508-1-coltonlewis@google.com/
> 
> v2:
> https://lore.kernel.org/kvmarm/20240319164341.1674863-1-coltonlewis@google.com/
> 
> v1:
> https://lore.kernel.org/kvmarm/20240129213918.3124494-1-coltonlewis@google.com/
> 
>  .../admin-guide/kernel-parameters.txt         | 22 +++++++-
>  arch/arm64/include/asm/kvm_emulate.h          | 24 ++++++++-
>  arch/arm64/include/asm/kvm_host.h             |  7 +++
>  arch/arm64/kvm/arm.c                          | 54 +++++++++++++++++--
>  4 files changed, 101 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 31b3a25680d0..f8d16c792e66 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2653,6 +2653,27 @@
>  			[KVM,ARM] Allow use of GICv4 for direct injection of
>  			LPIs.
> 
> +	kvm-arm.wfe_trap_policy=
> +			[KVM,ARM] Control when to set wfe instruction trap.

nitpick: when referring to the instruction, please capitalize it.

Also, it doesn't hurt to be verbose here and say this cmdline option
"Controls the WFE instruction trap behavior for KVM VMs"

I say this because there is a separate set of trap controls that allow
WFE or WFI to execute in EL0 (i.e. host userspace).

> +			trap: set wfe instruction trap
> +
> +			notrap: clear wfe instruction trap
> +
> +			default: set wfe instruction trap only if multiple
> +				 tasks are running on the CPU

I would strongly prefer we not make any default behavior user-visible.
The default KVM behavior can (and will) change in the future.

Only the absence of an explicit trap / notrap policy should fall back to
KVM's default heuristics.

> +	kvm-arm.wfi_trap_policy=
> +			[KVM,ARM] Control when to set wfi instruction trap.
> +
> +			trap: set wfi instruction trap
> +
> +			notrap: clear wfi instruction trap
> +
> +			default: set wfi instruction trap only if multiple
> +				 tasks are running on the CPU
> +
> +
>  	kvm_cma_resv_ratio=n [PPC]
>  			Reserves given percentage from system memory area for
>  			contiguous memory allocation for KVM hash pagetable
> @@ -7394,4 +7415,3 @@
>  				memory, and other data can't be written using
>  				xmon commands.
>  			off	xmon is disabled.
> -
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index b804fe832184..efd0a3fb6f00 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -109,9 +109,13 @@ static inline unsigned long *vcpu_hcr(struct kvm_vcpu *vcpu)
>  	return (unsigned long *)&vcpu->arch.hcr_el2;
>  }
> 
> -static inline void vcpu_clear_wfx_traps(struct kvm_vcpu *vcpu)
> +static inline void vcpu_clear_wfe_trap(struct kvm_vcpu *vcpu)
>  {
>  	vcpu->arch.hcr_el2 &= ~HCR_TWE;
> +}
> +
> +static inline void vcpu_clear_wfi_trap(struct kvm_vcpu *vcpu)
> +{
>  	if (atomic_read(&vcpu->arch.vgic_cpu.vgic_v3.its_vpe.vlpi_count) ||
>  	    vcpu->kvm->arch.vgic.nassgireq)
>  		vcpu->arch.hcr_el2 &= ~HCR_TWI;
> @@ -119,12 +123,28 @@ static inline void vcpu_clear_wfx_traps(struct kvm_vcpu *vcpu)
>  		vcpu->arch.hcr_el2 |= HCR_TWI;
>  }

This helper definitely does not do as it says on the tin. It ignores the
policy requested on the command line and conditionally *sets* TWI. If
the operator believes they know best and ask for a particular trap policy
KVM should uphold it unconditionally. Even if they've managed to shoot
themselves in the foot.

> -static inline void vcpu_set_wfx_traps(struct kvm_vcpu *vcpu)
> +static inline void vcpu_clear_wfx_traps(struct kvm_vcpu *vcpu)
> +{
> +	vcpu_clear_wfe_trap(vcpu);
> +	vcpu_clear_wfi_trap(vcpu);
> +}
> +
> +static inline void vcpu_set_wfe_trap(struct kvm_vcpu *vcpu)
>  {
>  	vcpu->arch.hcr_el2 |= HCR_TWE;
> +}
> +
> +static inline void vcpu_set_wfi_trap(struct kvm_vcpu *vcpu)
> +{
>  	vcpu->arch.hcr_el2 |= HCR_TWI;
>  }
> 
> +static inline void vcpu_set_wfx_traps(struct kvm_vcpu *vcpu)
> +{
> +	vcpu_set_wfe_trap(vcpu);
> +	vcpu_set_wfi_trap(vcpu);
> +}
> +
>  static inline void vcpu_ptrauth_enable(struct kvm_vcpu *vcpu)
>  {
>  	vcpu->arch.hcr_el2 |= (HCR_API | HCR_APK);
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 21c57b812569..315ee7bfc1cb 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -67,6 +67,13 @@ enum kvm_mode {
>  	KVM_MODE_NV,
>  	KVM_MODE_NONE,
>  };
> +
> +enum kvm_wfx_trap_policy {
> +	KVM_WFX_NOTRAP_SINGLE_TASK, /* Default option */
> +	KVM_WFX_NOTRAP,
> +	KVM_WFX_TRAP,
> +};
> +
>  #ifdef CONFIG_KVM
>  enum kvm_mode kvm_get_mode(void);
>  #else
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index a25265aca432..5106ba5a8a39 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -46,6 +46,8 @@
>  #include <kvm/arm_psci.h>
> 
>  static enum kvm_mode kvm_mode = KVM_MODE_DEFAULT;
> +static enum kvm_wfx_trap_policy kvm_wfi_trap_policy = KVM_WFX_NOTRAP_SINGLE_TASK;
> +static enum kvm_wfx_trap_policy kvm_wfe_trap_policy = KVM_WFX_NOTRAP_SINGLE_TASK;
> 
>  DECLARE_KVM_HYP_PER_CPU(unsigned long, kvm_hyp_vector);
> 
> @@ -423,6 +425,12 @@ void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu)
> 
>  }
> 
> +static bool kvm_should_clear_wfx_trap(enum kvm_wfx_trap_policy p)
> +{
> +	return (p == KVM_WFX_NOTRAP && kvm_vgic_global_state.has_gicv4)
> +		|| (p == KVM_WFX_NOTRAP_SINGLE_TASK && single_task_running());
> +}

style nitpick: operators should always go on the preceding line for a
multi-line statement.

>  void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  {
>  	struct kvm_s2_mmu *mmu;
> @@ -456,10 +464,15 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  	if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
>  		kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
> 
> -	if (single_task_running())
> -		vcpu_clear_wfx_traps(vcpu);
> +	if (kvm_should_clear_wfx_trap(kvm_wfi_trap_policy))
> +		vcpu_clear_wfi_trap(vcpu);
>  	else
> -		vcpu_set_wfx_traps(vcpu);
> +		vcpu_set_wfi_trap(vcpu);
> +
> +	if (kvm_should_clear_wfx_trap(kvm_wfe_trap_policy))
> +		vcpu_clear_wfe_trap(vcpu);
> +	else
> +		vcpu_set_wfe_trap(vcpu);
> 
>  	if (vcpu_has_ptrauth(vcpu))
>  		vcpu_ptrauth_disable(vcpu);

I find all of the layering rather hard to follow; we don't need
accessors for doing simple bit manipulation.

Rough sketch:

static bool kvm_vcpu_should_clear_twi(struct kvm_vcpu *vcpu)
{
	if (unlikely(kvm_wfi_trap != KVM_WFX_DEFAULT))
		return kvm_wfi_trap == KVM_WFX_NOTRAP;

	return single_task_running() &&
	       (atomic_read(&vcpu->arch.vgic_cpu.vgic_v3.its_vpe.vlpi_count) ||
	        vcpu->kvm->arch.vgic.nassgireq);
}

static bool kvm_vcpu_should_clear_twe(struct kvm_vcpu *vcpu)
{
	if (unlikely(kvm_wfe_trap != KVM_WFX_DEFAULT))
		return kvm_wfe_trap == KVM_WFX_NOTRAP;

	return single_task_running();
}

static void kvm_vcpu_load_compute_hcr(struct kvm_vcpu *vcpu)
{
	vcpu->arch.hcr_el2 |= HCR_TWE | HCR_TWI;

	if (kvm_vcpu_should_clear_twe(vcpu))
		vcpu->arch.hcr_el2 &= ~HCR_TWE;
	if (kvm_vcpu_should_clear_twi(vcpu))
		vcpu->arch.hcr_el2 &= ~HCR_TWI;
}

And if we really wanted to, the non-default trap configuration could be
moved to vcpu_reset_hcr() if we cared.

-- 
Thanks,
Oliver