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

[Matthew Rosato]

On 3/25/26 9:42 PM, 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@xxxxxxxxxxxxx>
> ---

[...]

>  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;
>  
> -	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;
> +	ind_info = get_map_info(adapter, adapter_int->ind_addr);
> +	if (!ind_info) {
> +		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);

Sashiko's comments about this area of code should already be addressed
by my comments in v1 wrt to marking the page dirty before unpinning.

> +		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);
> +	}
> +	summary_info = get_map_info(adapter, adapter_int->summary_addr);
> +	if (!summary_info) {
> +		summary_page = get_map_page(kvm, adapter_int->summary_addr);
> +		if (!summary_page) {
> +			put_page(ind_page);

Sashiko has a point here.  It is possible that you may have taken the
else path above where ind_info != NULL and never used ind_page.

Initialize ind_page to a NULL value first and check for a NULL value
here before attempting a put_page.
Or I think you can also check (!ind_info) and only put the page in this
case like you do later in this function.

[...]

>  
> @@ -2890,6 +2924,7 @@ static int set_adapter_int(struct kvm_kernel_irq_routing_entry *e,
>  {
>  	int ret;
>  	struct s390_io_adapter *adapter;
> +	unsigned long flags;
>  
>  	/* We're only interested in the 0->1 transition. */
>  	if (!level)
> @@ -2897,7 +2932,9 @@ static int set_adapter_int(struct kvm_kernel_irq_routing_entry *e,
>  	adapter = get_io_adapter(kvm, e->adapter.adapter_id);
>  	if (!adapter)
>  		return -1;
> +	spin_lock_irqsave(&adapter->maps_lock, flags);
>  	ret = adapter_indicators_set(kvm, adapter, &e->adapter);
> +	spin_unlock_irqrestore(&adapter->maps_lock, flags);

Another point for Sashiko -- This is almost certainly a fallout from my
recommendation to drop the semaphore from patch 1 and go straight to a
spinlock vs adding semaphore in patch 1 and converting in patch 3.

You can't put a spinlock here as you may lose control inside
adapter_indicators_set.  You resolve the incompatibility in the next
patch by moving the spinlock inside the call and dropping it before you
could lose control -- but you have to already account for that in this
patch.

>  	if ((ret > 0) && !adapter->masked) {
>  		ret = kvm_s390_inject_airq(kvm, adapter);
>  		if (ret == 0)