From: "Maciej S. Szmigiero" <mail@maciej.szmigiero.name>
To: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
Vitaly Kuznetsov <vkuznets@redhat.com>,
Wanpeng Li <wanpengli@tencent.com>,
Jim Mattson <jmattson@google.com>,
Igor Mammedov <imammedo@redhat.com>,
Marc Zyngier <maz@kernel.org>, James Morse <james.morse@arm.com>,
Julien Thierry <julien.thierry.kdev@gmail.com>,
Suzuki K Poulose <suzuki.poulose@arm.com>,
Huacai Chen <chenhuacai@kernel.org>,
Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>,
Paul Mackerras <paulus@ozlabs.org>,
Christian Borntraeger <borntraeger@de.ibm.com>,
Janosch Frank <frankja@linux.ibm.com>,
David Hildenbrand <david@redhat.com>,
Cornelia Huck <cohuck@redhat.com>,
Claudio Imbrenda <imbrenda@linux.ibm.com>,
Joerg Roedel <joro@8bytes.org>,
kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 01/13] KVM: x86: Cache total page count to avoid traversing the memslot array
Date: Wed, 3 Nov 2021 12:59:27 +0100 [thread overview]
Message-ID: <8017cf9d-2b03-0c27-b78a-41b3d03c308b@maciej.szmigiero.name> (raw)
In-Reply-To: <YYBqMipZT9qcwDMt@google.com>
On 01.11.2021 23:29, Sean Christopherson wrote:
> On Wed, Oct 20, 2021, Sean Christopherson wrote:
>> On Wed, Oct 20, 2021, Maciej S. Szmigiero wrote:
>>> On 20.10.2021 00:24, Sean Christopherson wrote:
>>>> E.g. the whole thing can be
>>>>
>>>> if (!kvm->arch.n_requested_mmu_pages &&
>>>> (change == KVM_MR_CREATE || change == KVM_MR_DELETE)) {
>>>> unsigned long nr_mmu_pages;
>>>>
>>>> if (change == KVM_MR_CREATE) {
>>>> kvm->arch.n_memslots_pages += new->npages;
>>>> } else {
>>>> WARN_ON(kvm->arch.n_memslots_pages < old->npages);
>>>> kvm->arch.n_memslots_pages -= old->npages;
>>>> }
>>>>
>>>> nr_mmu_pages = (unsigned long)kvm->arch.n_memslots_pages;
>>>> nr_mmu_pages *= (KVM_PERMILLE_MMU_PAGES / 1000);
>>>
>>> The above line will set nr_mmu_pages to zero since KVM_PERMILLE_MMU_PAGES
>>> is 20, so when integer-divided by 1000 will result in a multiplication
>>> coefficient of zero.
>>
>> Ugh, math. And thus do_div() to avoid the whole 64-bit divide issue on 32-bit KVM.
>> Bummer.
>
> I was revisiting this today because (a) simply making n_memslots_pages a u64 doesn't
> cleanly handle the case where the resulting nr_mmu_pages would wrap,
Handling this case without capping total n_memslots_pages would require
either capping memslots_pages on 32-bit KVM to make it fit in 32-bits or
changing kvm_mmu_change_mmu_pages() and all the logic further down to
accept u64's.
> (b) any fix
> in that are should really go in a separate patch to fix
> kvm_mmu_calculate_default_mmu_pages() and then carry that behavior forward
>
> But as I dove deeper (and deeper), I came to the conclusion that supporting a
> total number of memslot pages that doesn't fit in an unsigned long is a waste of
> our time. With a 32-bit kernel, userspace can at most address 3gb of virtual
> memory, whereas wrapping the total number of pages would require 4tb+ of guest
> physical memory. Even with x86's second address space for SMM, that means userspace
> would need to alias all of guest memory more than one _thousand_ times. And on
> older hardware with MAXPHYADDR < 43, the guest couldn't actually access any of those
> aliases even if userspace lied about guest.MAXPHYADDR.
>
> So unless I'm missing something, or PPC or MIPS has some crazy way for a 32-bit
> host to support 4TB of guest memory, my vote would be to explicitly disallow
> creating more memslot pages than can fit in an unsigned long. Then x86 KVM could
> reuse the cache nr_memslot_pages and x86's MMU wouldn't have to update a big pile
> of code to support a scenario that practically speaking is useless.
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 72b329e82089..acabdbdef5cf 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -552,6 +552,7 @@ struct kvm {
> */
> struct mutex slots_arch_lock;
> struct mm_struct *mm; /* userspace tied to this vm */
> + unsigned long nr_memslot_pages;
> struct kvm_memslots __rcu *memslots[KVM_ADDRESS_SPACE_NUM];
> struct kvm_vcpu *vcpus[KVM_MAX_VCPUS];
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 8bf4b89cfa03..c63fc5c05322 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1567,6 +1567,15 @@ static void kvm_commit_memory_region(struct kvm *kvm,
> const struct kvm_memory_slot *new,
> enum kvm_mr_change change)
> {
> + /*
> + * Update the total number of memslot pages before calling the arch
> + * hook so that architectures can consume the result directly.
> + */
> + if (change == KVM_MR_DELETE)
> + kvm->nr_memslot_pages -= old->npages;
> + else if (change == KVM_MR_CREATE)
> + kvm->nr_memslot_pages += new->npages;
> +
> kvm_arch_commit_memory_region(kvm, old, new, change);
>
> /*
> @@ -1738,6 +1747,9 @@ int __kvm_set_memory_region(struct kvm *kvm,
> if (!old || !old->npages)
> return -EINVAL;
>
> + if (WARN_ON_ONCE(kvm->nr_memslot_pages < old->npages))
> + return -EIO;
> +
> memset(&new, 0, sizeof(new));
> new.id = id;
> new.as_id = as_id;
> @@ -1756,6 +1768,13 @@ int __kvm_set_memory_region(struct kvm *kvm,
>
> if (!old || !old->npages) {
> change = KVM_MR_CREATE;
> +
> + /*
> + * To simplify KVM internals, the total number of pages across
> + * all memslots must fit in an unsigned long.
> + */
> + if ((kvm->nr_memslot_pages + new.npages) < kvm->nr_memslot_pages)
> + return -EINVAL;
> } else { /* Modify an existing slot. */
> if ((new.userspace_addr != old->userspace_addr) ||
> (new.npages != old->npages) ||
>
Capping total n_memslots_pages makes sense to me to avoid the (existing)
nr_mmu_pages wraparound issue, will update the next patchset version
accordingly.
Thanks,
Maciej
next prev parent reply other threads:[~2021-11-03 11:59 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-20 21:38 [PATCH v5 00/13] KVM: Scalable memslots implementation Maciej S. Szmigiero
2021-09-20 21:38 ` [PATCH v5 01/13] KVM: x86: Cache total page count to avoid traversing the memslot array Maciej S. Szmigiero
2021-10-19 22:24 ` Sean Christopherson
2021-10-19 22:31 ` Sean Christopherson
2021-10-20 18:40 ` Maciej S. Szmigiero
2021-10-20 18:41 ` Maciej S. Szmigiero
2021-10-20 19:01 ` Sean Christopherson
2021-11-01 22:29 ` Sean Christopherson
2021-11-03 11:59 ` Maciej S. Szmigiero [this message]
2021-11-03 14:47 ` Sean Christopherson
2021-11-03 15:38 ` Maciej S. Szmigiero
2021-09-20 21:38 ` [PATCH v5 02/13] KVM: x86: Don't call kvm_mmu_change_mmu_pages() if the count hasn't changed Maciej S. Szmigiero
2021-09-20 21:38 ` [PATCH v5 03/13] KVM: Add "old" memslot parameter to kvm_arch_prepare_memory_region() Maciej S. Szmigiero
2021-09-20 21:38 ` [PATCH v5 04/13] KVM: x86: Move n_memslots_pages recalc " Maciej S. Szmigiero
2021-10-19 22:38 ` Sean Christopherson
2021-10-20 18:41 ` Maciej S. Szmigiero
2021-09-20 21:38 ` [PATCH v5 05/13] KVM: Integrate gfn_to_memslot_approx() into search_memslots() Maciej S. Szmigiero
2021-10-19 23:38 ` Sean Christopherson
2021-10-20 18:41 ` Maciej S. Szmigiero
2021-09-20 21:38 ` [PATCH v5 06/13] KVM: Move WARN on invalid memslot index to update_memslots() Maciej S. Szmigiero
2021-10-19 23:42 ` Sean Christopherson
2021-09-20 21:38 ` [PATCH v5 07/13] KVM: Just resync arch fields when slots_arch_lock gets reacquired Maciej S. Szmigiero
2021-10-19 23:55 ` Sean Christopherson
2021-10-20 18:41 ` Maciej S. Szmigiero
2021-10-20 18:57 ` Sean Christopherson
2021-10-20 18:58 ` Maciej S. Szmigiero
2021-09-20 21:38 ` [PATCH v5 08/13] KVM: Resolve memslot ID via a hash table instead of via a static array Maciej S. Szmigiero
2021-10-20 0:43 ` Sean Christopherson
2021-10-20 18:42 ` Maciej S. Szmigiero
2021-10-20 22:39 ` Sean Christopherson
2021-10-21 14:15 ` Maciej S. Szmigiero
2021-09-20 21:38 ` [PATCH v5 09/13] KVM: Use interval tree to do fast hva lookup in memslots Maciej S. Szmigiero
2021-10-26 18:19 ` Sean Christopherson
2021-10-26 18:46 ` Maciej S. Szmigiero
2021-09-20 21:38 ` [PATCH v5 10/13] KVM: s390: Introduce kvm_s390_get_gfn_end() Maciej S. Szmigiero
2021-09-20 21:38 ` [PATCH v5 11/13] KVM: Keep memslots in tree-based structures instead of array-based ones Maciej S. Szmigiero
2021-10-27 0:36 ` Sean Christopherson
2021-10-27 23:54 ` Sean Christopherson
2021-10-28 22:22 ` Sean Christopherson
2021-09-20 21:39 ` [PATCH v5 12/13] KVM: Optimize gfn lookup in kvm_zap_gfn_range() Maciej S. Szmigiero
2021-10-20 23:47 ` Sean Christopherson
2021-10-21 14:16 ` Maciej S. Szmigiero
2021-10-21 16:30 ` Sean Christopherson
2021-10-21 21:44 ` Maciej S. Szmigiero
2021-09-20 21:39 ` [PATCH v5 13/13] KVM: Optimize overlapping memslots check Maciej S. Szmigiero
2021-10-26 18:59 ` Sean Christopherson
2021-10-27 13:48 ` Maciej S. Szmigiero
2021-10-28 17:53 ` Sean Christopherson
2021-10-29 16:23 ` Maciej S. Szmigiero
2021-10-30 0:32 ` Sean Christopherson
2021-10-19 22:07 ` [PATCH v5 00/13] KVM: Scalable memslots implementation Sean Christopherson
2021-10-20 18:40 ` Maciej S. Szmigiero
2021-10-20 19:58 ` Sean Christopherson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8017cf9d-2b03-0c27-b78a-41b3d03c308b@maciej.szmigiero.name \
--to=mail@maciej.szmigiero.name \
--cc=aleksandar.qemu.devel@gmail.com \
--cc=borntraeger@de.ibm.com \
--cc=chenhuacai@kernel.org \
--cc=cohuck@redhat.com \
--cc=david@redhat.com \
--cc=frankja@linux.ibm.com \
--cc=imammedo@redhat.com \
--cc=imbrenda@linux.ibm.com \
--cc=james.morse@arm.com \
--cc=jmattson@google.com \
--cc=joro@8bytes.org \
--cc=julien.thierry.kdev@gmail.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maz@kernel.org \
--cc=paulus@ozlabs.org \
--cc=pbonzini@redhat.com \
--cc=seanjc@google.com \
--cc=suzuki.poulose@arm.com \
--cc=vkuznets@redhat.com \
--cc=wanpengli@tencent.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).