[PATCH RFC 0/4] KVM: x86: Drastically raise KVM_USER_MEM_SLOTS limit

[PATCH RFC 0/4] KVM: x86: Drastically raise KVM_USER_MEM_SLOTS limit

[Vitaly Kuznetsov]

TL;DR: any particular reason why KVM_USER_MEM_SLOTS is so low?

Longer version:

Current KVM_USER_MEM_SLOTS limit (509) can be a limiting factor for some
configurations. In particular, when QEMU tries to start a Windows guest
with Hyper-V SynIC enabled and e.g. 256 vCPUs the limit is hit as SynIC
requires two pages per vCPU and the guest is free to pick any GFN for
each of them, this fragments memslots as QEMU wants to have a separate
memslot for each of these pages (which are supposed to act as 'overlay'
pages).

Memory slots are allocated dynamically in KVM when added so the only real
limitation is 'id_to_index' array which is 'short'. We don't have any
KVM_MEM_SLOTS_NUM/KVM_USER_MEM_SLOTS-sized statically defined arrays.

We could've just raised the limit to e.g. '1021' (we have 3 private
memslots on x86) and this should be enough for now as KVM_MAX_VCPUS is
'288' but AFAIK there are plans to raise this limit as well.

Vitaly Kuznetsov (4):
  KVM: x86: Drop redundant KVM_MEM_SLOTS_NUM definition
  KVM: mips: Drop KVM_PRIVATE_MEM_SLOTS definition
  KVM: Define KVM_USER_MEM_SLOTS in arch-neutral
    include/linux/kvm_host.h
  KVM: x86: Stop limiting KVM_USER_MEM_SLOTS

 arch/mips/include/asm/kvm_host.h | 2 --
 arch/x86/include/asm/kvm_host.h  | 3 +--
 include/linux/kvm_host.h         | 4 ++++
 3 files changed, 5 insertions(+), 4 deletions(-)

-- 
2.29.2

[PATCH RFC 1/4] KVM: x86: Drop redundant KVM_MEM_SLOTS_NUM definition

[Vitaly Kuznetsov]

KVM_MEM_SLOTS_NUM is already defined in include/linux/kvm_host.h the
exact same way.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/include/asm/kvm_host.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index c27cbe3baccc..1bcf67d76753 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -43,7 +43,6 @@
 #define KVM_USER_MEM_SLOTS 509
 /* memory slots that are not exposed to userspace */
 #define KVM_PRIVATE_MEM_SLOTS 3
-#define KVM_MEM_SLOTS_NUM (KVM_USER_MEM_SLOTS + KVM_PRIVATE_MEM_SLOTS)
 
 #define KVM_HALT_POLL_NS_DEFAULT 200000
 
-- 
2.29.2

Re: [PATCH RFC 1/4] KVM: x86: Drop redundant KVM_MEM_SLOTS_NUM definition

[Sean Christopherson]

On Fri, Jan 15, 2021, Vitaly Kuznetsov wrote:
> KVM_MEM_SLOTS_NUM is already defined in include/linux/kvm_host.h the
> exact same way.

I'm pretty sure you can remove the "#ifndef KVM_MEM_SLOTS_NUM" in
linux/kvm_host.h, looks like x86 is the only arch that was defining
KVM_MEM_SLOTS_NUM.

> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  arch/x86/include/asm/kvm_host.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index c27cbe3baccc..1bcf67d76753 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -43,7 +43,6 @@
>  #define KVM_USER_MEM_SLOTS 509
>  /* memory slots that are not exposed to userspace */
>  #define KVM_PRIVATE_MEM_SLOTS 3
> -#define KVM_MEM_SLOTS_NUM (KVM_USER_MEM_SLOTS + KVM_PRIVATE_MEM_SLOTS)
>  
>  #define KVM_HALT_POLL_NS_DEFAULT 200000
>  
> -- 
> 2.29.2
> 

[PATCH RFC 2/4] KVM: mips: Drop KVM_PRIVATE_MEM_SLOTS definition

[Vitaly Kuznetsov]

KVM_PRIVATE_MEM_SLOTS is set to '0' in include/linux/kvm_host.h already.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/mips/include/asm/kvm_host.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/mips/include/asm/kvm_host.h b/arch/mips/include/asm/kvm_host.h
index 24f3d0f9996b..603841ce7f42 100644
--- a/arch/mips/include/asm/kvm_host.h
+++ b/arch/mips/include/asm/kvm_host.h
@@ -84,8 +84,6 @@
 
 #define KVM_MAX_VCPUS		16
 #define KVM_USER_MEM_SLOTS	16
-/* memory slots that does not exposed to userspace */
-#define KVM_PRIVATE_MEM_SLOTS	0
 
 #define KVM_HALT_POLL_NS_DEFAULT 500000
 
-- 
2.29.2

[PATCH RFC 3/4] KVM: Define KVM_USER_MEM_SLOTS in arch-neutral include/linux/kvm_host.h

[Vitaly Kuznetsov]

Memory slots are allocated dynamically when added so the only real
limitation in KVM is 'id_to_index' array which is 'short'. Define
KVM_USER_MEM_SLOTS to the maximum possible value in the arch-neutral
include/linux/kvm_host.h, architectures can still overtide the setting
if needed.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 include/linux/kvm_host.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index f3b1013fb22c..ab83a20a52ca 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -425,6 +425,10 @@ struct kvm_irq_routing_table {
 #define KVM_PRIVATE_MEM_SLOTS 0
 #endif
 
+#ifndef KVM_USER_MEM_SLOTS
+#define KVM_USER_MEM_SLOTS (SHRT_MAX - KVM_PRIVATE_MEM_SLOTS)
+#endif
+
 #ifndef KVM_MEM_SLOTS_NUM
 #define KVM_MEM_SLOTS_NUM (KVM_USER_MEM_SLOTS + KVM_PRIVATE_MEM_SLOTS)
 #endif
-- 
2.29.2

Re: [PATCH RFC 3/4] KVM: Define KVM_USER_MEM_SLOTS in arch-neutral include/linux/kvm_host.h

[Sean Christopherson]

On Fri, Jan 15, 2021, Vitaly Kuznetsov wrote:
> Memory slots are allocated dynamically when added so the only real
> limitation in KVM is 'id_to_index' array which is 'short'. Define
> KVM_USER_MEM_SLOTS to the maximum possible value in the arch-neutral
> include/linux/kvm_host.h, architectures can still overtide the setting
> if needed.

Leaving the max number of slots nearly unbounded is probably a bad idea.  If my
math is not completely wrong, this would let userspace allocate 6mb of kernel
memory per VM.  Actually, worst case scenario would be 12mb since modifying
memslots temporarily has two allocations.

If we remove the arbitrary limit, maybe replace it with a module param with a
sane default?

> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  include/linux/kvm_host.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index f3b1013fb22c..ab83a20a52ca 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -425,6 +425,10 @@ struct kvm_irq_routing_table {
>  #define KVM_PRIVATE_MEM_SLOTS 0
>  #endif
>  
> +#ifndef KVM_USER_MEM_SLOTS
> +#define KVM_USER_MEM_SLOTS (SHRT_MAX - KVM_PRIVATE_MEM_SLOTS)
> +#endif
> +
>  #ifndef KVM_MEM_SLOTS_NUM
>  #define KVM_MEM_SLOTS_NUM (KVM_USER_MEM_SLOTS + KVM_PRIVATE_MEM_SLOTS)
>  #endif
> -- 
> 2.29.2
> 

Re: [PATCH RFC 3/4] KVM: Define KVM_USER_MEM_SLOTS in arch-neutral include/linux/kvm_host.h

[Vitaly Kuznetsov]

Sean Christopherson <seanjc@google.com> writes:

> On Fri, Jan 15, 2021, Vitaly Kuznetsov wrote:
>> Memory slots are allocated dynamically when added so the only real
>> limitation in KVM is 'id_to_index' array which is 'short'. Define
>> KVM_USER_MEM_SLOTS to the maximum possible value in the arch-neutral
>> include/linux/kvm_host.h, architectures can still overtide the setting
>> if needed.
>
> Leaving the max number of slots nearly unbounded is probably a bad idea.  If my
> math is not completely wrong, this would let userspace allocate 6mb of kernel
> memory per VM.  Actually, worst case scenario would be 12mb since modifying
> memslots temporarily has two allocations.

Yea I had thought too but on the other hand, if your VMM went rogue and
and is trying to eat all your memory, how is allocating 32k memslots
different from e.g. creating 64 VMs with 512 slots each? We use
GFP_KERNEL_ACCOUNT to allocate memslots (and other per-VM stuff) so
e.g. cgroup limits should work ...

>
> If we remove the arbitrary limit, maybe replace it with a module param with a
> sane default?

This can be a good solution indeed. The only question then is what should
we pick as the default? It seems to me this can be KVM_MAX_VCPUS
dependent, e.g. 4 x KVM_MAX_VCPUS would suffice.

>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  include/linux/kvm_host.h | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index f3b1013fb22c..ab83a20a52ca 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -425,6 +425,10 @@ struct kvm_irq_routing_table {
>>  #define KVM_PRIVATE_MEM_SLOTS 0
>>  #endif
>>  
>> +#ifndef KVM_USER_MEM_SLOTS
>> +#define KVM_USER_MEM_SLOTS (SHRT_MAX - KVM_PRIVATE_MEM_SLOTS)
>> +#endif
>> +
>>  #ifndef KVM_MEM_SLOTS_NUM
>>  #define KVM_MEM_SLOTS_NUM (KVM_USER_MEM_SLOTS + KVM_PRIVATE_MEM_SLOTS)
>>  #endif
>> -- 
>> 2.29.2
>> 
>

-- 
Vitaly

Re: [PATCH RFC 3/4] KVM: Define KVM_USER_MEM_SLOTS in arch-neutral include/linux/kvm_host.h

[Sean Christopherson]

On Mon, Jan 18, 2021, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@google.com> writes:
> 
> > On Fri, Jan 15, 2021, Vitaly Kuznetsov wrote:
> >> Memory slots are allocated dynamically when added so the only real
> >> limitation in KVM is 'id_to_index' array which is 'short'. Define
> >> KVM_USER_MEM_SLOTS to the maximum possible value in the arch-neutral
> >> include/linux/kvm_host.h, architectures can still overtide the setting
> >> if needed.
> >
> > Leaving the max number of slots nearly unbounded is probably a bad idea.  If my
> > math is not completely wrong, this would let userspace allocate 6mb of kernel
> > memory per VM.  Actually, worst case scenario would be 12mb since modifying
> > memslots temporarily has two allocations.
> 
> Yea I had thought too but on the other hand, if your VMM went rogue and
> and is trying to eat all your memory, how is allocating 32k memslots
> different from e.g. creating 64 VMs with 512 slots each? We use
> GFP_KERNEL_ACCOUNT to allocate memslots (and other per-VM stuff) so
> e.g. cgroup limits should work ...

I see it as an easy way to mitigate the damage.  E.g. if a containers use case
is spinning up hundreds of VMs and something goes awry in the config, it would
be the difference between consuming tens of MBs and hundreds of MBs.  Cgroup
limits should also be in play, but defense in depth and all that. 

> > If we remove the arbitrary limit, maybe replace it with a module param with a
> > sane default?
> 
> This can be a good solution indeed. The only question then is what should
> we pick as the default? It seems to me this can be KVM_MAX_VCPUS
> dependent, e.g. 4 x KVM_MAX_VCPUS would suffice.

Hrm, I don't love tying it to KVM_MAX_VPUCS.  Other than SynIC, are there any
other features/modes/configuration that cause the number of memslots to grop
with the number of vCPUs?  But, limiting via a module param does effectively
require using KVM_MAX_VCPUS, otherwise everyone that might run Windows guests
would have to override the default and thereby defeat the purpose of limiting by
default.

Were you planning on adding a capability to check for the new and improved
memslots limit, e.g. to know whether or not KVM might die on a large VM?
If so, requiring the VMM to call an ioctl() to set a higher (or lower?) limit
would be another option.  That wouldn't have the same permission requirements as
a module param, but it would likely be a more effective safeguard in practice,
e.g. use cases with a fixed number of memslots or a well-defined upper bound
could use the capability to limit themselves.

Thoughts?  An ioctl() feels a little over-engineered, but I suspect that adding
a module param that defaults to N*KVM_MAX_VPCUS will be a waste, e.g. no one
will ever touch the param and we'll end up with dead, rarely-tested code.

Re: [PATCH RFC 3/4] KVM: Define KVM_USER_MEM_SLOTS in arch-neutral include/linux/kvm_host.h

[Igor Mammedov]

On Tue, 19 Jan 2021 09:20:42 -0800
Sean Christopherson <seanjc@google.com> wrote:

> On Mon, Jan 18, 2021, Vitaly Kuznetsov wrote:
> > Sean Christopherson <seanjc@google.com> writes:
> >   
> > > On Fri, Jan 15, 2021, Vitaly Kuznetsov wrote:  
> > >> Memory slots are allocated dynamically when added so the only real
> > >> limitation in KVM is 'id_to_index' array which is 'short'. Define
> > >> KVM_USER_MEM_SLOTS to the maximum possible value in the arch-neutral
> > >> include/linux/kvm_host.h, architectures can still overtide the setting
> > >> if needed.  
> > >
> > > Leaving the max number of slots nearly unbounded is probably a bad idea.  If my
> > > math is not completely wrong, this would let userspace allocate 6mb of kernel
> > > memory per VM.  Actually, worst case scenario would be 12mb since modifying
> > > memslots temporarily has two allocations.  
> > 
> > Yea I had thought too but on the other hand, if your VMM went rogue and
> > and is trying to eat all your memory, how is allocating 32k memslots
> > different from e.g. creating 64 VMs with 512 slots each? We use
> > GFP_KERNEL_ACCOUNT to allocate memslots (and other per-VM stuff) so
> > e.g. cgroup limits should work ...  
> 
> I see it as an easy way to mitigate the damage.  E.g. if a containers use case
> is spinning up hundreds of VMs and something goes awry in the config, it would
> be the difference between consuming tens of MBs and hundreds of MBs.  Cgroup
> limits should also be in play, but defense in depth and all that. 
> 
> > > If we remove the arbitrary limit, maybe replace it with a module param with a
> > > sane default?  
> > 
> > This can be a good solution indeed. The only question then is what should
> > we pick as the default? It seems to me this can be KVM_MAX_VCPUS
> > dependent, e.g. 4 x KVM_MAX_VCPUS would suffice.  
> 
> Hrm, I don't love tying it to KVM_MAX_VPUCS.  Other than SynIC, are there any
> other features/modes/configuration that cause the number of memslots to grop

(NV)DIMMs in QEMU also consume slot/device but do not depend on vCPUs number.
Due current slot limit only 256 DIMMs are allowed. But if vCPUs start consuming
extra memslots, they will contend over possible slots.

> with the number of vCPUs?  But, limiting via a module param does effectively
> require using KVM_MAX_VCPUS, otherwise everyone that might run Windows guests
> would have to override the default and thereby defeat the purpose of limiting by
> default.
> 
> Were you planning on adding a capability to check for the new and improved
> memslots limit, e.g. to know whether or not KVM might die on a large VM?
> If so, requiring the VMM to call an ioctl() to set a higher (or lower?) limit
> would be another option.  That wouldn't have the same permission requirements as
> a module param, but it would likely be a more effective safeguard in practice,
> e.g. use cases with a fixed number of memslots or a well-defined upper bound
> could use the capability to limit themselves.
Currently QEMU uses KVM_CAP_NR_MEMSLOTS to get limit, and depending on place the
limit is reached it either fails gracefully (i.e. it checks if free slot is
available before slot allocation) or aborts (in case where it tries to allocate
slot without check).
New ioctl() seems redundant as we already have upper limit check
(unless it would allow go over that limit, which in its turn defeats purpose of
the limit).


> Thoughts?  An ioctl() feels a little over-engineered, but I suspect that adding
> a module param that defaults to N*KVM_MAX_VPCUS will be a waste, e.g. no one
> will ever touch the param and we'll end up with dead, rarely-tested code.
> 

Re: [PATCH RFC 3/4] KVM: Define KVM_USER_MEM_SLOTS in arch-neutral include/linux/kvm_host.h

[Vitaly Kuznetsov]

Igor Mammedov <imammedo@redhat.com> writes:

> On Tue, 19 Jan 2021 09:20:42 -0800
> Sean Christopherson <seanjc@google.com> wrote:
>
>> 
>> Were you planning on adding a capability to check for the new and improved
>> memslots limit, e.g. to know whether or not KVM might die on a large VM?
>> If so, requiring the VMM to call an ioctl() to set a higher (or lower?) limit
>> would be another option.  That wouldn't have the same permission requirements as
>> a module param, but it would likely be a more effective safeguard in practice,
>> e.g. use cases with a fixed number of memslots or a well-defined upper bound
>> could use the capability to limit themselves.
> Currently QEMU uses KVM_CAP_NR_MEMSLOTS to get limit, and depending on place the
> limit is reached it either fails gracefully (i.e. it checks if free slot is
> available before slot allocation) or aborts (in case where it tries to allocate
> slot without check).

FWIW, 'synic problem' causes it to abort.

> New ioctl() seems redundant as we already have upper limit check
> (unless it would allow go over that limit, which in its turn defeats purpose of
> the limit).
>

Right, I didn't plan to add any new CAP as what we already have should
be enough to query the limits. Having an ioctl to set the upper limit
seems complicated: with device and CPU hotplug it may not be easy to
guess what it should be upfront so VMMs will likely just add a call to
raise the limit in memslot modification code so it won't be providing
any new protection.


>> Thoughts?  An ioctl() feels a little over-engineered, but I suspect that adding
>> a module param that defaults to N*KVM_MAX_VPCUS will be a waste, e.g. no one
>> will ever touch the param and we'll end up with dead, rarely-tested code.

Alternatively, we can hard-code KVM_USER_MEM_SLOTS to N*KVM_MAX_VPCUS so
no new parameter is needed but personally, I'd prefer to have it
configurable (in case we decide to set it to something lower than
SHRT_MAX of course) even if it won't be altered very often (which is a
good thing for 'general purpose' usage, right?). First, it will allow
tightening the limit for some very specific deployments (e.g. FaaS/
Firecracker-style) to say '20' which should be enough. Second, we may be
overlooking some configurations where the number of memslots is actually
dependent on the number of vCPUs but nobody complained so far just
because these configutrarions use a farly small number and the ceiling
wasn't hit yet.

One more spare thought. Per-vCPU memslots may come handy if someone
decides to move some of the KVM PV features to userspace. E.g. I can
imagine an attempt to move async_pf out of kernel.

-- 
Vitaly

Re: [PATCH RFC 3/4] KVM: Define KVM_USER_MEM_SLOTS in arch-neutral include/linux/kvm_host.h

[Sean Christopherson]

On Wed, Jan 20, 2021, Vitaly Kuznetsov wrote:
> Igor Mammedov <imammedo@redhat.com> writes:
> 
> > On Tue, 19 Jan 2021 09:20:42 -0800
> > Sean Christopherson <seanjc@google.com> wrote:
> >
> >> 
> >> Were you planning on adding a capability to check for the new and improved
> >> memslots limit, e.g. to know whether or not KVM might die on a large VM?
> >> If so, requiring the VMM to call an ioctl() to set a higher (or lower?) limit
> >> would be another option.  That wouldn't have the same permission requirements as
> >> a module param, but it would likely be a more effective safeguard in practice,
> >> e.g. use cases with a fixed number of memslots or a well-defined upper bound
> >> could use the capability to limit themselves.
> > Currently QEMU uses KVM_CAP_NR_MEMSLOTS to get limit, and depending on place the
> > limit is reached it either fails gracefully (i.e. it checks if free slot is
> > available before slot allocation) or aborts (in case where it tries to allocate
> > slot without check).
> 
> FWIW, 'synic problem' causes it to abort.
> 
> > New ioctl() seems redundant as we already have upper limit check
> > (unless it would allow go over that limit, which in its turn defeats purpose of
> > the limit).

Gah, and I even looked for an ioctl().  No idea how I didn't find NR_MEMSLOTS.

> Right, I didn't plan to add any new CAP as what we already have should
> be enough to query the limits. Having an ioctl to set the upper limit
> seems complicated: with device and CPU hotplug it may not be easy to
> guess what it should be upfront so VMMs will likely just add a call to
> raise the limit in memslot modification code so it won't be providing
> any new protection.

True.  Maybe the best approach, if we want to make the limit configurable, would
be to make a lower limit opt-in.  I.e. default=KVM_CAP_NR_MEMSLOTS=SHRT_MAX,
with an ioctl() to set a lower limit.  That would also allow us to defer adding
a new ioctl() until someone actually plans on using it.

> >> Thoughts?  An ioctl() feels a little over-engineered, but I suspect that adding
> >> a module param that defaults to N*KVM_MAX_VPCUS will be a waste, e.g. no one
> >> will ever touch the param and we'll end up with dead, rarely-tested code.
> 
> Alternatively, we can hard-code KVM_USER_MEM_SLOTS to N*KVM_MAX_VPCUS so
> no new parameter is needed but personally, I'd prefer to have it
> configurable (in case we decide to set it to something lower than
> SHRT_MAX of course) even if it won't be altered very often (which is a
> good thing for 'general purpose' usage, right?).
>
> First, it will allow tightening the limit for some very specific deployments
> (e.g. FaaS/ Firecracker-style) to say '20' which should be enough.

A module param likely isn't usable for many such deployments though, as it would
require a completely isolated pool of systems.  That's why an ioctl() is
appealing; the expected number of memslots is a property of the VM, not of the
platform.

> Second, we may be overlooking some configurations where the number of
> memslots is actually dependent on the number of vCPUs but nobody complained
> so far just because these configutrarions use a farly small number and the
> ceiling wasn't hit yet.
> 
> One more spare thought. Per-vCPU memslots may come handy if someone
> decides to move some of the KVM PV features to userspace. E.g. I can
> imagine an attempt to move async_pf out of kernel.

Memslots aren't per-vCPU though, any userspace feature that tries to treat them
as such will be flawed in some way.  Practically speaking, per-vCPU memslots
just aren't feasible, at least not without KVM changes that are likely
unpalatable, e.g. KVM would need to incorporate the existence of a vCPU specific
memslot into the MMU role.

[PATCH RFC 4/4] KVM: x86: Stop limiting KVM_USER_MEM_SLOTS

[Vitaly Kuznetsov]

Current KVM_USER_MEM_SLOTS limit (509) can be a limiting factor for some
configurations. In particular, when QEMU tries to start a Windows guest
with Hyper-V SynIC enabled and e.g. 256 vCPUs the limit is hit as SynIC
requires two pages per vCPU and the guest is free to pick any GFN for
each of them, this fragments memslots as QEMU wants to have a separate
memslot for each of these pages (which are supposed to act as 'overlay'
pages).

Memory slots are allocated dynamically in KVM when added so the only real
limitation is 'id_to_index' array which is 'short'. We don't have any
KVM_MEM_SLOTS_NUM/KVM_USER_MEM_SLOTS-sized statically defined arrays.

Let's drop the limitation.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/include/asm/kvm_host.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 1bcf67d76753..546b839de797 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -40,7 +40,7 @@
 #define KVM_MAX_VCPUS 288
 #define KVM_SOFT_MAX_VCPUS 240
 #define KVM_MAX_VCPU_ID 1023
-#define KVM_USER_MEM_SLOTS 509
+
 /* memory slots that are not exposed to userspace */
 #define KVM_PRIVATE_MEM_SLOTS 3
 
-- 
2.29.2

Re: [PATCH RFC 0/4] KVM: x86: Drastically raise KVM_USER_MEM_SLOTS limit

[Sean Christopherson]

On Fri, Jan 15, 2021, Vitaly Kuznetsov wrote:
> TL;DR: any particular reason why KVM_USER_MEM_SLOTS is so low?

Because memslots were allocated statically up until fairly recently (v5.7), and
IIRC consumed ~92kb.  Doubling that for every VM would be quite painful. 

> Longer version:
> 
> Current KVM_USER_MEM_SLOTS limit (509) can be a limiting factor for some
> configurations. In particular, when QEMU tries to start a Windows guest
> with Hyper-V SynIC enabled and e.g. 256 vCPUs the limit is hit as SynIC
> requires two pages per vCPU and the guest is free to pick any GFN for
> each of them, this fragments memslots as QEMU wants to have a separate
> memslot for each of these pages (which are supposed to act as 'overlay'
> pages).

What exactly does QEMU do on the backend?  I poked around the code a bit, but
didn't see anything relevant.

> Memory slots are allocated dynamically in KVM when added so the only real
> limitation is 'id_to_index' array which is 'short'. We don't have any
> KVM_MEM_SLOTS_NUM/KVM_USER_MEM_SLOTS-sized statically defined arrays.
> 
> We could've just raised the limit to e.g. '1021' (we have 3 private
> memslots on x86) and this should be enough for now as KVM_MAX_VCPUS is
> '288' but AFAIK there are plans to raise this limit as well.
> 
> Vitaly Kuznetsov (4):
>   KVM: x86: Drop redundant KVM_MEM_SLOTS_NUM definition
>   KVM: mips: Drop KVM_PRIVATE_MEM_SLOTS definition
>   KVM: Define KVM_USER_MEM_SLOTS in arch-neutral
>     include/linux/kvm_host.h
>   KVM: x86: Stop limiting KVM_USER_MEM_SLOTS
> 
>  arch/mips/include/asm/kvm_host.h | 2 --
>  arch/x86/include/asm/kvm_host.h  | 3 +--
>  include/linux/kvm_host.h         | 4 ++++
>  3 files changed, 5 insertions(+), 4 deletions(-)
> 
> -- 
> 2.29.2
> 

Re: [PATCH RFC 0/4] KVM: x86: Drastically raise KVM_USER_MEM_SLOTS limit

[Vitaly Kuznetsov]

Sean Christopherson <seanjc@google.com> writes:

> On Fri, Jan 15, 2021, Vitaly Kuznetsov wrote:
>> TL;DR: any particular reason why KVM_USER_MEM_SLOTS is so low?
>
> Because memslots were allocated statically up until fairly recently (v5.7), and
> IIRC consumed ~92kb.  Doubling that for every VM would be quite painful. 
>

I should've added 'now' to the question). So the main reason is gone,
thanks for the confirmation!

>> Longer version:
>> 
>> Current KVM_USER_MEM_SLOTS limit (509) can be a limiting factor for some
>> configurations. In particular, when QEMU tries to start a Windows guest
>> with Hyper-V SynIC enabled and e.g. 256 vCPUs the limit is hit as SynIC
>> requires two pages per vCPU and the guest is free to pick any GFN for
>> each of them, this fragments memslots as QEMU wants to have a separate
>> memslot for each of these pages (which are supposed to act as 'overlay'
>> pages).
>
> What exactly does QEMU do on the backend?  I poked around the code a bit, but
> didn't see anything relevant.
>

In QEMU's terms it registers memory sub-regions for these two pages (see
synic_update() in hw/hyperv/hyperv.c). Memory for these page-sized
sub-regions is allocated separately so in KVM terms they become
page-sized slots and previously continuous 'system memory' slot breaks
into several slots.

-- 
Vitaly

Re: [PATCH RFC 0/4] KVM: x86: Drastically raise KVM_USER_MEM_SLOTS limit

[Sean Christopherson]

On Fri, Jan 15, 2021, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@google.com> writes:
> 
> > On Fri, Jan 15, 2021, Vitaly Kuznetsov wrote:
> >> Longer version:
> >> 
> >> Current KVM_USER_MEM_SLOTS limit (509) can be a limiting factor for some
> >> configurations. In particular, when QEMU tries to start a Windows guest
> >> with Hyper-V SynIC enabled and e.g. 256 vCPUs the limit is hit as SynIC
> >> requires two pages per vCPU and the guest is free to pick any GFN for
> >> each of them, this fragments memslots as QEMU wants to have a separate
> >> memslot for each of these pages (which are supposed to act as 'overlay'
> >> pages).
> >
> > What exactly does QEMU do on the backend?  I poked around the code a bit, but
> > didn't see anything relevant.
> >
> 
> In QEMU's terms it registers memory sub-regions for these two pages (see
> synic_update() in hw/hyperv/hyperv.c). Memory for these page-sized
> sub-regions is allocated separately so in KVM terms they become
> page-sized slots and previously continuous 'system memory' slot breaks
> into several slots.

Doh, I had a super stale version checked out (2.9.50), no wonder I couldn't find
anything.

Isn't the memslot approach inherently flawed in that the SynIC is per-vCPU, but
memslots are per-VM?  E.g. if vCPU1 accesses vCPU0's SynIC GPA, I would expect
that to access real memory, not the overlay.  Or is there more QEMU magic going
on that I'm missing?

Re: [PATCH RFC 0/4] KVM: x86: Drastically raise KVM_USER_MEM_SLOTS limit

[Maciej S. Szmigiero]

On 15.01.2021 14:18, Vitaly Kuznetsov wrote:
> TL;DR: any particular reason why KVM_USER_MEM_SLOTS is so low?
> 
> Longer version:
> 
> Current KVM_USER_MEM_SLOTS limit (509) can be a limiting factor for some
> configurations. In particular, when QEMU tries to start a Windows guest
> with Hyper-V SynIC enabled and e.g. 256 vCPUs the limit is hit as SynIC
> requires two pages per vCPU and the guest is free to pick any GFN for
> each of them, this fragments memslots as QEMU wants to have a separate
> memslot for each of these pages (which are supposed to act as 'overlay'
> pages).
> 
> Memory slots are allocated dynamically in KVM when added so the only real
> limitation is 'id_to_index' array which is 'short'. We don't have any
> KVM_MEM_SLOTS_NUM/KVM_USER_MEM_SLOTS-sized statically defined arrays.
> 
> We could've just raised the limit to e.g. '1021' (we have 3 private
> memslots on x86) and this should be enough for now as KVM_MAX_VCPUS is
> '288' but AFAIK there are plans to raise this limit as well.
> 

I have a patch series that reworks the whole memslot thing, bringing
performance improvements across the board.
Will post it in few days, together with a new mini benchmark set.

Maciej

Re: [PATCH RFC 0/4] KVM: x86: Drastically raise KVM_USER_MEM_SLOTS limit

[Vitaly Kuznetsov]

"Maciej S. Szmigiero" <maciej.szmigiero@oracle.com> writes:

> On 15.01.2021 14:18, Vitaly Kuznetsov wrote:
>> TL;DR: any particular reason why KVM_USER_MEM_SLOTS is so low?
>> 
>> Longer version:
>> 
>> Current KVM_USER_MEM_SLOTS limit (509) can be a limiting factor for some
>> configurations. In particular, when QEMU tries to start a Windows guest
>> with Hyper-V SynIC enabled and e.g. 256 vCPUs the limit is hit as SynIC
>> requires two pages per vCPU and the guest is free to pick any GFN for
>> each of them, this fragments memslots as QEMU wants to have a separate
>> memslot for each of these pages (which are supposed to act as 'overlay'
>> pages).
>> 
>> Memory slots are allocated dynamically in KVM when added so the only real
>> limitation is 'id_to_index' array which is 'short'. We don't have any
>> KVM_MEM_SLOTS_NUM/KVM_USER_MEM_SLOTS-sized statically defined arrays.
>> 
>> We could've just raised the limit to e.g. '1021' (we have 3 private
>> memslots on x86) and this should be enough for now as KVM_MAX_VCPUS is
>> '288' but AFAIK there are plans to raise this limit as well.
>> 
>
> I have a patch series that reworks the whole memslot thing, bringing
> performance improvements across the board.
> Will post it in few days, together with a new mini benchmark set.

I'm about to send a successor of this series. It will be implmenting
Sean's idea to make the maximum number of memslots a per-VM thing (and
also raise the default). Hope it won't interfere with your work!

-- 
Vitaly

Re: [PATCH RFC 0/4] KVM: x86: Drastically raise KVM_USER_MEM_SLOTS limit

[Maciej S. Szmigiero]

Hi Vitaly,

On 27.01.2021 18:55, Vitaly Kuznetsov wrote:
> "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com> writes:
> 
>> On 15.01.2021 14:18, Vitaly Kuznetsov wrote:
>>> TL;DR: any particular reason why KVM_USER_MEM_SLOTS is so low?
>>>
>>> Longer version:
>>>
>>> Current KVM_USER_MEM_SLOTS limit (509) can be a limiting factor for some
>>> configurations. In particular, when QEMU tries to start a Windows guest
>>> with Hyper-V SynIC enabled and e.g. 256 vCPUs the limit is hit as SynIC
>>> requires two pages per vCPU and the guest is free to pick any GFN for
>>> each of them, this fragments memslots as QEMU wants to have a separate
>>> memslot for each of these pages (which are supposed to act as 'overlay'
>>> pages).
>>>
>>> Memory slots are allocated dynamically in KVM when added so the only real
>>> limitation is 'id_to_index' array which is 'short'. We don't have any
>>> KVM_MEM_SLOTS_NUM/KVM_USER_MEM_SLOTS-sized statically defined arrays.
>>>
>>> We could've just raised the limit to e.g. '1021' (we have 3 private
>>> memslots on x86) and this should be enough for now as KVM_MAX_VCPUS is
>>> '288' but AFAIK there are plans to raise this limit as well.
>>>
>>
>> I have a patch series that reworks the whole memslot thing, bringing
>> performance improvements across the board.
>> Will post it in few days, together with a new mini benchmark set.
> 
> I'm about to send a successor of this series. It will be implmenting
> Sean's idea to make the maximum number of memslots a per-VM thing (and
> also raise the default). Hope it won't interfere with your work!

Thanks for your series and CC'ing me on it.

It looks like there should be no design conflicts, I will merely need to
rebase on top of it.

By the way I had to change a bit the KVM selftest framework memslot
handling for my stuff, too, since otherwise just adding 32k memslots
for a test would take almost forever.

Thanks,
Maciej

Re: [PATCH RFC 0/4] KVM: x86: Drastically raise KVM_USER_MEM_SLOTS limit

[Vitaly Kuznetsov]

"Maciej S. Szmigiero" <maciej.szmigiero@oracle.com> writes:

> Hi Vitaly,
>
> On 27.01.2021 18:55, Vitaly Kuznetsov wrote:
>> "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com> writes:
>> 
>>> On 15.01.2021 14:18, Vitaly Kuznetsov wrote:
>>>> TL;DR: any particular reason why KVM_USER_MEM_SLOTS is so low?
>>>>
>>>> Longer version:
>>>>
>>>> Current KVM_USER_MEM_SLOTS limit (509) can be a limiting factor for some
>>>> configurations. In particular, when QEMU tries to start a Windows guest
>>>> with Hyper-V SynIC enabled and e.g. 256 vCPUs the limit is hit as SynIC
>>>> requires two pages per vCPU and the guest is free to pick any GFN for
>>>> each of them, this fragments memslots as QEMU wants to have a separate
>>>> memslot for each of these pages (which are supposed to act as 'overlay'
>>>> pages).
>>>>
>>>> Memory slots are allocated dynamically in KVM when added so the only real
>>>> limitation is 'id_to_index' array which is 'short'. We don't have any
>>>> KVM_MEM_SLOTS_NUM/KVM_USER_MEM_SLOTS-sized statically defined arrays.
>>>>
>>>> We could've just raised the limit to e.g. '1021' (we have 3 private
>>>> memslots on x86) and this should be enough for now as KVM_MAX_VCPUS is
>>>> '288' but AFAIK there are plans to raise this limit as well.
>>>>
>>>
>>> I have a patch series that reworks the whole memslot thing, bringing
>>> performance improvements across the board.
>>> Will post it in few days, together with a new mini benchmark set.
>> 
>> I'm about to send a successor of this series. It will be implmenting
>> Sean's idea to make the maximum number of memslots a per-VM thing (and
>> also raise the default). Hope it won't interfere with your work!
>
> Thanks for your series and CC'ing me on it.
>
> It looks like there should be no design conflicts, I will merely need to
> rebase on top of it.
>
> By the way I had to change a bit the KVM selftest framework memslot
> handling for my stuff, too, since otherwise just adding 32k memslots
> for a test would take almost forever.
>

Yea, that's why PATCH5 of the new series increases the default timeout
from 45 seconds to 120.

-- 
Vitaly