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

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

On 6/1/26 9:14 AM, Janosch Frank wrote:
> On 5/31/26 21:03, Douglas Freimuth wrote:
>> s390 needs map/unmap ioctls, which map the adapter set
>> indicator pages, so the pages can be accessed when interrupts are
>> disabled. The mappings are cleaned up when the guest is removed.
>> pin_user_pages_remote is used for both the ioctl as well
>> as the pin-on-demand logic in adapter_indicators_set().
>>
>> Map/Unmap ioctls are fenced in order to avoid the longterm pinning
>> in Secure Execution environments. In Secure Execution
>> environments the path of execution available before this patch is 
>> followed.
>>
>> Statistical counters to count map/unmap functions for adapter indicator
>> pages are added. The counters can be used to analyze
>> map/unmap functions in non-Secure Execution environments and similarly
>> can be used to analyze Secure Execution environments where the counters
>> will not be incremented as the adapter indicator pages are not mapped.
>>
>> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
>> Signed-off-by: Douglas Freimuth <freimuth@linux.ibm.com>
>> ---
>>   arch/s390/include/asm/kvm_host.h |   5 +
>>   arch/s390/kvm/interrupt.c        | 226 +++++++++++++++++++++++++------
>>   arch/s390/kvm/kvm-s390.c         |   3 +
>>   arch/s390/kvm/kvm-s390.h         |   2 +
>>   4 files changed, 195 insertions(+), 41 deletions(-)
>>
>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/ 
>> kvm_host.h
>> index 8a4f4a39f7a2..0056cc9414a0 100644
>> --- a/arch/s390/include/asm/kvm_host.h
>> +++ b/arch/s390/include/asm/kvm_host.h
>> @@ -448,6 +448,8 @@ struct kvm_vcpu_arch {
>>   struct kvm_vm_stat {
>>       struct kvm_vm_stat_generic generic;
>>       u64 inject_io;
>> +    u64 io_390_adapter_map;
>> +    u64 io_390_adapter_unmap;
>>       u64 inject_float_mchk;
>>       u64 inject_pfault_done;
>>       u64 inject_service_signal;
>> @@ -479,6 +481,9 @@ struct s390_io_adapter {
>>       bool masked;
>>       bool swap;
>>       bool suppressible;
>> +    spinlock_t maps_lock;
>> +    struct list_head maps;
>> +    unsigned int nr_maps;
>>   };
>>   #define MAX_S390_IO_ADAPTERS ((MAX_ISC + 1) * 8)
>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> index 3bcdbbbb6891..5ad0b29c8c1b 100644
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
>> @@ -2411,24 +2411,34 @@ static int register_io_adapter(struct 
>> kvm_device *dev,
>>   {
>>       struct s390_io_adapter *adapter;
>>       struct kvm_s390_io_adapter adapter_info;
>> +    int rc = 0;
> 
> Why do you even set it to 0 if you return 0 on the success case instead 
> of return rc?

Janosch, thanks for your input. Agreed, I intend to use rc.>
>> +    mutex_lock(&dev->kvm->lock);
>>       if (copy_from_user(&adapter_info,
>> -               (void __user *)attr->addr, sizeof(adapter_info)))
>> -        return -EFAULT;
>> -
>> -    if (adapter_info.id >= MAX_S390_IO_ADAPTERS)
>> -        return -EINVAL;
>> -
>> +               (void __user *)attr->addr, sizeof(adapter_info))) {
>> +        rc = -EFAULT;
>> +        goto out;
>> +    }
>> +    if (adapter_info.id >= MAX_S390_IO_ADAPTERS) {
>> +        rc = -EINVAL;
>> +        goto out;
>> +    }
>>       adapter_info.id = array_index_nospec(adapter_info.id,
>>                            MAX_S390_IO_ADAPTERS);
>> -    if (dev->kvm->arch.adapters[adapter_info.id] != NULL)
>> -        return -EINVAL;
>> -
>> +    if (dev->kvm->arch.adapters[adapter_info.id] != NULL) {
>> +        rc = -EINVAL;
> 
> You could move the rc assignment outside of the braces and have one less 
> assignment. But if you like it more this way then keep it, I'm still 
> able to read it :)

I will leave the assignments inside the braces which will rarely, if 
ever, execute. And leave rc assigned as 0.

> 
>> +        goto out;
>> +    }
>>       adapter = kzalloc_obj(*adapter, GFP_KERNEL_ACCOUNT);
>> -    if (!adapter)
>> -        return -ENOMEM;
>> +    if (!adapter) {
>> +        rc = -ENOMEM;
>> +        goto out;
>> +    }
>> +    INIT_LIST_HEAD(&adapter->maps);
>> +    spin_lock_init(&adapter->maps_lock);
>> +    adapter->nr_maps = 0;
>>       adapter->id = adapter_info.id;
>>       adapter->isc = adapter_info.isc;
>>       adapter->maskable = adapter_info.maskable;
>> @@ -2437,8 +2447,13 @@ static int register_io_adapter(struct 
>> kvm_device *dev,
>>       adapter->suppressible = (adapter_info.flags) &
> 
> Why the braces?

That was existing code and not my change. But as a courtesy I will 
remove the braces to set it free.

> 
>>                   KVM_S390_ADAPTER_SUPPRESSIBLE;
>>       dev->kvm->arch.adapters[adapter->id] = adapter;
>> +    mutex_unlock(&dev->kvm->lock);
>>       return 0;
>> +
>> +out:
>> +    mutex_unlock(&dev->kvm->lock);
>> +    return rc;
> 
> If you set rc to 0 you can actually have one return for all cases.

Agreed, I will remove two lines before the label and let it fall through 
for one return for all cases.

> 
>>   }
>>   int kvm_s390_mask_adapter(struct kvm *kvm, unsigned int id, bool 
>> masked)
>> @@ -2453,12 +2468,151 @@ int kvm_s390_mask_adapter(struct kvm *kvm, 
>> unsigned int id, bool masked)
>>       return ret;
>>   }