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

Douglas Freimuth
2026-04-30T21:05:46Z

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