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

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

On 3/26/26 11:56 AM, Janosch Frank wrote:
> On 3/26/26 02:42, 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.
>>
>> 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.
>>
>> Signed-off-by: Douglas Freimuth <freimuth@linux.ibm.com>
>> ---
> 
> Looks good, two nits below.
> 
> [...]
>>           if (ret > 0)
>>               ret = 0;
>>           break;
>> -    /*
>> -     * The following operations are no longer needed and therefore 
>> no-ops.
>> -     * The gpa to hva translation is done when an IRQ route is set 
>> up. The
>> -     * set_irq code uses get_user_pages_remote() to do the actual write.
>> -     */
>>       case KVM_S390_IO_ADAPTER_MAP:
>>       case KVM_S390_IO_ADAPTER_UNMAP:
>> -        ret = 0;
>> +        /* If in Secure Execution mode do not long term pin. */
>> +        mutex_lock(&dev->kvm->lock);
>> +        if (kvm_s390_pv_is_protected(dev->kvm)) {
>> +            mutex_unlock(&dev->kvm->lock);
>> +            return 0;
>> +        }
>> +        mutex_unlock(&dev->kvm->lock);
>> +        idx = srcu_read_lock(&dev->kvm->srcu);
>> +        host_addr = gpa_to_hva(dev->kvm, req.addr);
>> +        if (kvm_is_error_hva(host_addr)) {
>> +            srcu_read_unlock(&dev->kvm->srcu, idx);
>> +            return -EFAULT;
>> +            }
> 
> Alignment issue
> 
> [...]
> 

I will correct that. (Surprised checkpatch strict didn't catch it.)

>>   static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
>>   {
>>       const bool need_lock = (cmd->cmd != KVM_PV_ASYNC_CLEANUP_PERFORM);
>> @@ -2503,6 +2522,7 @@ static int kvm_s390_handle_pv(struct kvm *kvm, 
>> struct kvm_pv_cmd *cmd)
>>       switch (cmd->cmd) {
>>       case KVM_PV_ENABLE: {
>> +        kvm_s390_unmap_all_adapters_pv(kvm);
> 
> Shouldn't this be located after the check that's below?
> 

While all of my tests do not observe it, not sure if there is a state 
change where anything is left mapped while in pv. There should not be so 
I can move it after the check as you mention.

>>           r = -EINVAL;
>>           if (kvm_s390_pv_is_protected(kvm))
>>               break;
>