[PATCH] arch/powerpc/kvm: kvmppc_core_vcpu_create_hv: check for kzalloc failure

[PATCH] arch/powerpc/kvm: kvmppc_core_vcpu_create_hv: check for kzalloc failure

[Kautuk Consul]

kvmppc_vcore_create() might not be able to allocate memory through
kzalloc. In that case the kvm->arch.online_vcores shouldn't be
incremented.
Add a check for kzalloc failure and return with -ENOMEM from
kvmppc_core_vcpu_create_hv().

Signed-off-by: Kautuk Consul <kconsul@linux.vnet.ibm.com>
---
 arch/powerpc/kvm/book3s_hv.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 6ba68dd6190b..e29ee755c920 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -2968,13 +2968,17 @@ static int kvmppc_core_vcpu_create_hv(struct kvm_vcpu *vcpu)
 			pr_devel("KVM: collision on id %u", id);
 			vcore = NULL;
 		} else if (!vcore) {
+			vcore = kvmppc_vcore_create(kvm,
+					id & ~(kvm->arch.smt_mode - 1));
+			if (unlikely(!vcore)) {
+				mutex_unlock(&kvm->lock);
+				return -ENOMEM;
+			}
+
 			/*
 			 * Take mmu_setup_lock for mutual exclusion
 			 * with kvmppc_update_lpcr().
 			 */
-			err = -ENOMEM;
-			vcore = kvmppc_vcore_create(kvm,
-					id & ~(kvm->arch.smt_mode - 1));
 			mutex_lock(&kvm->arch.mmu_setup_lock);
 			kvm->arch.vcores[core] = vcore;
 			kvm->arch.online_vcores++;
-- 
2.39.2

Re: [PATCH] arch/powerpc/kvm: kvmppc_core_vcpu_create_hv: check for kzalloc failure

[Kautuk Consul]

Hi,
On 2023-03-23 03:47:18, Kautuk Consul wrote:
> kvmppc_vcore_create() might not be able to allocate memory through
> kzalloc. In that case the kvm->arch.online_vcores shouldn't be
> incremented.
> Add a check for kzalloc failure and return with -ENOMEM from
> kvmppc_core_vcpu_create_hv().
Anyone wants to review this ?
Its been a few days of silence.
By the way, I anyway have to post a v2 as I now know that
the commit description needs to be different. But I'll wait
for review comments on the code before I do so.
> 
> Signed-off-by: Kautuk Consul <kconsul@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kvm/book3s_hv.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 6ba68dd6190b..e29ee755c920 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -2968,13 +2968,17 @@ static int kvmppc_core_vcpu_create_hv(struct kvm_vcpu *vcpu)
>  			pr_devel("KVM: collision on id %u", id);
>  			vcore = NULL;
>  		} else if (!vcore) {
> +			vcore = kvmppc_vcore_create(kvm,
> +					id & ~(kvm->arch.smt_mode - 1));
> +			if (unlikely(!vcore)) {
> +				mutex_unlock(&kvm->lock);
> +				return -ENOMEM;
> +			}
> +
>  			/*
>  			 * Take mmu_setup_lock for mutual exclusion
>  			 * with kvmppc_update_lpcr().
>  			 */
> -			err = -ENOMEM;
> -			vcore = kvmppc_vcore_create(kvm,
> -					id & ~(kvm->arch.smt_mode - 1));
>  			mutex_lock(&kvm->arch.mmu_setup_lock);
>  			kvm->arch.vcores[core] = vcore;
>  			kvm->arch.online_vcores++;
> -- 
> 2.39.2
> 

Re: [PATCH] arch/powerpc/kvm: kvmppc_core_vcpu_create_hv: check for kzalloc failure

[Michael Ellerman]

Kautuk Consul <kconsul@linux.vnet.ibm.com> writes:
> kvmppc_vcore_create() might not be able to allocate memory through
> kzalloc. In that case the kvm->arch.online_vcores shouldn't be
> incremented.

I agree that looks wrong.

Have you tried to test what goes wrong if it fails? It looks like it
will break the LPCR update, which likely will cause the guest to crash
horribly.

You could use CONFIG_FAIL_SLAB and fail-nth etc. to fail just one
allocation for a guest. Or probably easier to just hack the code to fail
the 4th time it's called using a static counter.

Doesn't really matter but could be interesting.

> Add a check for kzalloc failure and return with -ENOMEM from
> kvmppc_core_vcpu_create_hv().
>
> Signed-off-by: Kautuk Consul <kconsul@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kvm/book3s_hv.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 6ba68dd6190b..e29ee755c920 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -2968,13 +2968,17 @@ static int kvmppc_core_vcpu_create_hv(struct kvm_vcpu *vcpu)
>  			pr_devel("KVM: collision on id %u", id);
>  			vcore = NULL;
>  		} else if (!vcore) {
> +			vcore = kvmppc_vcore_create(kvm,
> +					id & ~(kvm->arch.smt_mode - 1));

That line doesn't need to be wrapped, we allow 90 columns.

> +			if (unlikely(!vcore)) {
> +				mutex_unlock(&kvm->lock);
> +				return -ENOMEM;
> +			}

Rather than introducing a new return point here, I think it would be
preferable to use the existing !vcore case below.

>  			/*
>  			 * Take mmu_setup_lock for mutual exclusion
>  			 * with kvmppc_update_lpcr().
>  			 */
> -			err = -ENOMEM;
> -			vcore = kvmppc_vcore_create(kvm,
> -					id & ~(kvm->arch.smt_mode - 1));

So leave that as is (maybe move the comment down).

And wrap the below in:

 +                      if (vcore) {

>  			mutex_lock(&kvm->arch.mmu_setup_lock);
>  			kvm->arch.vcores[core] = vcore;
>  			kvm->arch.online_vcores++;
 			
 			mutex_unlock(&kvm->arch.mmu_setup_lock);
 +                      }
		}
	}

Meaning the vcore == NULL case will fall through to here and return via
this existing path:

	mutex_unlock(&kvm->lock);

	if (!vcore)
		return err;


cheers

Re: [PATCH] arch/powerpc/kvm: kvmppc_core_vcpu_create_hv: check for kzalloc failure

[Kautuk Consul]

On 2023-03-28 20:44:48, Michael Ellerman wrote:
> Kautuk Consul <kconsul@linux.vnet.ibm.com> writes:
> > kvmppc_vcore_create() might not be able to allocate memory through
> > kzalloc. In that case the kvm->arch.online_vcores shouldn't be
> > incremented.
> 
> I agree that looks wrong.
> 
> Have you tried to test what goes wrong if it fails? It looks like it
> will break the LPCR update, which likely will cause the guest to crash
> horribly.
Not sure about LPCR update, but with and without the patch qemu exits
and so the kvm context is pulled down fine.
> 
> You could use CONFIG_FAIL_SLAB and fail-nth etc. to fail just one
> allocation for a guest. Or probably easier to just hack the code to fail
> the 4th time it's called using a static counter.
I am using live debug and I set the r3 return value to 0x0 after the
call to kzalloc.
> 
> Doesn't really matter but could be interesting.
With and without this patch qemu quits with:
qemu-system-ppc64: kvm_init_vcpu: kvm_get_vcpu failed (0): Cannot allocate memory

That's because qemu will shut down when any vcpu is not able
to be allocated.
> 
> > Add a check for kzalloc failure and return with -ENOMEM from
> > kvmppc_core_vcpu_create_hv().
> >
> > Signed-off-by: Kautuk Consul <kconsul@linux.vnet.ibm.com>
> > ---
> >  arch/powerpc/kvm/book3s_hv.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> > index 6ba68dd6190b..e29ee755c920 100644
> > --- a/arch/powerpc/kvm/book3s_hv.c
> > +++ b/arch/powerpc/kvm/book3s_hv.c
> > @@ -2968,13 +2968,17 @@ static int kvmppc_core_vcpu_create_hv(struct kvm_vcpu *vcpu)
> >  			pr_devel("KVM: collision on id %u", id);
> >  			vcore = NULL;
> >  		} else if (!vcore) {
> > +			vcore = kvmppc_vcore_create(kvm,
> > +					id & ~(kvm->arch.smt_mode - 1));
> 
> That line doesn't need to be wrapped, we allow 90 columns.
> 
> > +			if (unlikely(!vcore)) {
> > +				mutex_unlock(&kvm->lock);
> > +				return -ENOMEM;
> > +			}
> 
> Rather than introducing a new return point here, I think it would be
> preferable to use the existing !vcore case below.
> 
> >  			/*
> >  			 * Take mmu_setup_lock for mutual exclusion
> >  			 * with kvmppc_update_lpcr().
> >  			 */
> > -			err = -ENOMEM;
> > -			vcore = kvmppc_vcore_create(kvm,
> > -					id & ~(kvm->arch.smt_mode - 1));
> 
> So leave that as is (maybe move the comment down).
> 
> And wrap the below in:
> 
>  +                      if (vcore) {
> 
> >  			mutex_lock(&kvm->arch.mmu_setup_lock);
> >  			kvm->arch.vcores[core] = vcore;
> >  			kvm->arch.online_vcores++;
>  			
>  			mutex_unlock(&kvm->arch.mmu_setup_lock);
>  +                      }
> 		}
> 	}
> 
> Meaning the vcore == NULL case will fall through to here and return via
> this existing path:
> 
> 	mutex_unlock(&kvm->lock);
> 
> 	if (!vcore)
> 		return err;
> 
> 
> cheers

Re: [PATCH] arch/powerpc/kvm: kvmppc_core_vcpu_create_hv: check for kzalloc failure

[Kautuk Consul]

On 2023-03-28 15:44:02, Kautuk Consul wrote:
> On 2023-03-28 20:44:48, Michael Ellerman wrote:
> > Kautuk Consul <kconsul@linux.vnet.ibm.com> writes:
> > > kvmppc_vcore_create() might not be able to allocate memory through
> > > kzalloc. In that case the kvm->arch.online_vcores shouldn't be
> > > incremented.
> > 
> > I agree that looks wrong.
> > 
> > Have you tried to test what goes wrong if it fails? It looks like it
> > will break the LPCR update, which likely will cause the guest to crash
> > horribly.
Also, are you referring to the code in kvmppc_update_lpcr()?
That code will not crash as it checks for the vc before trying to
dereference it.
But the following 2 places that utilize the arch.online_vcores will have
problems in logic if the usermode test-case doesn't pull down the
kvm context after the -ENOMEM vcpu allocation failure:
book3s_hv.c:3030:       if (!kvm->arch.online_vcores) {
book3s_hv_rm_mmu.c:44:  if (kvm->arch.online_vcores == 1 && local_paca->kvm_hstate.kvm_vcpu)

> Not sure about LPCR update, but with and without the patch qemu exits
> and so the kvm context is pulled down fine.
> > 
> > You could use CONFIG_FAIL_SLAB and fail-nth etc. to fail just one
> > allocation for a guest. Or probably easier to just hack the code to fail
> > the 4th time it's called using a static counter.
> I am using live debug and I set the r3 return value to 0x0 after the
> call to kzalloc.
> > 
> > Doesn't really matter but could be interesting.
> With and without this patch qemu quits with:
> qemu-system-ppc64: kvm_init_vcpu: kvm_get_vcpu failed (0): Cannot allocate memory
> 
> That's because qemu will shut down when any vcpu is not able
> to be allocated.
> > 
> > > Add a check for kzalloc failure and return with -ENOMEM from
> > > kvmppc_core_vcpu_create_hv().
> > >
> > > Signed-off-by: Kautuk Consul <kconsul@linux.vnet.ibm.com>
> > > ---
> > >  arch/powerpc/kvm/book3s_hv.c | 10 +++++++---
> > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> > > index 6ba68dd6190b..e29ee755c920 100644
> > > --- a/arch/powerpc/kvm/book3s_hv.c
> > > +++ b/arch/powerpc/kvm/book3s_hv.c
> > > @@ -2968,13 +2968,17 @@ static int kvmppc_core_vcpu_create_hv(struct kvm_vcpu *vcpu)
> > >  			pr_devel("KVM: collision on id %u", id);
> > >  			vcore = NULL;
> > >  		} else if (!vcore) {
> > > +			vcore = kvmppc_vcore_create(kvm,
> > > +					id & ~(kvm->arch.smt_mode - 1));
> > 
> > That line doesn't need to be wrapped, we allow 90 columns.
> > 
> > > +			if (unlikely(!vcore)) {
> > > +				mutex_unlock(&kvm->lock);
> > > +				return -ENOMEM;
> > > +			}
> > 
> > Rather than introducing a new return point here, I think it would be
> > preferable to use the existing !vcore case below.
> > 
> > >  			/*
> > >  			 * Take mmu_setup_lock for mutual exclusion
> > >  			 * with kvmppc_update_lpcr().
> > >  			 */
> > > -			err = -ENOMEM;
> > > -			vcore = kvmppc_vcore_create(kvm,
> > > -					id & ~(kvm->arch.smt_mode - 1));
> > 
> > So leave that as is (maybe move the comment down).
> > 
> > And wrap the below in:
> > 
> >  +                      if (vcore) {
> > 
> > >  			mutex_lock(&kvm->arch.mmu_setup_lock);
> > >  			kvm->arch.vcores[core] = vcore;
> > >  			kvm->arch.online_vcores++;
> >  			
> >  			mutex_unlock(&kvm->arch.mmu_setup_lock);
> >  +                      }
> > 		}
> > 	}
> > 
> > Meaning the vcore == NULL case will fall through to here and return via
> > this existing path:
> > 
> > 	mutex_unlock(&kvm->lock);
> > 
> > 	if (!vcore)
> > 		return err;
> > 
> > 
> > cheers

Re: [PATCH] arch/powerpc/kvm: kvmppc_core_vcpu_create_hv: check for kzalloc failure

[Michael Ellerman]

Kautuk Consul <kconsul@linux.vnet.ibm.com> writes:
> On 2023-03-28 15:44:02, Kautuk Consul wrote:
>> On 2023-03-28 20:44:48, Michael Ellerman wrote:
>> > Kautuk Consul <kconsul@linux.vnet.ibm.com> writes:
>> > > kvmppc_vcore_create() might not be able to allocate memory through
>> > > kzalloc. In that case the kvm->arch.online_vcores shouldn't be
>> > > incremented.
>> > 
>> > I agree that looks wrong.
>> > 
>> > Have you tried to test what goes wrong if it fails? It looks like it
>> > will break the LPCR update, which likely will cause the guest to crash
>> > horribly.
> Also, are you referring to the code in kvmppc_update_lpcr()?
> That code will not crash as it checks for the vc before trying to
> dereference it.

Yeah that's what I was looking at. I didn't mean it would crash, but
that it would bail out early when it sees a NULL vcore, leaving other
vcores with the wrong LPCR value.

But as you say it doesn't happen because qemu quits on the first ENOMEM.

And regardless if qemu does something that means the guest is broken
that's just a qemu bug, no big deal as far as the kernel is concerned.

> But the following 2 places that utilize the arch.online_vcores will have
> problems in logic if the usermode test-case doesn't pull down the
> kvm context after the -ENOMEM vcpu allocation failure:
> book3s_hv.c:3030:       if (!kvm->arch.online_vcores) {
> book3s_hv_rm_mmu.c:44:  if (kvm->arch.online_vcores == 1 && local_paca->kvm_hstate.kvm_vcpu)

OK. Both of those look harmless to the host.

If we find a case where a misbehaving qemu can crash the host then we
need to be a bit more careful and treat it at least as a
denial-of-service bug. But looks like this is not one of those.

cheers

Re: [PATCH] arch/powerpc/kvm: kvmppc_core_vcpu_create_hv: check for kzalloc failure

[Kautuk Consul]

On 2023-03-28 23:02:09, Michael Ellerman wrote:
> Kautuk Consul <kconsul@linux.vnet.ibm.com> writes:
> > On 2023-03-28 15:44:02, Kautuk Consul wrote:
> >> On 2023-03-28 20:44:48, Michael Ellerman wrote:
> >> > Kautuk Consul <kconsul@linux.vnet.ibm.com> writes:
> >> > > kvmppc_vcore_create() might not be able to allocate memory through
> >> > > kzalloc. In that case the kvm->arch.online_vcores shouldn't be
> >> > > incremented.
> >> > 
> >> > I agree that looks wrong.
> >> > 
> >> > Have you tried to test what goes wrong if it fails? It looks like it
> >> > will break the LPCR update, which likely will cause the guest to crash
> >> > horribly.
> > Also, are you referring to the code in kvmppc_update_lpcr()?
> > That code will not crash as it checks for the vc before trying to
> > dereference it.
> 
> Yeah that's what I was looking at. I didn't mean it would crash, but
> that it would bail out early when it sees a NULL vcore, leaving other
> vcores with the wrong LPCR value.
> 
> But as you say it doesn't happen because qemu quits on the first ENOMEM.
> 
> And regardless if qemu does something that means the guest is broken
> that's just a qemu bug, no big deal as far as the kernel is concerned.
But there could be another user-mode application other than qemu that
actually tries to create a vcpu after it gets a -ENOMEM for another
vcpu. Shouldn't the kernel be independent of qemu?
> 
> > But the following 2 places that utilize the arch.online_vcores will have
> > problems in logic if the usermode test-case doesn't pull down the
> > kvm context after the -ENOMEM vcpu allocation failure:
> > book3s_hv.c:3030:       if (!kvm->arch.online_vcores) {
> > book3s_hv_rm_mmu.c:44:  if (kvm->arch.online_vcores == 1 && local_paca->kvm_hstate.kvm_vcpu)
> 
> OK. Both of those look harmless to the host.
Harmless to the host in terms of a crash, not in terms of behavior.
For example in the case of kvmhv_set_smt_mode:
If we got a kzalloc failure once (and online_vcores was wrongly incremented), 
then if kvmhv_set_smt_mode() is called after that then it would be
not be setting the arch.smt_mode and arch.emul_smt_mode correctly and it
would be wrongly returning with -EBUSY instead of 0.
Isn't that incorrect with respect to the intent of the code ?
I agree that applications like qemu might not do that but don't we need
to have some integrity with respect to the intent and value of variable
use ? What about good code and logic quality ?
> 
> If we find a case where a misbehaving qemu can crash the host then we
> need to be a bit more careful and treat it at least as a
> denial-of-service bug. But looks like this is not one of those.
> 
> cheers
beers

Re: [PATCH] arch/powerpc/kvm: kvmppc_core_vcpu_create_hv: check for kzalloc failure

[Michael Ellerman]

Kautuk Consul <kconsul@linux.vnet.ibm.com> writes:
> On 2023-03-28 23:02:09, Michael Ellerman wrote:
>> Kautuk Consul <kconsul@linux.vnet.ibm.com> writes:
>> > On 2023-03-28 15:44:02, Kautuk Consul wrote:
>> >> On 2023-03-28 20:44:48, Michael Ellerman wrote:
>> >> > Kautuk Consul <kconsul@linux.vnet.ibm.com> writes:
>> >> > > kvmppc_vcore_create() might not be able to allocate memory through
>> >> > > kzalloc. In that case the kvm->arch.online_vcores shouldn't be
>> >> > > incremented.
>> >> > 
>> >> > I agree that looks wrong.
>> >> > 
>> >> > Have you tried to test what goes wrong if it fails? It looks like it
>> >> > will break the LPCR update, which likely will cause the guest to crash
>> >> > horribly.
>> > Also, are you referring to the code in kvmppc_update_lpcr()?
>> > That code will not crash as it checks for the vc before trying to
>> > dereference it.
>> 
>> Yeah that's what I was looking at. I didn't mean it would crash, but
>> that it would bail out early when it sees a NULL vcore, leaving other
>> vcores with the wrong LPCR value.
>> 
>> But as you say it doesn't happen because qemu quits on the first ENOMEM.
>> 
>> And regardless if qemu does something that means the guest is broken
>> that's just a qemu bug, no big deal as far as the kernel is concerned.

> But there could be another user-mode application other than qemu that
> actually tries to create a vcpu after it gets a -ENOMEM for another
> vcpu. Shouldn't the kernel be independent of qemu?

Yes, the kernel is independent of qemu.

On P8 we had kvmtool, and there's several other VMMs these days, though
most don't support Power.

I didn't mean qemu specifically above. If any VMM continues blindly
after getting ENOMEM back from the KVM API then that's a bug in that
VMM.

>> > But the following 2 places that utilize the arch.online_vcores will have
>> > problems in logic if the usermode test-case doesn't pull down the
>> > kvm context after the -ENOMEM vcpu allocation failure:
>> > book3s_hv.c:3030:       if (!kvm->arch.online_vcores) {
>> > book3s_hv_rm_mmu.c:44:  if (kvm->arch.online_vcores == 1 && local_paca->kvm_hstate.kvm_vcpu)
>> 
>> OK. Both of those look harmless to the host.

> Harmless to the host in terms of a crash, not in terms of behavior.
> For example in the case of kvmhv_set_smt_mode:
> If we got a kzalloc failure once (and online_vcores was wrongly incremented), 
> then if kvmhv_set_smt_mode() is called after that then it would be
> not be setting the arch.smt_mode and arch.emul_smt_mode correctly and it
> would be wrongly returning with -EBUSY instead of 0.

But again that bug only affects that VM, which the VMM should have
terminated when it got the ENOMEM back.

It's definitely a bug that we increment online_vcores incorrectly, but
it only affects that VM, and a correctly operating VMM will terminate
the VM anyway because of the ENOMEM.

cheers

Re: [PATCH] arch/powerpc/kvm: kvmppc_core_vcpu_create_hv: check for kzalloc failure

[Kautuk Consul]

On 2023-03-30 10:59:19, Michael Ellerman wrote:
> Kautuk Consul <kconsul@linux.vnet.ibm.com> writes:
> > On 2023-03-28 23:02:09, Michael Ellerman wrote:
> >> Kautuk Consul <kconsul@linux.vnet.ibm.com> writes:
> >> > On 2023-03-28 15:44:02, Kautuk Consul wrote:
> >> >> On 2023-03-28 20:44:48, Michael Ellerman wrote:
> >> >> > Kautuk Consul <kconsul@linux.vnet.ibm.com> writes:
> >> >> > > kvmppc_vcore_create() might not be able to allocate memory through
> >> >> > > kzalloc. In that case the kvm->arch.online_vcores shouldn't be
> >> >> > > incremented.
> >> >> > 
> >> >> > I agree that looks wrong.
> >> >> > 
> >> >> > Have you tried to test what goes wrong if it fails? It looks like it
> >> >> > will break the LPCR update, which likely will cause the guest to crash
> >> >> > horribly.
> >> > Also, are you referring to the code in kvmppc_update_lpcr()?
> >> > That code will not crash as it checks for the vc before trying to
> >> > dereference it.
> >> 
> >> Yeah that's what I was looking at. I didn't mean it would crash, but
> >> that it would bail out early when it sees a NULL vcore, leaving other
> >> vcores with the wrong LPCR value.
> >> 
> >> But as you say it doesn't happen because qemu quits on the first ENOMEM.
> >> 
> >> And regardless if qemu does something that means the guest is broken
> >> that's just a qemu bug, no big deal as far as the kernel is concerned.
> 
> > But there could be another user-mode application other than qemu that
> > actually tries to create a vcpu after it gets a -ENOMEM for another
> > vcpu. Shouldn't the kernel be independent of qemu?
> 
> Yes, the kernel is independent of qemu.
> 
> On P8 we had kvmtool, and there's several other VMMs these days, though
> most don't support Power.
> 
> I didn't mean qemu specifically above. If any VMM continues blindly
> after getting ENOMEM back from the KVM API then that's a bug in that
> VMM.
> 
> >> > But the following 2 places that utilize the arch.online_vcores will have
> >> > problems in logic if the usermode test-case doesn't pull down the
> >> > kvm context after the -ENOMEM vcpu allocation failure:
> >> > book3s_hv.c:3030:       if (!kvm->arch.online_vcores) {
> >> > book3s_hv_rm_mmu.c:44:  if (kvm->arch.online_vcores == 1 && local_paca->kvm_hstate.kvm_vcpu)
> >> 
> >> OK. Both of those look harmless to the host.
> 
> > Harmless to the host in terms of a crash, not in terms of behavior.
> > For example in the case of kvmhv_set_smt_mode:
> > If we got a kzalloc failure once (and online_vcores was wrongly incremented), 
> > then if kvmhv_set_smt_mode() is called after that then it would be
> > not be setting the arch.smt_mode and arch.emul_smt_mode correctly and it
> > would be wrongly returning with -EBUSY instead of 0.
> 
> But again that bug only affects that VM, which the VMM should have
> terminated when it got the ENOMEM back.
> 
> It's definitely a bug that we increment online_vcores incorrectly, but
> it only affects that VM, and a correctly operating VMM will terminate
> the VM anyway because of the ENOMEM.
Okay, I understand. I used to earlier try to contribute to other
mailing lists and they were very particular about correcting code
that was doing something wrong (just by code review) irrespective of whether
it would actually result in a bug/crash or misbehaviour. I guess maintainers
look at the generic part of the kernel in a different way than arch or
device specific kernel/driver code.
> 
> cheers