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

[Colton Lewis]

Thanks for the review Marc.

Marc Zyngier <maz@xxxxxxxxxx> writes:

> On Wed, 10 Apr 2024 18:54:37 +0100,
> Colton Lewis <coltonlewis@xxxxxxxxxx> 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