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

[Douglas Freimuth <freimuth@linux.ibm.com>]

On 8/18/26 4:43 PM, Matthew Rosato wrote:
> 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
> 

I did some testing (not exhaustive) with blkpvt and stressed with 
stress-ng to force paging and I didn't observe any lockdep messages.

>> ---
>>   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;
>>   }
>>   
>