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

[Douglas Freimuth]

On 4/29/26 10:44 AM, Matthew Rosato wrote:
> > +static struct page *get_map_page(struct kvm *kvm, u64 uaddr)
> > +{
> > +	struct mm_struct *mm = kvm->mm;
> > +	struct page *page = NULL;
> > +	int locked = 1;
> > +
> > +	if (mmget_not_zero(mm)) {
> > +		mmap_read_lock(mm);
> > +		get_user_pages_remote(mm, uaddr, 1, FOLL_WRITE,
> > +				      &page, &locked);
>
> I have wondered this before, and Sashiko mentions it now:  Would it make
> sense to also FOLL_LONGTERM here?
>
> I recognize that the old ioctl code that you are resurrecting here did
> not use FOLL_LONGTERM, but I can't think of a reason why.
>
> The mapping may indeed be held long-term (life of the guest or at least
> the associated adapter in the guest), and it's effectively under
> userspace control, waiting for a corresponding unmap ioctl or for the
> guest to go away or enter pv mode.
>
> Can you please test?

I tested this with get_user_pages_remote() and
FOLL_WRITE | FOLL_LONGTERM. I get null pages back. Thus the exploration 
into whether pin_user_pages_remote() with those flags is desirous in 
this case.

> > +		if (locked)
> > +			mmap_read_unlock(mm);
> > +		mmput(mm);
> > +	}
> > +
> > +	return page;
> > +}
> > +
> > +static int kvm_s390_adapter_map(struct kvm *kvm, unsigned int id, __u64 addr)
> > +{
> > +	struct s390_io_adapter *adapter = get_io_adapter(kvm, id);
> > +	struct s390_map_info *map;
> > +	unsigned long flags;
> > +	__u64 host_addr;
> > +	int ret, idx;
> > +
> > +	if (!adapter || !addr)
> > +		return -EINVAL;
> > +
> > +	map = kzalloc_obj(*map, GFP_KERNEL_ACCOUNT);
> > +	if (!map)
> > +		return -ENOMEM;
> > +
> > +	INIT_LIST_HEAD(&map->list);
> > +	idx = srcu_read_lock(&kvm->srcu);
> > +	host_addr = gpa_to_hva(kvm, addr);
> > +	if (kvm_is_error_hva(host_addr)) {
> > +		srcu_read_unlock(&kvm->srcu, idx);
> > +		kfree(map);
>
> Drop this kfree(), you already do this when you goto out
>
> > +		ret = -EFAULT;
> > +		goto out;
> > +	}
> > +	srcu_read_unlock(&kvm->srcu, idx);
> > +	map->guest_addr = addr;
> > +	map->addr = host_addr;
> > +	map->page = get_map_page(kvm, host_addr);
> > +	if (!map->page) {
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +	raw_spin_lock_irqsave(&adapter->maps_lock, flags);
> > +	if (adapter->nr_maps < MAX_S390_ADAPTER_MAPS) {
> > +		list_add_tail(&map->list, &adapter->maps);
> > +		adapter->nr_maps++;
> > +		ret = 0;
> > +	} else {
> > +		put_page(map->page);
> > +		ret = -EINVAL;
> > +	}
> > +	raw_spin_unlock_irqrestore(&adapter->maps_lock, flags);
>
> Sashiko is concerned about put_page() potentially sleeping under
> PREEMPT_RT; drilling down to functions like free_one_page() indeed I see
> regular spinlocks employed.
>
> RT aside, it might be worth doing this anyway to reduce the critical
> section you are holding this lock over, like so:
>
> 	raw_spin_lock_irqsave(&adapter->maps_lock, flags);
> 	if (adapter->nr_maps < MAX_S390_ADAPTER_MAPS) {
> 		list_add_tail(&map->list, &adapter->maps);
> 		adapter->nr_maps++;
> 		ret = 0;
> 	} else {
> 		ret = -EINVAL;
> 	}
> 	raw_spin_unlock_irqrestore(&adapter->maps_lock, flags);
> 	if (ret)
> 		put_page(map->page);
>
>
> > +out:
> > +	if (ret)
> > +		kfree(map);
> > +	return ret;
> > +}
> > +
> > +static int kvm_s390_adapter_unmap(struct kvm *kvm, unsigned int id, __u64 addr)
> > +{
> > +	struct s390_io_adapter *adapter = get_io_adapter(kvm, id);
> > +	struct s390_map_info *map, *tmp;
> > +	struct page *map_page_to_put = NULL;
> > +	u64 map_addr_to_mark = 0;
> > +	unsigned long flags;
> > +	int found = 0, idx;
> > +
> > +	if (!adapter || !addr)
> > +		return -EINVAL;
> > +
> > +	raw_spin_lock_irqsave(&adapter->maps_lock, flags);
> > +	list_for_each_entry_safe(map, tmp, &adapter->maps, list) {
> > +		if (map->guest_addr == addr) {
> > +			found = 1;
> > +			adapter->nr_maps--;
> > +			list_del(&map->list);
> > +			map_page_to_put = map->page;
> > +			map_addr_to_mark = map->guest_addr;
> > +			kfree(map);
>
> Move the kfree() outside of the raw spinlock and instead call it...
>
> > +			break;
> > +		}
> > +	}
> > +	raw_spin_unlock_irqrestore(&adapter->maps_lock, flags);
> > +
> > +	if (found) {
>
> ... right here.
>
> > +		idx = srcu_read_lock(&kvm->srcu);
> > +		mark_page_dirty(kvm, map_addr_to_mark >> PAGE_SHIFT);
> > +		set_page_dirty_lock(map_page_to_put);
> > +		srcu_read_unlock(&kvm->srcu, idx);
> > +		put_page(map_page_to_put);
> > +	}
> > +
> > +	return found ? 0 : -ENOENT;
> > +}
> > +
> >   void kvm_s390_destroy_adapters(struct kvm *kvm)
> >   {
> >   	int i;
> > +	struct s390_map_info *map, *tmp;
> > +	unsigned long flags;
> >   
> > -	for (i = 0; i < MAX_S390_IO_ADAPTERS; i++)
> > +	for (i = 0; i < MAX_S390_IO_ADAPTERS; i++) {
> > +		if (!kvm->arch.adapters[i])
> > +			continue;
> > +		raw_spin_lock_irqsave(&kvm->arch.adapters[i]->maps_lock, flags);
> > +		list_for_each_entry_safe(map, tmp,
> > +					 &kvm->arch.adapters[i]->maps, list) {
> > +			list_del(&map->list);
> > +			put_page(map->page);
> > +			kfree(map);
> > +		}
> > +		raw_spin_unlock_irqrestore(&kvm->arch.adapters[i]->maps_lock, flags);
>
> Moving put_page/kfree out of the spinlock is a bit more work here.
> Handle this the same way you did in kvm_s390_unmap_all_adapters_pv()?
>
> Actually wait -- besides the dirty page logic (which should be fine to
> do here too) this is the same code as kvm_s390_unmap_all_adapters_pv().
> Can you make the code in kvm_s390_unmap_all_adapters_pv() a single
> routine with a different name (e.g. kvm_s390_unmap_all_adapters()?) that
> is called both from here as well as from kvm_s390_handle_pv()?