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 <[email protected]>
---
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/[email protected]/
v1:
https://lore.kernel.org/kvmarm/[email protected]/
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,
+};
+
#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()))
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);
+
enum kvm_mode kvm_get_mode(void)
{
return kvm_mode;
--
2.44.0.478.gd926399ef9-goog
On Wed, 10 Apr 2024 18:54:37 +0100,
Colton Lewis <[email protected]> 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 <[email protected]>
> ---
> 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/[email protected]/
>
> v1:
> https://lore.kernel.org/kvmarm/[email protected]/
>
> 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.
Thanks for the review Marc.
Marc Zyngier <[email protected]> writes:
> On Wed, 10 Apr 2024 18:54:37 +0100,
> Colton Lewis <[email protected]> wrote:
>> +
>> +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.
Looking at this, I did let the language get away from me by mixing up
interrupts and the instructions dealing with them.
"Passthrough" is not a technical term but has pervaded some of my
internal conversations about this and I've just been using it to mean
the opposite of trapping. That can be easily swapped.
I understand always and never are not what the architecture guarantees,
but was trying to capture what KVM code is attempting to do. I could
just drop it entirely.
So the enum values could be named something like:
KVM_WFX_TRAP
KVM_WFX_NOTRAP
KVM_WFX_NOTRAP_SINGLE_TASK (default option)
>> - 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.
It's true they are different, but I couldn't think of any cases where
you would want trapping for one to be different than for the other. The
current behavior also assumes trapping should be the same for both.
Are you suggesting separate controls for the two?
>> @@ -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.
Understood. Should be something like "kvm-arm.wfx-instruction-trapping".
>> enum kvm_mode kvm_get_mode(void)
>> {
>> return kvm_mode;
> Finally, this needs to be documented.
Right, in Documentation/admin-guide/kernel-parameters.txt