Re: [PATCH v3] KVM: arm64: Add early_param to control WFx trapping

[Marc Zyngier <maz@kernel.org>]

On Wed, 10 Apr 2024 18:54:37 +0100,
Colton Lewis <coltonlewis@google.com> wrote:
> 
> Add an early_param to control WFx (WFI or WFE) trapping. This is so
> interrupts can be passed through if the CPU has support for direct
> interrupt injection, a feature of GICv4. This is described as an
> enumeration with three possible behaviors, always passthrough (never
> trap), never passthrough (always trap), or default (trap if more than
> one task is running. Default matches the current behavior.
> 
> Signed-off-by: Colton Lewis <coltonlewis@google.com>
> ---
> v3:
> * Changed control mechanism to an early_param on Marc's advice this should be
>   a system level decision and not exposed via uapi
> * Reduced behavior to an enum from an integer as there are only a few options
>   that make logical sense
> * Limit option for always passthrough to systems with GICv4 since the primary
>   case for always passthrough is systems with direct interrupt injection
> 
> v2:
> https://lore.kernel.org/kvmarm/20240319164341.1674863-1-coltonlewis@google.com/
> 
> v1:
> https://lore.kernel.org/kvmarm/20240129213918.3124494-1-coltonlewis@google.com/
> 
> arch/arm64/include/asm/kvm_host.h |  7 +++++++
>  arch/arm64/kvm/arm.c              | 30 +++++++++++++++++++++++++++++-
>  2 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 21c57b812569..e9225b1d0e9b 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_interrupt_passthrough {
> +	KVM_INTERRUPT_PASSTHROUGH_DEFAULT,
> +	KVM_INTERRUPT_PASSTHROUGH_ALWAYS,
> +	KVM_INTERRUPT_PASSTHROUGH_NEVER,

What does this mean? This is not dealing with interrupts, this is
supposed to deal with the behaviour of specific instructions
(WFI/WFE). The notion of "passthrough" is really odd as well. Finally,
both ALWAYS and NEVER are wrong -- the architecture makes no such
guarantee.

> +};
> +
>  #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..5d0ea6b2c652 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -46,6 +46,7 @@
>  #include <kvm/arm_psci.h>
> 
>  static enum kvm_mode kvm_mode = KVM_MODE_DEFAULT;
> +static enum kvm_interrupt_passthrough kvm_interrupt_passthrough = KVM_INTERRUPT_PASSTHROUGH_DEFAULT;
> 
>  DECLARE_KVM_HYP_PER_CPU(unsigned long, kvm_hyp_vector);
> 
> @@ -456,7 +457,10 @@ 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())
> +	if ((kvm_interrupt_passthrough == KVM_INTERRUPT_PASSTHROUGH_ALWAYS
> +	     && kvm_vgic_global_state.has_gicv4) ||
> +	    (kvm_interrupt_passthrough == KVM_INTERRUPT_PASSTHROUGH_DEFAULT
> +	     && single_task_running()))

Why is this affecting both WFI and WFE? They are very different and
lumping them together makes little sense.

>  		vcpu_clear_wfx_traps(vcpu);
>  	else
>  		vcpu_set_wfx_traps(vcpu);
> @@ -2654,6 +2658,30 @@ static int __init early_kvm_mode_cfg(char *arg)
>  }
>  early_param("kvm-arm.mode", early_kvm_mode_cfg);
> 
> +static int __init early_kvm_interrupt_passthrough_cfg(char *arg)
> +{
> +	if (!arg)
> +		return -EINVAL;
> +
> +	if (strcmp(arg, "always") == 0) {
> +		kvm_interrupt_passthrough = KVM_INTERRUPT_PASSTHROUGH_ALWAYS;
> +		return 0;
> +	}
> +
> +	if (strcmp(arg, "never") == 0) {
> +		kvm_interrupt_passthrough = KVM_INTERRUPT_PASSTHROUGH_NEVER;
> +		return 0;
> +	}
> +
> +	if (strcmp(arg, "default") == 0) {
> +		kvm_interrupt_passthrough = KVM_INTERRUPT_PASSTHROUGH_DEFAULT;
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +early_param("kvm-arm.interrupt-passthrough", early_kvm_interrupt_passthrough_cfg);
> +

Again, this is not dealing with interrupts. This is dealing with the
*potential* trapping of instructions in certain circumstances.

>  enum kvm_mode kvm_get_mode(void)
>  {
>  	return kvm_mode;

Finally, this needs to be documented.

Thanks,

	M.

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