| Message ID | 20211116141054.17800-4-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/kvm: add boot parameters for max vcpu configs | expand |
On Tue, Nov 16, 2021, Juergen Gross wrote: > When emulating Hyperv the theoretical maximum of vcpus supported is > 4096, as this is the architectural limit for sending IPIs via the PV > interface. > > For restricting the actual supported number of vcpus for that case > introduce another define KVM_MAX_HYPERV_VCPUS and set it to 1024, like > today's KVM_MAX_VCPUS. Make both values unsigned ones as this will be > needed later. > > The actual number of supported vcpus for Hyperv emulation will be the > lower value of both defines. > > This is a preparation for a future boot parameter support of the max > number of vcpus for a KVM guest. > > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > V3: > - new patch > --- > arch/x86/include/asm/kvm_host.h | 3 ++- > arch/x86/kvm/hyperv.c | 15 ++++++++------- > 2 files changed, 10 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 886930ec8264..8ea03ff01c45 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -38,7 +38,8 @@ > > #define __KVM_HAVE_ARCH_VCPU_DEBUGFS > > -#define KVM_MAX_VCPUS 1024 > +#define KVM_MAX_VCPUS 1024U > +#define KVM_MAX_HYPERV_VCPUS 1024U I don't see any reason to put this in kvm_host.h, it should never be used outside of hyperv.c. > #define KVM_MAX_VCPU_IDS kvm_max_vcpu_ids() > /* memory slots that are not exposed to userspace */ > #define KVM_PRIVATE_MEM_SLOTS 3 > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > index 4a555f32885a..c0fa837121f1 100644 > --- a/arch/x86/kvm/hyperv.c > +++ b/arch/x86/kvm/hyperv.c > @@ -41,7 +41,7 @@ > /* "Hv#1" signature */ > #define HYPERV_CPUID_SIGNATURE_EAX 0x31237648 > > -#define KVM_HV_MAX_SPARSE_VCPU_SET_BITS DIV_ROUND_UP(KVM_MAX_VCPUS, 64) > +#define KVM_HV_MAX_SPARSE_VCPU_SET_BITS DIV_ROUND_UP(KVM_MAX_HYPERV_VCPUS, 64) > > static void stimer_mark_pending(struct kvm_vcpu_hv_stimer *stimer, > bool vcpu_kick); > @@ -166,7 +166,7 @@ static struct kvm_vcpu *get_vcpu_by_vpidx(struct kvm *kvm, u32 vpidx) > struct kvm_vcpu *vcpu = NULL; > int i; > > - if (vpidx >= KVM_MAX_VCPUS) > + if (vpidx >= min(KVM_MAX_VCPUS, KVM_MAX_HYPERV_VCPUS)) IMO, this is conceptually wrong. KVM should refuse to allow Hyper-V to be enabled if the max number of vCPUs exceeds what can be supported, or should refuse to create the vCPUs. I agree it makes sense to add a Hyper-V specific limit, since there are Hyper-V structures that have a hard limit, but detection of violations should be a BUILD_BUG_ON, not a silent failure at runtime.
On 17.11.21 21:50, Sean Christopherson wrote: > On Tue, Nov 16, 2021, Juergen Gross wrote: >> When emulating Hyperv the theoretical maximum of vcpus supported is >> 4096, as this is the architectural limit for sending IPIs via the PV >> interface. >> >> For restricting the actual supported number of vcpus for that case >> introduce another define KVM_MAX_HYPERV_VCPUS and set it to 1024, like >> today's KVM_MAX_VCPUS. Make both values unsigned ones as this will be >> needed later. >> >> The actual number of supported vcpus for Hyperv emulation will be the >> lower value of both defines. >> >> This is a preparation for a future boot parameter support of the max >> number of vcpus for a KVM guest. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> V3: >> - new patch >> --- >> arch/x86/include/asm/kvm_host.h | 3 ++- >> arch/x86/kvm/hyperv.c | 15 ++++++++------- >> 2 files changed, 10 insertions(+), 8 deletions(-) >> >> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h >> index 886930ec8264..8ea03ff01c45 100644 >> --- a/arch/x86/include/asm/kvm_host.h >> +++ b/arch/x86/include/asm/kvm_host.h >> @@ -38,7 +38,8 @@ >> >> #define __KVM_HAVE_ARCH_VCPU_DEBUGFS >> >> -#define KVM_MAX_VCPUS 1024 >> +#define KVM_MAX_VCPUS 1024U >> +#define KVM_MAX_HYPERV_VCPUS 1024U > > I don't see any reason to put this in kvm_host.h, it should never be used outside > of hyperv.c. Okay, fine with me. > >> #define KVM_MAX_VCPU_IDS kvm_max_vcpu_ids() >> /* memory slots that are not exposed to userspace */ >> #define KVM_PRIVATE_MEM_SLOTS 3 >> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c >> index 4a555f32885a..c0fa837121f1 100644 >> --- a/arch/x86/kvm/hyperv.c >> +++ b/arch/x86/kvm/hyperv.c >> @@ -41,7 +41,7 @@ >> /* "Hv#1" signature */ >> #define HYPERV_CPUID_SIGNATURE_EAX 0x31237648 >> >> -#define KVM_HV_MAX_SPARSE_VCPU_SET_BITS DIV_ROUND_UP(KVM_MAX_VCPUS, 64) >> +#define KVM_HV_MAX_SPARSE_VCPU_SET_BITS DIV_ROUND_UP(KVM_MAX_HYPERV_VCPUS, 64) >> >> static void stimer_mark_pending(struct kvm_vcpu_hv_stimer *stimer, >> bool vcpu_kick); >> @@ -166,7 +166,7 @@ static struct kvm_vcpu *get_vcpu_by_vpidx(struct kvm *kvm, u32 vpidx) >> struct kvm_vcpu *vcpu = NULL; >> int i; >> >> - if (vpidx >= KVM_MAX_VCPUS) >> + if (vpidx >= min(KVM_MAX_VCPUS, KVM_MAX_HYPERV_VCPUS)) > > IMO, this is conceptually wrong. KVM should refuse to allow Hyper-V to be enabled > if the max number of vCPUs exceeds what can be supported, or should refuse to create TBH, I wasn't sure where to put this test. Is there a guaranteed sequence of ioctl()s regarding vcpu creation (or setting the max number of vcpus) and the Hyper-V enabling? > the vCPUs. I agree it makes sense to add a Hyper-V specific limit, since there are > Hyper-V structures that have a hard limit, but detection of violations should be a > BUILD_BUG_ON, not a silent failure at runtime. > A BUILD_BUG_ON won't be possible with KVM_MAX_VCPUS being selecteble via boot parameter. Juergen
On Thu, Nov 18, 2021, Juergen Gross wrote: > On 17.11.21 21:50, Sean Christopherson wrote: > > > @@ -166,7 +166,7 @@ static struct kvm_vcpu *get_vcpu_by_vpidx(struct kvm *kvm, u32 vpidx) > > > struct kvm_vcpu *vcpu = NULL; > > > int i; > > > - if (vpidx >= KVM_MAX_VCPUS) > > > + if (vpidx >= min(KVM_MAX_VCPUS, KVM_MAX_HYPERV_VCPUS)) > > > > IMO, this is conceptually wrong. KVM should refuse to allow Hyper-V to be enabled > > if the max number of vCPUs exceeds what can be supported, or should refuse to create > > TBH, I wasn't sure where to put this test. Is there a guaranteed > sequence of ioctl()s regarding vcpu creation (or setting the max > number of vcpus) and the Hyper-V enabling? For better or worse (mostly worse), like all other things CPUID, Hyper-V is a per-vCPU knob. If KVM can't detect the impossible condition at compile time, kvm_check_cpuid() is probably the right place to prevent enabling Hyper-V on an unreachable vCPU. > > the vCPUs. I agree it makes sense to add a Hyper-V specific limit, since there are > > Hyper-V structures that have a hard limit, but detection of violations should be a > > BUILD_BUG_ON, not a silent failure at runtime. > > > > A BUILD_BUG_ON won't be possible with KVM_MAX_VCPUS being selecteble via > boot parameter. I was thinking that there would still be a KVM-defined max that would cap whatever comes in from userspace.
On 18.11.21 15:49, Sean Christopherson wrote: > On Thu, Nov 18, 2021, Juergen Gross wrote: >> On 17.11.21 21:50, Sean Christopherson wrote: >>>> @@ -166,7 +166,7 @@ static struct kvm_vcpu *get_vcpu_by_vpidx(struct kvm *kvm, u32 vpidx) >>>> struct kvm_vcpu *vcpu = NULL; >>>> int i; >>>> - if (vpidx >= KVM_MAX_VCPUS) >>>> + if (vpidx >= min(KVM_MAX_VCPUS, KVM_MAX_HYPERV_VCPUS)) >>> >>> IMO, this is conceptually wrong. KVM should refuse to allow Hyper-V to be enabled >>> if the max number of vCPUs exceeds what can be supported, or should refuse to create >> >> TBH, I wasn't sure where to put this test. Is there a guaranteed >> sequence of ioctl()s regarding vcpu creation (or setting the max >> number of vcpus) and the Hyper-V enabling? > > For better or worse (mostly worse), like all other things CPUID, Hyper-V is a per-vCPU > knob. If KVM can't detect the impossible condition at compile time, kvm_check_cpuid() > is probably the right place to prevent enabling Hyper-V on an unreachable vCPU. With HYPERV_CPUID_IMPLEMENT_LIMITS already returning the supported number of vcpus for the Hyper-V case I'm not sure there is really more needed. The problem I'm seeing is that the only thing I can do is to let kvm_get_hv_cpuid() not adding the Hyper-V cpuid leaves for vcpus > 64. I can't return a failure, because that would probably let vcpu creation fail. And this is something we don't want, as kvm_get_hv_cpuid() is called even in the case the guest doesn't plan to use Hyper-V extensions. > >>> the vCPUs. I agree it makes sense to add a Hyper-V specific limit, since there are >>> Hyper-V structures that have a hard limit, but detection of violations should be a >>> BUILD_BUG_ON, not a silent failure at runtime. >>> >> >> A BUILD_BUG_ON won't be possible with KVM_MAX_VCPUS being selecteble via >> boot parameter. > > I was thinking that there would still be a KVM-defined max that would cap whatever > comes in from userspace. > See my answers to you your other responses. Juergen
On Thu, Nov 18, 2021, Juergen Gross wrote: > On 18.11.21 15:49, Sean Christopherson wrote: > > On Thu, Nov 18, 2021, Juergen Gross wrote: > > > On 17.11.21 21:50, Sean Christopherson wrote: > > > > > @@ -166,7 +166,7 @@ static struct kvm_vcpu *get_vcpu_by_vpidx(struct kvm *kvm, u32 vpidx) > > > > > struct kvm_vcpu *vcpu = NULL; > > > > > int i; > > > > > - if (vpidx >= KVM_MAX_VCPUS) > > > > > + if (vpidx >= min(KVM_MAX_VCPUS, KVM_MAX_HYPERV_VCPUS)) > > > > > > > > IMO, this is conceptually wrong. KVM should refuse to allow Hyper-V to be enabled > > > > if the max number of vCPUs exceeds what can be supported, or should refuse to create > > > > > > TBH, I wasn't sure where to put this test. Is there a guaranteed > > > sequence of ioctl()s regarding vcpu creation (or setting the max > > > number of vcpus) and the Hyper-V enabling? > > > > For better or worse (mostly worse), like all other things CPUID, Hyper-V is a per-vCPU > > knob. If KVM can't detect the impossible condition at compile time, kvm_check_cpuid() > > is probably the right place to prevent enabling Hyper-V on an unreachable vCPU. > > With HYPERV_CPUID_IMPLEMENT_LIMITS already returning the > supported number of vcpus for the Hyper-V case I'm not sure > there is really more needed. Yep, that'll do nicely. > The problem I'm seeing is that the only thing I can do is to > let kvm_get_hv_cpuid() not adding the Hyper-V cpuid leaves for > vcpus > 64. I can't return a failure, because that would > probably let vcpu creation fail. And this is something we don't > want, as kvm_get_hv_cpuid() is called even in the case the guest > doesn't plan to use Hyper-V extensions. Argh, that thing is annoying. My vote is still to reject KVM_SET_CPUID{2} if userspace attempts to enable Hyper-V for a vCPU when the max number of vCPUs exceeds HYPERV_CPUID_IMPLEMENT_LIMITS. If userspace parrots back KVM_GET_SUPPORTED_CPUID, it will specify KVM as the hypervisor, i.e. enabling Hyper-V requires deliberate action from userspace. The non-vCPU version of KVM_GET_SUPPORTED_HV_CPUID is not an issue, e.g. the generic KVM_GET_SUPPORTED_CPUID also reports features that become unsupported if dependent CPUID features are not enabled by userspace. The discrepancy with the per-vCPU variant of kvm_get_hv_cpuid() would be unfortunate, but IMO that ship sailed when the per-vCPU variant was added by commit 2bc39970e932 ("x86/kvm/hyper-v: Introduce KVM_GET_SUPPORTED_HV_CPUID"). We can't retroactively yank that code out, but I don't think we should be overly concerned with keeping it 100% accurate. IMO it's perfectly fine for KVM to define the output of KVM_GET_SUPPORTED_HV_CPUID as being garbage if the vCPU cannot possibly support Hyper-V enlightments. That situation isn't possible today, so there's no backwards compatibility to worry about.
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 886930ec8264..8ea03ff01c45 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -38,7 +38,8 @@ #define __KVM_HAVE_ARCH_VCPU_DEBUGFS -#define KVM_MAX_VCPUS 1024 +#define KVM_MAX_VCPUS 1024U +#define KVM_MAX_HYPERV_VCPUS 1024U #define KVM_MAX_VCPU_IDS kvm_max_vcpu_ids() /* memory slots that are not exposed to userspace */ #define KVM_PRIVATE_MEM_SLOTS 3 diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c index 4a555f32885a..c0fa837121f1 100644 --- a/arch/x86/kvm/hyperv.c +++ b/arch/x86/kvm/hyperv.c @@ -41,7 +41,7 @@ /* "Hv#1" signature */ #define HYPERV_CPUID_SIGNATURE_EAX 0x31237648 -#define KVM_HV_MAX_SPARSE_VCPU_SET_BITS DIV_ROUND_UP(KVM_MAX_VCPUS, 64) +#define KVM_HV_MAX_SPARSE_VCPU_SET_BITS DIV_ROUND_UP(KVM_MAX_HYPERV_VCPUS, 64) static void stimer_mark_pending(struct kvm_vcpu_hv_stimer *stimer, bool vcpu_kick); @@ -166,7 +166,7 @@ static struct kvm_vcpu *get_vcpu_by_vpidx(struct kvm *kvm, u32 vpidx) struct kvm_vcpu *vcpu = NULL; int i; - if (vpidx >= KVM_MAX_VCPUS) + if (vpidx >= min(KVM_MAX_VCPUS, KVM_MAX_HYPERV_VCPUS)) return NULL; vcpu = kvm_get_vcpu(kvm, vpidx); @@ -1446,7 +1446,8 @@ static int kvm_hv_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data, bool host) struct kvm_hv *hv = to_kvm_hv(vcpu->kvm); u32 new_vp_index = (u32)data; - if (!host || new_vp_index >= KVM_MAX_VCPUS) + if (!host || + new_vp_index >= min(KVM_MAX_VCPUS, KVM_MAX_HYPERV_VCPUS)) return 1; if (new_vp_index == hv_vcpu->vp_index) @@ -1729,7 +1730,7 @@ static __always_inline unsigned long *sparse_set_to_vcpu_mask( return (unsigned long *)vp_bitmap; } - bitmap_zero(vcpu_bitmap, KVM_MAX_VCPUS); + bitmap_zero(vcpu_bitmap, min(KVM_MAX_VCPUS, KVM_MAX_HYPERV_VCPUS)); kvm_for_each_vcpu(i, vcpu, kvm) { if (test_bit(kvm_hv_get_vpindex(vcpu), (unsigned long *)vp_bitmap)) __set_bit(i, vcpu_bitmap); @@ -1757,7 +1758,7 @@ static u64 kvm_hv_flush_tlb(struct kvm_vcpu *vcpu, struct kvm_hv_hcall *hc, bool struct hv_tlb_flush_ex flush_ex; struct hv_tlb_flush flush; u64 vp_bitmap[KVM_HV_MAX_SPARSE_VCPU_SET_BITS]; - DECLARE_BITMAP(vcpu_bitmap, KVM_MAX_VCPUS); + DECLARE_BITMAP(vcpu_bitmap, KVM_MAX_HYPERV_VCPUS); unsigned long *vcpu_mask; u64 valid_bank_mask; u64 sparse_banks[64]; @@ -1880,7 +1881,7 @@ static u64 kvm_hv_send_ipi(struct kvm_vcpu *vcpu, struct kvm_hv_hcall *hc, bool struct hv_send_ipi_ex send_ipi_ex; struct hv_send_ipi send_ipi; u64 vp_bitmap[KVM_HV_MAX_SPARSE_VCPU_SET_BITS]; - DECLARE_BITMAP(vcpu_bitmap, KVM_MAX_VCPUS); + DECLARE_BITMAP(vcpu_bitmap, KVM_MAX_HYPERV_VCPUS); unsigned long *vcpu_mask; unsigned long valid_bank_mask; u64 sparse_banks[64]; @@ -2505,7 +2506,7 @@ int kvm_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid, case HYPERV_CPUID_IMPLEMENT_LIMITS: /* Maximum number of virtual processors */ - ent->eax = KVM_MAX_VCPUS; + ent->eax = min(KVM_MAX_VCPUS, KVM_MAX_HYPERV_VCPUS); /* * Maximum number of logical processors, matches * HyperV 2016.
When emulating Hyperv the theoretical maximum of vcpus supported is 4096, as this is the architectural limit for sending IPIs via the PV interface. For restricting the actual supported number of vcpus for that case introduce another define KVM_MAX_HYPERV_VCPUS and set it to 1024, like today's KVM_MAX_VCPUS. Make both values unsigned ones as this will be needed later. The actual number of supported vcpus for Hyperv emulation will be the lower value of both defines. This is a preparation for a future boot parameter support of the max number of vcpus for a KVM guest. Signed-off-by: Juergen Gross <jgross@suse.com> --- V3: - new patch --- arch/x86/include/asm/kvm_host.h | 3 ++- arch/x86/kvm/hyperv.c | 15 ++++++++------- 2 files changed, 10 insertions(+), 8 deletions(-)