Re: [PATCH v6 1/4] KVM: s390: Add map/unmap ioctl and clean mappings post-guest

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

On 5/11/26 12:14 PM, Douglas Freimuth wrote:
> S390 needs map/unmap ioctls, which map the adapter set
> indicator pages, so the pages can be accessed when interrupts are
> disabled. The mappings are cleaned up when the guest is removed.
> 
> Map/Unmap ioctls are fenced in order to avoid the longterm pinning
> in Secure Execution environments. In Secure Execution
> environments the path of execution available before this patch is followed.
> 
> Statistical counters to count map/unmap functions for adapter indicator
> pages are added. The counters can be used to analyze
> map/unmap functions in non-Secure Execution environments and similarly
> can be used to analyze Secure Execution environments where the counters
> will not be incremented as the adapter indicator pages are not mapped.

I think it's worth mentioning that this patch also converts to
pin_user_pages_remote for both the ioctl as well as the pin-on-demand
logic in adapter_indicators_set()

[...]

> +
> +void kvm_s390_unmap_all_adapters(struct kvm *kvm)
> +{
> +	struct s390_map_info *map, *tmp;
> +	unsigned long flags;
> +	int i, idx;

Sashiko makes a valid point here:
the ioctl is doing unmaps with the kvm->lock held.
the kvm_s390_unmap_all_adapters_pv() path is also.
But the registration of adapters is not, nor is the direct call to 
kvm_s390_unmap_all_adapter() from kvm_s390_destroy_adapters(), the
latter of which is not too big of a deal since vcpus are already gone.

The goal here is to protect the contents of kvm->arch.adapters[] from
changing while you're running the list here.  

Can you look at acquiring the kvm->lock over register_io_adapter() as
part of this patch?  AFAICT it should be fine to do so.  There are
other flic operations like kvm_s390_clear_float_irqs() which do so.

> +
> +	for (i = 0; i < MAX_S390_IO_ADAPTERS; i++) {
> +		struct s390_io_adapter *adapter = kvm->arch.adapters[i];
> +		LIST_HEAD(local_list);
> +
> +		if (!adapter)
> +			continue;
> +
> +		raw_spin_lock_irqsave(&adapter->maps_lock, flags);
> +		list_splice_init(&adapter->maps, &local_list);
> +		adapter->nr_maps = 0;
> +		raw_spin_unlock_irqrestore(&adapter->maps_lock, flags);
> +
> +		list_for_each_entry_safe(map, tmp, &local_list, list) {
> +			list_del(&map->list);
> +			idx = srcu_read_lock(&kvm->srcu);
> +			mark_page_dirty(kvm, map->guest_addr >> PAGE_SHIFT);
> +			set_page_dirty_lock(map->page);
> +			srcu_read_unlock(&kvm->srcu, idx);
> +			unpin_user_page(map->page);
> +			kfree(map);
> +		}
> +	}
> +}

[...]

> +static void kvm_s390_unmap_all_adapters_pv(struct kvm *kvm)
> +{
> +	kvm_s390_unmap_all_adapters(kvm);
> +}

Just get rid of kvm_s390_unmap_all_adapters_pv() and call
kvm_s390_unmap_all_adapters() directly in kvm_s390_handle_pv()?

Otherwise this patch looks good to me.

Thanks,
Matt