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

Christian Borntraeger
2020-02-21T11:05:38Z

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.