Re: [PATCH v2 1/6] KVM: s390: Fix dirty marking in adapter_indicators_set*()

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

On 8/14/26 12:33 PM, Claudio Imbrenda wrote:
> When the indicator and/or summary bits are set in the guest, the
> accessed page was only marked dirty in KVM if the access was performed
> using the slow path; accesses through the new kvm_arch_set_irq_inatomic
> fast inject path would not mark the page as dirty.
> 
> Fix by adding/moving the missing calls to mark_page_dirty(). Note that
> for the inatomic path set_page_dirty{,_lock}() is not needed as the
> page stays pinned; the unpin path correctly marks it as dirty.
> 
> Opportunistically reorder the local variables to be in reverse
> Christmas tree order and refactor to use guard().
> 
> Fixes: 1e95e3bc6b05 ("KVM: s390: Enable adapter_indicators_set to use mapped pages")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

The code itself looks good to me:

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

Doug, can you please test (w/ lockdep enabled)?

Thanks,
Matt

> ---
>  arch/s390/kvm/interrupt.c | 40 ++++++++++++++++++++-------------------
>  1 file changed, 21 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index da740a378a8c..fc4d1f8193d9 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -2984,12 +2984,14 @@ 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 s390_map_info *ind_info, *summary_info;
> -	void *map;
>  	struct page *ind_page, *summary_page;
>  	unsigned long flags;
> +	unsigned long bit;
> +	int summary_set;
> +	void *map;
> +
> +	guard(srcu)(&kvm->srcu);
>  
>  	ind_page = NULL;
>  
> @@ -3000,14 +3002,11 @@ static int adapter_indicators_set(struct kvm *kvm,
>  		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);
>  	} else {
>  		map = page_address(ind_info->page);
> @@ -3015,6 +3014,7 @@ static int adapter_indicators_set(struct kvm *kvm,
>  		set_bit(bit, map);
>  		spin_unlock_irqrestore(&adapter->maps_lock, flags);
>  	}
> +	mark_page_dirty(kvm, gpa_to_gfn(adapter_int->ind_gaddr));
>  
>  	spin_lock_irqsave(&adapter->maps_lock, flags);
>  	summary_info = get_map_info(adapter, adapter_int->summary_addr);
> @@ -3023,14 +3023,11 @@ static int adapter_indicators_set(struct kvm *kvm,
>  		summary_page = pin_map_page(kvm, adapter_int->summary_addr, 0);
>  		if (!summary_page)
>  			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);
>  		unpin_user_page(summary_page);
>  	} else {
>  		map = page_address(summary_info->page);
> @@ -3039,6 +3036,7 @@ static int adapter_indicators_set(struct kvm *kvm,
>  		summary_set = test_and_set_bit(bit, map);
>  		spin_unlock_irqrestore(&adapter->maps_lock, flags);
>  	}
> +	mark_page_dirty(kvm, gpa_to_gfn(adapter_int->summary_gaddr));
>  
>  	return summary_set ? 0 : 1;
>  }
> @@ -3048,26 +3046,29 @@ static int adapter_indicators_set_fast(struct kvm *kvm,
>  				       struct kvm_s390_adapter_int *adapter_int,
>  				       int setbit)
>  {
> +	struct s390_map_info *ind_info, *summary_info;
>  	unsigned long bit;
>  	int summary_set;
> -	struct s390_map_info *ind_info, *summary_info;
>  	void *map;
>  
> -	spin_lock(&adapter->maps_lock);
> +	guard(srcu)(&kvm->srcu);
> +	guard(spinlock)(&adapter->maps_lock);
> +
>  	ind_info = get_map_info(adapter, adapter_int->ind_addr);
> -	if (!ind_info) {
> -		spin_unlock(&adapter->maps_lock);
> +	if (!ind_info)
>  		return -EWOULDBLOCK;
> -	}
> +
>  	map = page_address(ind_info->page);
>  	bit = get_ind_bit(ind_info->addr, adapter_int->ind_offset, adapter->swap);
> -	if (setbit)
> +	if (setbit) {
>  		set_bit(bit, map);
> +		mark_page_dirty(kvm, gpa_to_gfn(adapter_int->ind_gaddr));
> +	}
> +
>  	summary_info = get_map_info(adapter, adapter_int->summary_addr);
> -	if (!summary_info) {
> -		spin_unlock(&adapter->maps_lock);
> +	if (!summary_info)
>  		return -EWOULDBLOCK;
> -	}
> +
>  	map = page_address(summary_info->page);
>  	bit = get_ind_bit(summary_info->addr, adapter_int->summary_offset,
>  			  adapter->swap);
> @@ -3077,7 +3078,8 @@ static int adapter_indicators_set_fast(struct kvm *kvm,
>  		summary_set = test_and_set_bit(bit, map);
>  	else
>  		summary_set = test_and_clear_bit(bit, map);
> -	spin_unlock(&adapter->maps_lock);
> +	mark_page_dirty(kvm, gpa_to_gfn(adapter_int->summary_gaddr));
> +
>  	return summary_set ? 0 : 1;
>  }
>