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

[Douglas Freimuth <freimuth@linux.ibm.com>]

On 4/29/26 12:11 PM, Matthew Rosato wrote:
> 
>> +static int adapter_indicators_set_fast(struct kvm *kvm,
>> +				       struct s390_io_adapter *adapter,
>> +				       struct kvm_s390_adapter_int *adapter_int,
>> +				       int setbit)
>> +{
>> +	unsigned long bit;
>> +	int summary_set;
>> +	struct s390_map_info *ind_info, *summary_info;
>> +	void *map;
>> +
>> +	raw_spin_lock(&adapter->maps_lock);
>> +	ind_info = get_map_info(adapter, adapter_int->ind_addr);
>> +	if (!ind_info) {
>> +		raw_spin_unlock(&adapter->maps_lock);
>> +		return -EWOULDBLOCK;
>> +	}
>> +	map = page_address(ind_info->page);
>> +	bit = get_ind_bit(ind_info->addr, adapter_int->ind_offset, adapter->swap);
>> +	if (setbit)
>> +		set_bit(bit, map);
>> +	else
>> +		clear_bit(bit, map);
> 
> Hmm, I don't know about this.  In my comment on v2 I was only concerned
> about undoing the setting of the summary indicator as that will be used
> on the slow path to decide whether or not we need to inject an interrupt
> in addition to setting the indicator bits.
> 
> I think we should drop the else clear_bit() here.  If _fast already set
> it and we are now backing out to the slow path, then it will stay on all
> the way through the slow path and that should be OK.
> 
>> +	summary_info = get_map_info(adapter, adapter_int->summary_addr);
>> +	if (!summary_info) {
>> +		raw_spin_unlock(&adapter->maps_lock);
>> +		return -EWOULDBLOCK;
>> +	}
>> +	map = page_address(summary_info->page);
>> +	bit = get_ind_bit(summary_info->addr, adapter_int->summary_offset,
>> +			  adapter->swap);
>> +	if (setbit)
>> +		summary_set = test_and_set_bit(bit, map);
>> +	else
>> +		summary_set = test_and_clear_bit(bit, map);
> 
> I had to go back and refresh myself about WHY we needed to 'undo' our
> prior setting of the summary bit specifically.
> 
> The reason is that, if we need to fall back to the slow path, that code
> will see the summary bit already on and therefore not inject an
> interrupt believing that the thread that initially set the summary bit
> did that.  But, if we fell back from the _fast path via -EWOULDBLOCK
> after setting the summary indicator, no interrupt was ever injected at
> that time.
> 
> So my point: this _really_ deserves some comment blocks describing the
> purpose of setbit + a specific statement that it's OK to clear this
> summary bit now when setbit=0 but then the caller must re-drive this
> summary indication again via adapter_indicators_set().
> 
> [...]
> 
>> +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, setbit;
>> +	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);
>> +	setbit = 1;
>> +	ret = adapter_indicators_set_fast(kvm, adapter, &e->adapter, setbit);
>> +	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)
> 
> You need to undo the summary bit indication on this path as well.
> 
> [...]
> 
>> -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)
>>   {
>>   	struct kvm_s390_interrupt inti;
>>   	struct kvm_s390_irq irq;
>> +	struct kvm_s390_interrupt_info *inti_mem = NULL;
>>   
>>   	if (start_token) {
>>   		irq.u.ext.ext_params2 = token;
>>   		irq.type = KVM_S390_INT_PFAULT_INIT;
>>   		WARN_ON_ONCE(kvm_s390_inject_vcpu(vcpu, &irq));
>>   	} else {
>> +		inti_mem = kzalloc_obj(*inti_mem, GFP_KERNEL_ACCOUNT);
>> +		if (!inti_mem)
>> +			return -ENOMEM;
> 
> To match the other nonzero cases here, rather than making
> __kvm_inject_pfault_token() return a value can you leave it a void
> return and then just do something like:
> 
> if (WARN_ON_ONCE(!inti_mem))
> 	return;
> 
> 

Thanks. This function was a bit of sideshow and kind of an after thought 
but that said I was determined to give the caller a return code. It 
really wanted a WARN_ON_ONCE. That works better here.