Re: [PATCH v2 3/3] KVM: s390: Introducing kvm_arch_set_irq_inatomic fast inject

[Matthew Rosato]

On 3/25/26 9:42 PM, Douglas Freimuth wrote:

> @@ -1963,15 +1963,10 @@ static int __inject_vm(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
>  }
>  
>  int kvm_s390_inject_vm(struct kvm *kvm,
> -		       struct kvm_s390_interrupt *s390int)
> +		       struct kvm_s390_interrupt *s390int, struct kvm_s390_interrupt_info *inti)

Sashiko makes a point here, sort of.

I believe it's wrong in the sense that you still left the code in this
function that frees inti on error paths, effectively freeing it under
the exact same circumstances as before this patch when it was allocated
in kvm_s390_inject_vm.

But now that you have moved allocation of inti to be the responsibility
of the caller of kvm_s390_inject_vm it generally would imply that the
caller should ALSO be responsible for freeing that memory.  But now both
the passing case and the failing case have given up control of that
memory and assume it's taken care of.

I think that's worth a comment explaining it.

[...]

> +/*
> + * kvm_arch_set_irq_inatomic: fast-path for irqfd injection
> + */
> +int kvm_arch_set_irq_inatomic(struct kvm_kernel_irq_routing_entry *e,
> +			      struct kvm *kvm, int irq_source_id, int level,
> +			      bool line_status)
> +{
> +	int ret;
> +	struct s390_io_adapter *adapter;
> +	struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int;
> +	struct kvm_s390_interrupt_info *inti;
> +	struct kvm_s390_interrupt s390int = {
> +			.type = KVM_S390_INT_IO(1, 0, 0, 0),
> +			.parm = 0,
> +	};
> +
> +	kvm->stat.io_390_inatomic++;
> +
> +	/* We're only interested in the 0->1 transition. */
> +	if (!level)
> +		return -EWOULDBLOCK;
> +	if (e->type != KVM_IRQ_ROUTING_S390_ADAPTER)
> +		return -EWOULDBLOCK;
> +
> +	adapter = get_io_adapter(kvm, e->adapter.adapter_id);
> +	if (!adapter)
> +		return -EWOULDBLOCK;
> +
> +	s390int.parm64 = isc_to_int_word(adapter->isc);
> +	ret = adapter_indicators_set_fast(kvm, adapter, &e->adapter);

Sashiko asks an interesting question around this point.  I think it's a
bit more broad than it states, however:

What happens if we -EWOULDBLOCK for any reason past this point and
adapter_indicators_set_fast above here has set the summary bit.

If we then fall back to the 'slow path' and the code in
adapter_indicators_set(), we could possibly see the summary bit was
already set (by adapter_indicators_set_fast() above) and assume that we
don't need to inject anything because the thread of executing that set
the summary bit ALSO did the inject.  But in this particular case we
actually never did.

Do we need to unset the summary bit for these subsequent -EWOULDBLOCK
cases so that the subsequent fallback path will know it must inject?
But we should only undo this if adapter_indicators_set_fast() actually
set the summary bit in the first place.
The indicator bits themselves that might have been set by
adapter_indicators_set_fast() should be fine to leave on, injection
decision is based on the setting of the summary indicator.

> +	if (ret < 0)
> +		return -EWOULDBLOCK;
> +	if (!ret || adapter->masked) {
> +		kvm->stat.io_390_inatomic_adapter_masked++;
> +		return 0;
> +	}
> +
> +	inti = kzalloc_obj(*inti, GFP_ATOMIC);
> +	if (!inti)
> +		return -EWOULDBLOCK;
> +
> +	if (!test_kvm_facility(kvm, 72) || !adapter->suppressible) {
> +		ret = kvm_s390_inject_vm(kvm, &s390int, inti);
> +			if (ret == 0)
> +				return ret;
> +			else
> +				return -EWOULDBLOCK;
> +		}
> +
> +	spin_lock(&fi->ais_lock);
> +	if (fi->nimm & AIS_MODE_MASK(adapter->isc)) {
> +		trace_kvm_s390_airq_suppressed(adapter->id, adapter->isc);
> +		kfree(inti);
> +		goto out;
> +	}
> +
> +	ret = kvm_s390_inject_vm(kvm, &s390int, inti);
> +	if (!ret && (fi->simm & AIS_MODE_MASK(adapter->isc))) {
> +		fi->nimm |= AIS_MODE_MASK(adapter->isc);
> +		trace_kvm_s390_modify_ais_mode(adapter->isc,
> +					       KVM_S390_AIS_MODE_SINGLE, 2);
> +	}
> +	goto out;

Goto is unncessary here.

Also, you don't check returns from kvm_s390_inject_vm on this path?  If
any errors we still return 0 to caller (compare this to the call above
where any errors would instead yield -EWOULDBLOCK)  Can you please
double check this?

[...]

>  
> -static void __kvm_inject_pfault_token(struct kvm_vcpu *vcpu, bool start_token,
> -				      unsigned long token)
> +static int __kvm_inject_pfault_token(struct kvm_vcpu *vcpu, bool start_token,
> +				     unsigned long token)

You updated this to return a value but AFAICT you never update the
callers of this function to actually handle the return value.