Patchwork [07/28] kvm tools: Move 'kvm__recommended_cpus' to arch-specific code

login
register
mail settings
Submitter Matt Evans
Date Dec. 7, 2011, 7:19 a.m.
Message ID <4EDF138E.6040703@ozlabs.org>
Download mbox | patch
Permalink /patch/129909/
State New
Headers show

Comments

Matt Evans - Dec. 7, 2011, 7:19 a.m.
On 07/12/11 17:34, Sasha Levin wrote:
> On Wed, 2011-12-07 at 17:17 +1100, Matt Evans wrote:
>> On 06/12/11 19:20, Sasha Levin wrote:
>>> Why is it getting moved out of generic code?
>>>
>>> This is used to determine the maximum amount of vcpus supported by the
>>> host for a single guest, and as far as I know KVM_CAP_NR_VCPUS and
>>> KVM_CAP_MAX_VCPUS are not arch specific.
>>
>> I checked api.txt and you're right, it isn't arch-specific.  I assumed it was,
>> because PPC KVM doesn't support it ;-) I've dropped this patch and in its place
>> implemented the api.txt suggestion of "if KVM_CAP_NR_VCPUS fails, use 4" instead
>> of die(); you'll see that when I repost.
>>
>> This will have the effect of PPC being limited to 4 CPUs until the kernel
>> supports that CAP.  (I'll see about this part too.)
> 
> I went to look at which limitation PPC places on amount of vcpus in
> guest, and saw this in kvmppc_core_vcpu_create() in the book3s code:
> 
> 	vcpu = kvmppc_core_vcpu_create(kvm, id);
> 	vcpu->arch.wqp = &vcpu->wq;
> 	if (!IS_ERR(vcpu))
> 		kvmppc_create_vcpu_debugfs(vcpu, id);
> 
> This is wrong, right? The VCPU is dereferenced before actually checking
> that it's not an error.

Yeah, that's b0rk.  Alex, a patch below. :)


Cheers,


Matt

---
Subject: [PATCH] KVM: PPC: Fix vcpu_create dereference before validity check.


Signed-off-by: Matt Evans <matt@ozlabs.org>
---
 arch/powerpc/kvm/powerpc.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
Alexander Graf - Dec. 7, 2011, 7:24 a.m.
On 07.12.2011, at 08:19, Matt Evans <matt@ozlabs.org> wrote:

> On 07/12/11 17:34, Sasha Levin wrote:
>> On Wed, 2011-12-07 at 17:17 +1100, Matt Evans wrote:
>>> On 06/12/11 19:20, Sasha Levin wrote:
>>>> Why is it getting moved out of generic code?
>>>> 
>>>> This is used to determine the maximum amount of vcpus supported by the
>>>> host for a single guest, and as far as I know KVM_CAP_NR_VCPUS and
>>>> KVM_CAP_MAX_VCPUS are not arch specific.
>>> 
>>> I checked api.txt and you're right, it isn't arch-specific.  I assumed it was,
>>> because PPC KVM doesn't support it ;-) I've dropped this patch and in its place
>>> implemented the api.txt suggestion of "if KVM_CAP_NR_VCPUS fails, use 4" instead
>>> of die(); you'll see that when I repost.
>>> 
>>> This will have the effect of PPC being limited to 4 CPUs until the kernel
>>> supports that CAP.  (I'll see about this part too.)
>> 
>> I went to look at which limitation PPC places on amount of vcpus in
>> guest, and saw this in kvmppc_core_vcpu_create() in the book3s code:
>> 
>>    vcpu = kvmppc_core_vcpu_create(kvm, id);
>>    vcpu->arch.wqp = &vcpu->wq;
>>    if (!IS_ERR(vcpu))
>>        kvmppc_create_vcpu_debugfs(vcpu, id);
>> 
>> This is wrong, right? The VCPU is dereferenced before actually checking
>> that it's not an error.
> 
> Yeah, that's b0rk.  Alex, a patch below. :)

Thanks :). Will apply asap but don't have a real keyboard today :).

I suppose this is stable material?

Alex

> 
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matt Evans - Dec. 7, 2011, 7:28 a.m.
On 07/12/11 18:24, Alexander Graf wrote:
> 
> On 07.12.2011, at 08:19, Matt Evans <matt@ozlabs.org> wrote:
> 
>> On 07/12/11 17:34, Sasha Levin wrote:
>>> On Wed, 2011-12-07 at 17:17 +1100, Matt Evans wrote:
>>>> On 06/12/11 19:20, Sasha Levin wrote:
>>>>> Why is it getting moved out of generic code?
>>>>>
>>>>> This is used to determine the maximum amount of vcpus supported by the
>>>>> host for a single guest, and as far as I know KVM_CAP_NR_VCPUS and
>>>>> KVM_CAP_MAX_VCPUS are not arch specific.
>>>>
>>>> I checked api.txt and you're right, it isn't arch-specific.  I assumed it was,
>>>> because PPC KVM doesn't support it ;-) I've dropped this patch and in its place
>>>> implemented the api.txt suggestion of "if KVM_CAP_NR_VCPUS fails, use 4" instead
>>>> of die(); you'll see that when I repost.
>>>>
>>>> This will have the effect of PPC being limited to 4 CPUs until the kernel
>>>> supports that CAP.  (I'll see about this part too.)
>>>
>>> I went to look at which limitation PPC places on amount of vcpus in
>>> guest, and saw this in kvmppc_core_vcpu_create() in the book3s code:
>>>
>>>    vcpu = kvmppc_core_vcpu_create(kvm, id);
>>>    vcpu->arch.wqp = &vcpu->wq;
>>>    if (!IS_ERR(vcpu))
>>>        kvmppc_create_vcpu_debugfs(vcpu, id);
>>>
>>> This is wrong, right? The VCPU is dereferenced before actually checking
>>> that it's not an error.
>>
>> Yeah, that's b0rk.  Alex, a patch below. :)
> 
> Thanks :). Will apply asap but don't have a real keyboard today :).

Ha!  Voice control on your phone, what could go wrong?

> I suppose this is stable material?

Good idea, (and if we're formal,
Signed-off-by: Matt Evans <matt@ozlabs.org>
).  I suppose no one's seen a vcpu fail to be created, yet.


Thanks,

Matt
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Graf - Dec. 20, 2011, 3:20 p.m.
On 07.12.2011, at 08:19, Matt Evans wrote:

> On 07/12/11 17:34, Sasha Levin wrote:
>> On Wed, 2011-12-07 at 17:17 +1100, Matt Evans wrote:
>>> On 06/12/11 19:20, Sasha Levin wrote:
>>>> Why is it getting moved out of generic code?
>>>> 
>>>> This is used to determine the maximum amount of vcpus supported by the
>>>> host for a single guest, and as far as I know KVM_CAP_NR_VCPUS and
>>>> KVM_CAP_MAX_VCPUS are not arch specific.
>>> 
>>> I checked api.txt and you're right, it isn't arch-specific.  I assumed it was,
>>> because PPC KVM doesn't support it ;-) I've dropped this patch and in its place
>>> implemented the api.txt suggestion of "if KVM_CAP_NR_VCPUS fails, use 4" instead
>>> of die(); you'll see that when I repost.
>>> 
>>> This will have the effect of PPC being limited to 4 CPUs until the kernel
>>> supports that CAP.  (I'll see about this part too.)
>> 
>> I went to look at which limitation PPC places on amount of vcpus in
>> guest, and saw this in kvmppc_core_vcpu_create() in the book3s code:
>> 
>> 	vcpu = kvmppc_core_vcpu_create(kvm, id);
>> 	vcpu->arch.wqp = &vcpu->wq;
>> 	if (!IS_ERR(vcpu))
>> 		kvmppc_create_vcpu_debugfs(vcpu, id);
>> 
>> This is wrong, right? The VCPU is dereferenced before actually checking
>> that it's not an error.
> 
> Yeah, that's b0rk.  Alex, a patch below. :)
> 
> 
> Cheers,
> 
> 
> Matt
> 
> ---
> Subject: [PATCH] KVM: PPC: Fix vcpu_create dereference before validity check.
> 
> 
> Signed-off-by: Matt Evans <matt@ozlabs.org>

Thanks, applied to kvm-ppc-next with an actual patch description added.

Alex

> ---
> arch/powerpc/kvm/powerpc.c |    5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 084d1c5..7c7220c 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -285,9 +285,10 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, unsigned int id)
> {
> 	struct kvm_vcpu *vcpu;
> 	vcpu = kvmppc_core_vcpu_create(kvm, id);
> -	vcpu->arch.wqp = &vcpu->wq;
> -	if (!IS_ERR(vcpu))
> +	if (!IS_ERR(vcpu)) {
> +		vcpu->arch.wqp = &vcpu->wq;
> 		kvmppc_create_vcpu_debugfs(vcpu, id);
> +	}
> 	return vcpu;
> }
> 
> -- 
> 1.7.0.4
> 

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 084d1c5..7c7220c 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -285,9 +285,10 @@  struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, unsigned int id)
 {
 	struct kvm_vcpu *vcpu;
 	vcpu = kvmppc_core_vcpu_create(kvm, id);
-	vcpu->arch.wqp = &vcpu->wq;
-	if (!IS_ERR(vcpu))
+	if (!IS_ERR(vcpu)) {
+		vcpu->arch.wqp = &vcpu->wq;
 		kvmppc_create_vcpu_debugfs(vcpu, id);
+	}
 	return vcpu;
 }