Re: [PATCH v3.1 02/37] KVM: s390/interrupt: do not pin adapter interrupt pages

[Christian Borntraeger <borntraeger@de.ibm.com>]

On 21.02.20 11:41, Cornelia Huck wrote:
> On Fri, 21 Feb 2020 03:09:42 -0500
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
>>
>> The adapter interrupt page containing the indicator bits is currently
>> pinned. That means that a guest with many devices can pin a lot of
>> memory pages in the host. This also complicates the reference tracking
>> which is needed for memory management handling of protected virtual
>> machines. It might also have some strange side effects for madvise
>> MADV_DONTNEED and other things.
>>
>> We can simply try to get the userspace page set the bits and free the
>> page. By storing the userspace address in the irq routing entry instead
>> of the guest address we can actually avoid many lookups and list walks
>> so that this variant is very likely not slower.
>>
>> If userspace messes around with the memory slots the worst thing that
>> can happen is that we write to some other memory within that process.
>> As we get the the page with FOLL_WRITE this can also not be used to
>> write to shared read-only pages.
>>
>> Signed-off-by: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
>> [borntraeger@de.ibm.com: patch simplification]
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  Documentation/virt/kvm/devices/s390_flic.rst |  11 +-
>>  arch/s390/include/asm/kvm_host.h             |   3 -
>>  arch/s390/kvm/interrupt.c                    | 172 ++++++-------------
>>  3 files changed, 52 insertions(+), 134 deletions(-)
> 
> (...)
> 
>> @@ -2456,12 +2378,13 @@ static int modify_io_adapter(struct kvm_device *dev,
>>  		if (ret > 0)
>>  			ret = 0;
>>  		break;
>> +	/*
>> +	 * We resolve the gpa to hva when setting the IRQ routing. the set_irq
>> +	 * code uses get_user_pages_remote() to do the actual write.
>> +	 */
> 
> What about:
> 
> "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."

ack, reads better.

> 
>>  	case KVM_S390_IO_ADAPTER_MAP:
>> -		ret = kvm_s390_adapter_map(dev->kvm, req.id, req.addr);
>> -		break;
>>  	case KVM_S390_IO_ADAPTER_UNMAP:
>> -		ret = kvm_s390_adapter_unmap(dev->kvm, req.id, req.addr);
>> -		break;
>> +		return 0;
> 
> I think
> 		ret = 0;
> 		break;
> would be better, as the other cases do not do a direct return, either.

fine with me. ack
> 
>>  	default:
>>  		ret = -EINVAL;
>>  	}
>> @@ -2699,19 +2622,17 @@ static unsigned long get_ind_bit(__u64 addr, unsigned long bit_nr, bool swap)
>>  	return swap ? (bit ^ (BITS_PER_LONG - 1)) : bit;
>>  }
>>  
>> -static struct s390_map_info *get_map_info(struct s390_io_adapter *adapter,
>> -					  u64 addr)
>> +static struct page *get_map_page(struct kvm *kvm,
>> +				 struct s390_io_adapter *adapter,
> 
> adapter seems to be unused in this function now? Should we remove it from the parameter list?

yep, thanks. removed.