Re: [PATCH v2 RFC] KVM: s390/interrupt: do not pin adapter interrupt pages

[David Hildenbrand <david@redhat.com>]

> +	/*
> +	 * We resolve the gpa to hva when setting the IRQ routing. If userspace
> +	 * decides to mess with the memslots it better also updates the irq
> +	 * routing. Otherwise we will write to the wrong userspace address.
> +	 */

I guess this is just as old handling, where a page was pinned. But
slightly better :) So the pages are definitely part of guest memory.

Fun stuff: If (a nasty) guest (in current code) zappes this page using
balloon inflation and the page is re-accessed (e.g., by the guest or by
the host), a new page will be faulted in, and there will be an
inconsistency between what the guest/user space sees and what this code
sees. Going via the user space address looks cleaner.

Now, with postcopy live migration, we will also zap all guest memory
before starting the guest, I do wonder if that produces a similar
inconsistency ... usually, when pages are pinned in the kernel, we
inhibit the balloon and implicitly also postcopy.

If so, this actually fixes an issue. But might depend on the order
things are initialized in user space. Or I am messing up things :)

[...]

>  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;
> -	int found = 0;
> -
> -	if (!adapter || !addr)
> -		return -EINVAL;
> -
> -	down_write(&adapter->maps_lock);
> -	list_for_each_entry_safe(map, tmp, &adapter->maps, list) {
> -		if (map->guest_addr == addr) {
> -			found = 1;
> -			atomic_dec(&adapter->nr_maps);
> -			list_del(&map->list);
> -			put_page(map->page);
> -			kfree(map);
> -			break;
> -		}
> -	}
> -	up_write(&adapter->maps_lock);
> -
> -	return found ? 0 : -EINVAL;
> +	return 0;

Can we get rid of this function?

>  }

> +static struct page *get_map_page(struct kvm *kvm,
> +				 struct s390_io_adapter *adapter,
> +				 u64 uaddr)
>  {
> -	struct s390_map_info *map;
> +	struct page *page;
> +	int ret;
>  
>  	if (!adapter)
>  		return NULL;
> -
> -	list_for_each_entry(map, &adapter->maps, list) {
> -		if (map->guest_addr == addr)
> -			return map;
> -	}
> -	return NULL;
> +	page = NULL;

struct page *page = NULL;

> +	if (!uaddr)
> +		return NULL;
> +	down_read(&kvm->mm->mmap_sem);
> +	ret = get_user_pages_remote(NULL, kvm->mm, uaddr, 1, FOLL_WRITE,
> +				    &page, NULL, NULL);
> +	if (ret < 1)
> +		page = NULL;

Is that really necessary? According to the doc, pinned pages are stored
to the array.  ret < 1 means "no pages" were pinned, so nothing should
be stored.

-- 
Thanks,

David / dhildenb