[PATCH][kvmtool] kvm: Request VM specific limits instead of system-wide ones

[PATCH][kvmtool] kvm: Request VM specific limits instead of system-wide ones

[Marc Zyngier]

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);
 
-- 
2.26.2

Re: [PATCH][kvmtool] kvm: Request VM specific limits instead of system-wide ones

[Alexandru Elisei]

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

Re: [PATCH][kvmtool] kvm: Request VM specific limits instead of system-wide ones

[Alexandru Elisei]

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

Re: [PATCH][kvmtool] kvm: Request VM specific limits instead of system-wide ones

[André Przywara]

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);
>  
> 

Re: [PATCH][kvmtool] kvm: Request VM specific limits instead of system-wide ones

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH][kvmtool] kvm: Request VM specific limits instead of system-wide ones

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH][kvmtool] kvm: Request VM specific limits instead of system-wide ones

[Alexandru Elisei]

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