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

Douglas Freimuth
2026-05-04T13:21:50Z

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.