| Message ID | 20200427141738.285217-1-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvmtool] kvm: Request VM specific limits instead of system-wide ones | expand |
Hi, On 4/27/20 3:17 PM, Marc Zyngier wrote: > On arm64, the maximum number of vcpus is constrained by the type > of interrupt controller that has been selected (GICv2 imposes a > limit of 8 vcpus, while GICv3 currently has a limit of 512). > > It is thus important to request this limit on the VM file descriptor > rather than on the one that corresponds to /dev/kvm, as the latter > is likely to return something that doesn't take the constraints into > account. > > Reported-by: Ard Biesheuvel <ardb@kernel.org> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > kvm.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/kvm.c b/kvm.c > index e327541..3d5173d 100644 > --- a/kvm.c > +++ b/kvm.c > @@ -406,7 +406,7 @@ int kvm__recommended_cpus(struct kvm *kvm) > { > int ret; > > - ret = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, KVM_CAP_NR_VCPUS); > + ret = ioctl(kvm->vm_fd, KVM_CHECK_EXTENSION, KVM_CAP_NR_VCPUS); > if (ret <= 0) > /* > * api.txt states that if KVM_CAP_NR_VCPUS does not exist, > @@ -421,7 +421,7 @@ int kvm__max_cpus(struct kvm *kvm) > { > int ret; > > - ret = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, KVM_CAP_MAX_VCPUS); > + ret = ioctl(kvm->vm_fd, KVM_CHECK_EXTENSION, KVM_CAP_MAX_VCPUS); > if (ret <= 0) > ret = kvm__recommended_cpus(kvm); > I've checked that gic__create comes before the call kvm__recommended_capus: gic__create is in core_init (called via kvm__init->kvm_arch_init), and kvm__recommended_cpus is in base_init (called via kvm__cpu_init -> kvm__{recommended,max}_cpus). The KVM api documentation states that KVM_CHECK_EXTENSION is available for the vm fd only if the system capability KVM_CAP_CHECK_EXTENSION_VM is present. kvmtool already has a function for checking extensions on the vm fd, it's called kvm__supports_vm_extension. Can we use that instead of doing the ioctl directly on the vm fd? Thanks, Alex
Hi, On 4/27/20 3:44 PM, Alexandru Elisei wrote: > Hi, > > On 4/27/20 3:17 PM, Marc Zyngier wrote: >> On arm64, the maximum number of vcpus is constrained by the type >> of interrupt controller that has been selected (GICv2 imposes a >> limit of 8 vcpus, while GICv3 currently has a limit of 512). >> >> It is thus important to request this limit on the VM file descriptor >> rather than on the one that corresponds to /dev/kvm, as the latter >> is likely to return something that doesn't take the constraints into >> account. >> >> Reported-by: Ard Biesheuvel <ardb@kernel.org> >> Signed-off-by: Marc Zyngier <maz@kernel.org> >> --- >> kvm.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/kvm.c b/kvm.c >> index e327541..3d5173d 100644 >> --- a/kvm.c >> +++ b/kvm.c >> @@ -406,7 +406,7 @@ int kvm__recommended_cpus(struct kvm *kvm) >> { >> int ret; >> >> - ret = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, KVM_CAP_NR_VCPUS); >> + ret = ioctl(kvm->vm_fd, KVM_CHECK_EXTENSION, KVM_CAP_NR_VCPUS); >> if (ret <= 0) >> /* >> * api.txt states that if KVM_CAP_NR_VCPUS does not exist, >> @@ -421,7 +421,7 @@ int kvm__max_cpus(struct kvm *kvm) >> { >> int ret; >> >> - ret = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, KVM_CAP_MAX_VCPUS); >> + ret = ioctl(kvm->vm_fd, KVM_CHECK_EXTENSION, KVM_CAP_MAX_VCPUS); >> if (ret <= 0) >> ret = kvm__recommended_cpus(kvm); >> > I've checked that gic__create comes before the call kvm__recommended_capus: > gic__create is in core_init (called via kvm__init->kvm_arch_init), and > kvm__recommended_cpus is in base_init (called via kvm__cpu_init -> > kvm__{recommended,max}_cpus). > > The KVM api documentation states that KVM_CHECK_EXTENSION is available for the vm > fd only if the system capability KVM_CAP_CHECK_EXTENSION_VM is present. kvmtool > already has a function for checking extensions on the vm fd, it's called > kvm__supports_vm_extension. Can we use that instead of doing the ioctl directly on > the vm fd? Scratch that, kvm__supports_vm_extension returns a bool, not an int. How about we write kvm__check_vm_extension that returns an int, and kvm__supports_vm_extension calls it? > > Thanks, > Alex
On 27/04/2020 15:17, Marc Zyngier wrote: Hi, > On arm64, the maximum number of vcpus is constrained by the type > of interrupt controller that has been selected (GICv2 imposes a > limit of 8 vcpus, while GICv3 currently has a limit of 512). > > It is thus important to request this limit on the VM file descriptor > rather than on the one that corresponds to /dev/kvm, as the latter > is likely to return something that doesn't take the constraints into > account. That sounds reasonable, but I fail to find any distinction in the kernel code. We don't make any difference between the VM or the system FD in the ioctl handler for those two extensions. For arm64 we always return max. 512 (max VCPUs on GICv3), and number of online host cores for the recommended value. For arm there was a distinction between GICv3 support compiled in or not, but otherwise the same constant values returned. Quickly tested on Juno and N1SDP, the ioctls return the same expected values, regardless of sys_fd vs vm_fd. So what am I missing here? Is this for some older or even newer kernels? Cheers, Andre. > > Reported-by: Ard Biesheuvel <ardb@kernel.org> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > kvm.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/kvm.c b/kvm.c > index e327541..3d5173d 100644 > --- a/kvm.c > +++ b/kvm.c > @@ -406,7 +406,7 @@ int kvm__recommended_cpus(struct kvm *kvm) > { > int ret; > > - ret = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, KVM_CAP_NR_VCPUS); > + ret = ioctl(kvm->vm_fd, KVM_CHECK_EXTENSION, KVM_CAP_NR_VCPUS); > if (ret <= 0) > /* > * api.txt states that if KVM_CAP_NR_VCPUS does not exist, > @@ -421,7 +421,7 @@ int kvm__max_cpus(struct kvm *kvm) > { > int ret; > > - ret = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, KVM_CAP_MAX_VCPUS); > + ret = ioctl(kvm->vm_fd, KVM_CHECK_EXTENSION, KVM_CAP_MAX_VCPUS); > if (ret <= 0) > ret = kvm__recommended_cpus(kvm); > >
On 2020-04-27 16:37, André Przywara wrote: > On 27/04/2020 15:17, Marc Zyngier wrote: > Hi, > >> On arm64, the maximum number of vcpus is constrained by the type >> of interrupt controller that has been selected (GICv2 imposes a >> limit of 8 vcpus, while GICv3 currently has a limit of 512). >> >> It is thus important to request this limit on the VM file descriptor >> rather than on the one that corresponds to /dev/kvm, as the latter >> is likely to return something that doesn't take the constraints into >> account. > > That sounds reasonable, but I fail to find any distinction in the > kernel > code. We don't make any difference between the VM or the system FD in > the ioctl handler for those two extensions. For arm64 we always return > max. 512 (max VCPUs on GICv3), and number of online host cores for the > recommended value. For arm there was a distinction between GICv3 > support > compiled in or not, but otherwise the same constant values returned. > Quickly tested on Juno and N1SDP, the ioctls return the same expected > values, regardless of sys_fd vs vm_fd. > > So what am I missing here? Is this for some older or even newer > kernels? You're missing this: https://lore.kernel.org/kvm/20200427141507.284985-1-maz@kernel.org/ which adds the missing bits to the kernel. Thanks, M.
On Mon, 27 Apr 2020 16:00:58 +0100 Alexandru Elisei <alexandru.elisei@arm.com> wrote: > Hi, > > On 4/27/20 3:44 PM, Alexandru Elisei wrote: > > Hi, > > > > On 4/27/20 3:17 PM, Marc Zyngier wrote: > >> On arm64, the maximum number of vcpus is constrained by the type > >> of interrupt controller that has been selected (GICv2 imposes a > >> limit of 8 vcpus, while GICv3 currently has a limit of 512). > >> > >> It is thus important to request this limit on the VM file descriptor > >> rather than on the one that corresponds to /dev/kvm, as the latter > >> is likely to return something that doesn't take the constraints into > >> account. > >> > >> Reported-by: Ard Biesheuvel <ardb@kernel.org> > >> Signed-off-by: Marc Zyngier <maz@kernel.org> > >> --- > >> kvm.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/kvm.c b/kvm.c > >> index e327541..3d5173d 100644 > >> --- a/kvm.c > >> +++ b/kvm.c > >> @@ -406,7 +406,7 @@ int kvm__recommended_cpus(struct kvm *kvm) > >> { > >> int ret; > >> > >> - ret = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, KVM_CAP_NR_VCPUS); > >> + ret = ioctl(kvm->vm_fd, KVM_CHECK_EXTENSION, KVM_CAP_NR_VCPUS); > >> if (ret <= 0) > >> /* > >> * api.txt states that if KVM_CAP_NR_VCPUS does not exist, > >> @@ -421,7 +421,7 @@ int kvm__max_cpus(struct kvm *kvm) > >> { > >> int ret; > >> > >> - ret = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, KVM_CAP_MAX_VCPUS); > >> + ret = ioctl(kvm->vm_fd, KVM_CHECK_EXTENSION, KVM_CAP_MAX_VCPUS); > >> if (ret <= 0) > >> ret = kvm__recommended_cpus(kvm); > >> > > I've checked that gic__create comes before the call kvm__recommended_capus: > > gic__create is in core_init (called via kvm__init->kvm_arch_init), and > > kvm__recommended_cpus is in base_init (called via kvm__cpu_init -> > > kvm__{recommended,max}_cpus). > > > > The KVM api documentation states that KVM_CHECK_EXTENSION is available for the vm > > fd only if the system capability KVM_CAP_CHECK_EXTENSION_VM is present. kvmtool > > already has a function for checking extensions on the vm fd, it's called > > kvm__supports_vm_extension. Can we use that instead of doing the ioctl directly on > > the vm fd? > > Scratch that, kvm__supports_vm_extension returns a bool, not an int. > How about we write kvm__check_vm_extension that returns an int, and > kvm__supports_vm_extension calls it? That, or we just change the return type for kvm__supports_vm_extension, and hack the only places that uses it so far (the GIC code) to detect the error. Thanks, M.
Hi, On 4/27/20 6:33 PM, Marc Zyngier wrote: > On Mon, 27 Apr 2020 16:00:58 +0100 > Alexandru Elisei <alexandru.elisei@arm.com> wrote: > >> Hi, >> >> On 4/27/20 3:44 PM, Alexandru Elisei wrote: >>> Hi, >>> >>> On 4/27/20 3:17 PM, Marc Zyngier wrote: >>>> On arm64, the maximum number of vcpus is constrained by the type >>>> of interrupt controller that has been selected (GICv2 imposes a >>>> limit of 8 vcpus, while GICv3 currently has a limit of 512). >>>> >>>> It is thus important to request this limit on the VM file descriptor >>>> rather than on the one that corresponds to /dev/kvm, as the latter >>>> is likely to return something that doesn't take the constraints into >>>> account. >>>> >>>> Reported-by: Ard Biesheuvel <ardb@kernel.org> >>>> Signed-off-by: Marc Zyngier <maz@kernel.org> >>>> --- >>>> kvm.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/kvm.c b/kvm.c >>>> index e327541..3d5173d 100644 >>>> --- a/kvm.c >>>> +++ b/kvm.c >>>> @@ -406,7 +406,7 @@ int kvm__recommended_cpus(struct kvm *kvm) >>>> { >>>> int ret; >>>> >>>> - ret = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, KVM_CAP_NR_VCPUS); >>>> + ret = ioctl(kvm->vm_fd, KVM_CHECK_EXTENSION, KVM_CAP_NR_VCPUS); >>>> if (ret <= 0) >>>> /* >>>> * api.txt states that if KVM_CAP_NR_VCPUS does not exist, >>>> @@ -421,7 +421,7 @@ int kvm__max_cpus(struct kvm *kvm) >>>> { >>>> int ret; >>>> >>>> - ret = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, KVM_CAP_MAX_VCPUS); >>>> + ret = ioctl(kvm->vm_fd, KVM_CHECK_EXTENSION, KVM_CAP_MAX_VCPUS); >>>> if (ret <= 0) >>>> ret = kvm__recommended_cpus(kvm); >>>> >>> I've checked that gic__create comes before the call kvm__recommended_capus: >>> gic__create is in core_init (called via kvm__init->kvm_arch_init), and >>> kvm__recommended_cpus is in base_init (called via kvm__cpu_init -> >>> kvm__{recommended,max}_cpus). >>> >>> The KVM api documentation states that KVM_CHECK_EXTENSION is available for the vm >>> fd only if the system capability KVM_CAP_CHECK_EXTENSION_VM is present. kvmtool >>> already has a function for checking extensions on the vm fd, it's called >>> kvm__supports_vm_extension. Can we use that instead of doing the ioctl directly on >>> the vm fd? >> Scratch that, kvm__supports_vm_extension returns a bool, not an int. >> How about we write kvm__check_vm_extension that returns an int, and >> kvm__supports_vm_extension calls it? > That, or we just change the return type for kvm__supports_vm_extension, > and hack the only places that uses it so far (the GIC code) to detect > the error. Yep, whatever you prefer. Thanks, Alex
diff --git a/kvm.c b/kvm.c index e327541..3d5173d 100644 --- a/kvm.c +++ b/kvm.c @@ -406,7 +406,7 @@ int kvm__recommended_cpus(struct kvm *kvm) { int ret; - ret = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, KVM_CAP_NR_VCPUS); + ret = ioctl(kvm->vm_fd, KVM_CHECK_EXTENSION, KVM_CAP_NR_VCPUS); if (ret <= 0) /* * api.txt states that if KVM_CAP_NR_VCPUS does not exist, @@ -421,7 +421,7 @@ int kvm__max_cpus(struct kvm *kvm) { int ret; - ret = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, KVM_CAP_MAX_VCPUS); + ret = ioctl(kvm->vm_fd, KVM_CHECK_EXTENSION, KVM_CAP_MAX_VCPUS); if (ret <= 0) ret = kvm__recommended_cpus(kvm);
On arm64, the maximum number of vcpus is constrained by the type of interrupt controller that has been selected (GICv2 imposes a limit of 8 vcpus, while GICv3 currently has a limit of 512). It is thus important to request this limit on the VM file descriptor rather than on the one that corresponds to /dev/kvm, as the latter is likely to return something that doesn't take the constraints into account. Reported-by: Ard Biesheuvel <ardb@kernel.org> Signed-off-by: Marc Zyngier <maz@kernel.org> --- kvm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)