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

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

> +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?

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