Re: [PATCH v6 2/4] KVM: s390: Enable adapter_indicators_set to use mapped pages

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

On 5/11/26 12:14 PM, Douglas Freimuth wrote:
> The S390 adapter_indicators_set function needs to be able to use mapped

Nit: s/S390/s390/

> 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.

Slight tweak to the commit message:  adapter_indicators_set doesn't
really _need_ to use the long-term mapped pages.  It's an optimization
to do so, while your will later add the _fast() variant where it is
a requirement.

Maybe re-word like:
'The S390 adapter_indicators_set function can now be optimized to use
long-term mapped pages when available so that work can be
processed...'

[...]

>  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) {
> -		unpin_user_page(ind_page);
> -		return -1;
> +	raw_spin_lock_irqsave(&adapter->maps_lock, flags);
> +	ind_info = get_map_info(adapter, adapter_int->ind_addr);
> +	if (!ind_info) {
> +		raw_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);

I think you can simplify some of this unpin logic by just calling 
unpin_user_page(ind_page) here?  You don't use it past this point
and you aren't holding a raw spinlock ...

> +	} else {
> +		map = page_address(ind_info->page);
> +		bit = get_ind_bit(ind_info->addr, adapter_int->ind_offset, adapter->swap);
> +		set_bit(bit, map);
> +		raw_spin_unlock_irqrestore(&adapter->maps_lock, flags);
> +	}
> +	raw_spin_lock_irqsave(&adapter->maps_lock, flags);
> +	summary_info = get_map_info(adapter, adapter_int->summary_addr);
> +	if (!summary_info) {
> +		raw_spin_unlock_irqrestore(&adapter->maps_lock, flags);
> +		summary_page = pin_map_page(kvm, adapter_int->summary_addr, 0);
> +		if (!summary_page) {
> +			if (!ind_info) {
> +				WARN_ON_ONCE(!ind_page);
> +				unpin_user_page(ind_page);
> +			}

... Then you wouldn't need this !ind_info logic, it's safe to return
immediately

I also don't think that WARN_ON_ONCE does what was intended.  Looking
back any my v3 comments, the idea was to catch a case where we map
indicator pages and set the associated bits but cannot map summary
page.  That means if you want a WARN here, then something like...

if (!summary_page) {
  WARN_ON_ONCE(ind_page || ind_info);
  return -1;
}

Also ind_page needs to be initialized to NULL for that to work.


> +			return -1;
> +		}
> +		idx = srcu_read_lock(&kvm->srcu);
> +		map = page_address(summary_page);
> +		bit = get_ind_bit(adapter_int->summary_addr,
> +				  adapter_int->summary_offset, adapter->swap);
> +		summary_set = test_and_set_bit(bit, map);
> +		mark_page_dirty(kvm, adapter_int->summary_gaddr >> PAGE_SHIFT);
> +		set_page_dirty_lock(summary_page);
> +		srcu_read_unlock(&kvm->srcu, idx);

Same here, unpin_user_page(summary_page) ...

> +	} else {
> +		map = page_address(summary_info->page);
> +		bit = get_ind_bit(summary_info->addr, adapter_int->summary_offset,
> +				  adapter->swap);
> +		summary_set = test_and_set_bit(bit, map);
> +		raw_spin_unlock_irqrestore(&adapter->maps_lock, flags);
>  	}
>  
> -	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);
> -	map = page_address(summary_page);
> -	bit = get_ind_bit(adapter_int->summary_addr,
> -			  adapter_int->summary_offset, adapter->swap);
> -	summary_set = test_and_set_bit(bit, map);
> -	mark_page_dirty(kvm, adapter_int->summary_gaddr >> PAGE_SHIFT);
> -	set_page_dirty_lock(summary_page);
> -	srcu_read_unlock(&kvm->srcu, idx);
> -
> -	unpin_user_page(ind_page);
> -	unpin_user_page(summary_page);
> +	if (!ind_info)
> +		unpin_user_page(ind_page);
> +	if (!summary_info)
> +		unpin_user_page(summary_page);

... And then you don't need either of these checks/unpins?

>  	return summary_set ? 0 : 1;
>  }
>