Re: [PATCH v3 2/3] KVM: s390: Enable adapter_indicators_set to use mapped pages

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

On 4/6/26 2:44 AM, Douglas Freimuth wrote:
> The S390 adapter_indicators_set function needs to be able to use mapped 
> pages so that worked can be processed,on a fast path when interrupts are
> disabled. If adapter indicator pages are not mapped then local mapping is 
> done on a slow path as it is prior to this patch. For example, Secure
> Execution environments will take the local mapping path as it does prior to
> this patch.
> 
> Signed-off-by: Douglas Freimuth <freimuth@linux.ibm.com>
> ---
>  arch/s390/kvm/interrupt.c | 91 ++++++++++++++++++++++++++++-----------
>  1 file changed, 66 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 47bd6361c849..f3183c9ec7f1 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -2847,41 +2847,82 @@ static unsigned long get_ind_bit(__u64 addr, unsigned long bit_nr, bool swap)
>  	return swap ? (bit ^ (BITS_PER_LONG - 1)) : bit;
>  }
>  
> +static struct s390_map_info *get_map_info(struct s390_io_adapter *adapter,
> +					  u64 addr)
> +{
> +	struct s390_map_info *map;
> +
> +	if (!adapter)
> +		return NULL;
> +
> +	list_for_each_entry(map, &adapter->maps, list) {
> +		if (map->guest_addr == addr)
> +			return map;
> +	}
> +	return NULL;
> +}
> +
>  static int adapter_indicators_set(struct kvm *kvm,
>  				  struct s390_io_adapter *adapter,
>  				  struct kvm_s390_adapter_int *adapter_int)
>  {
>  	unsigned long bit;
>  	int summary_set, idx;
> -	struct page *ind_page, *summary_page;
> +	struct s390_map_info *ind_info, *summary_info;
>  	void *map;
> +	struct page *ind_page, *summary_page;
> +	unsigned long flags;
>  
> -	ind_page = get_map_page(kvm, adapter_int->ind_addr);
> -	if (!ind_page)
> -		return -1;
> -	summary_page = get_map_page(kvm, adapter_int->summary_addr);
> -	if (!summary_page) {
> -		put_page(ind_page);
> -		return -1;
> +	spin_lock_irqsave(&adapter->maps_lock, flags);
> +	ind_info = get_map_info(adapter, adapter_int->ind_addr);
> +	if (!ind_info) {
> +		spin_unlock_irqrestore(&adapter->maps_lock, flags);
> +		ind_page = get_map_page(kvm, adapter_int->ind_addr);
> +		if (!ind_page)
> +			return -1;
> +		idx = srcu_read_lock(&kvm->srcu);
> +		map = page_address(ind_page);
> +		bit = get_ind_bit(adapter_int->ind_addr,
> +				  adapter_int->ind_offset, adapter->swap);
> +		set_bit(bit, map);
> +		mark_page_dirty(kvm, adapter_int->ind_gaddr >> PAGE_SHIFT);
> +		set_page_dirty_lock(ind_page);
> +		srcu_read_unlock(&kvm->srcu, idx);
> +	} else {
> +		map = page_address(ind_info->page);
> +		bit = get_ind_bit(ind_info->addr, adapter_int->ind_offset, adapter->swap);
> +		set_bit(bit, map);
> +		spin_unlock_irqrestore(&adapter->maps_lock, flags);
> +	}
> +	spin_lock_irqsave(&adapter->maps_lock, flags);
> +	summary_info = get_map_info(adapter, adapter_int->summary_addr);
> +	if (!summary_info) {
> +		spin_unlock_irqrestore(&adapter->maps_lock, flags);
> +		summary_page = get_map_page(kvm, adapter_int->summary_addr);
> +		if (!summary_page && !ind_info) {
> +			put_page(ind_page);
> +			return -1;

1) Sashiko mentions that this now allows for a path where we already set
the indicator bits above but bail out early because we couldn't find the
summary page.  The old code validated that it could find the indicator
page AND the summary page before setting any bits.

I think re-implementing to achieve that model is _preferable_ but I
think we are also OK with the current approach; in this unlikely event
the indicator bits are set but we fail to set the summary bit, then I
can envision at least 2 scenarios...

1a) an already-running interrupt handler in the guest happens to see the
indicator bits on because the summary bit was already set and proceeds
to handle it and clear the indicator bits.  I suppose we might get an
over-indication of those bits later if the host attempts a re-delivery.

1b) Same as above but the summary bit was off (or the interrupt handler
never was given initiative to run in the first place) so the indicator
bits are not noticed and nothing is handled.  But if re-delivery is
attempted then this code would re-indicate the same bits which were
already on -- this would not prevent an attempt at indicating the
summary bit again.  Assuming that succeeds, it will result in an adapter
interrupt delivered.

If we continually fail to map the summary page I guess it could be an
awkward dance of indicating the bits but never being able to set the
summary bit.  Realistically, if we are in this situation (cannot map the
summary page that was previously valid) it seems to me we've got a
bigger issue.  Would a WARN_ON_ONCE make sense?


2) If you keep this approach then there is another issue here that is
definitely a valid concern -- if summary_page is NULL but info_info is
non-NULL, you continue on and use a summary_page of 0 below which is
wrong - I think you wanted to return -1 (but without a put_page) in this
case e.g.:

if (!summary_page) {
	if (!ind_page)
		put_page(ind_page);
	return -1;
}