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

Matthew Rosato
2026-04-29T14:44:58Z

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