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

[Christian Borntraeger]

Am 14.08.26 um 18:33 schrieb Claudio Imbrenda:
> 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@xxxxxxxxxxxxx>

Doug, Matt, can you have a look?



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