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

[Matthew Rosato]

On 5/20/26 12:04 PM, Douglas Freimuth wrote:
> The s390 adapter_indicators_set function can now be optimized to use
> long-term mapped pages when available so that work can be
> processed, on a fast path when interrupts are disabled.

nit: drop the comma

> 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@xxxxxxxxxxxxx>
> ---
>  arch/s390/kvm/interrupt.c | 89 ++++++++++++++++++++++++++++-----------
>  1 file changed, 65 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 654c37e5e898..c41461b6f365 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -2885,41 +2885,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->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 = pin_map_page(kvm, adapter_int->ind_addr, 0);
> -	if (!ind_page)
> -		return -1;
> -	summary_page = pin_map_page(kvm, adapter_int->summary_addr, 0);
> -	if (!summary_page) {
> +	ind_page = NULL;
> +
> +	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 = pin_map_page(kvm, adapter_int->ind_addr, 0);
> +		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);
>  		unpin_user_page(ind_page);
> -		return -1;
> +	} 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 = pin_map_page(kvm, adapter_int->summary_addr, 0);
> +		if (!summary_page) {
> +			WARN_ON_ONCE(ind_page || ind_info);

I think this was my suggestion on v6, but this is basically WARN_ON_ONCE(true).
We can't reach this code unless we got the indicator page somehow. 

Really then it should be:

if (WARN_ON_ONCE(!summary_page))
    return -1;

And we catch the first instance of setting indicator bits but failing to set
the summary bit.

With that:

Reviewed-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxx>