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

[Matthew Rosato <mjrosato@linux.ibm.com>]

> +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;