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

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

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()?
> 
>