[PATCH] KVM: arm/arm64: ensure vcpu target is unset on reset failure

[PATCH] KVM: arm/arm64: ensure vcpu target is unset on reset failure

[Andrew Jones]

A failed KVM_ARM_VCPU_INIT, should not set the vcpu target,
as the vcpu target is used by kvm_vcpu_initialized() to
determine if other vcpu ioctls may proceed. We need to set
the target before calling kvm_reset_vcpu(), but if that call
fails, we should then unset it.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 virt/kvm/arm/arm.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 99c37384ba7b..7e5724ae1efd 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -934,7 +934,7 @@ int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct kvm_irq_level *irq_level,
 static int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
 			       const struct kvm_vcpu_init *init)
 {
-	unsigned int i;
+	unsigned int i, ret;
 	int phys_target = kvm_target_cpu();
 
 	if (init->target != phys_target)
@@ -969,9 +969,15 @@ static int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
 	vcpu->arch.target = phys_target;
 
 	/* Now we know what it is, we can reset it. */
-	return kvm_reset_vcpu(vcpu);
-}
+	ret = kvm_reset_vcpu(vcpu);
+	if (ret) {
+		vcpu->arch.target = -1;
+		bitmap_zero(vcpu->arch.features, KVM_VCPU_MAX_FEATURES);
+		return ret;
+	}
 
+	return 0;
+}
 
 static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
 					 struct kvm_vcpu_init *init)
-- 
2.20.1

Re: [PATCH] KVM: arm/arm64: ensure vcpu target is unset on reset failure

[Marc Zyngier]

On Thu, 04 Apr 2019 18:42:30 +0100,
Andrew Jones <drjones@redhat.com> wrote:
> 
> A failed KVM_ARM_VCPU_INIT, should not set the vcpu target,
> as the vcpu target is used by kvm_vcpu_initialized() to
> determine if other vcpu ioctls may proceed. We need to set
> the target before calling kvm_reset_vcpu(), but if that call
> fails, we should then unset it.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  virt/kvm/arm/arm.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 99c37384ba7b..7e5724ae1efd 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -934,7 +934,7 @@ int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct kvm_irq_level *irq_level,
>  static int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
>  			       const struct kvm_vcpu_init *init)
>  {
> -	unsigned int i;
> +	unsigned int i, ret;
>  	int phys_target = kvm_target_cpu();
>  
>  	if (init->target != phys_target)
> @@ -969,9 +969,15 @@ static int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
>  	vcpu->arch.target = phys_target;
>  
>  	/* Now we know what it is, we can reset it. */
> -	return kvm_reset_vcpu(vcpu);
> -}
> +	ret = kvm_reset_vcpu(vcpu);
> +	if (ret) {
> +		vcpu->arch.target = -1;
> +		bitmap_zero(vcpu->arch.features, KVM_VCPU_MAX_FEATURES);
> +		return ret;

This return could trivially be removed...

> +	}
>  
> +	return 0;
> +}

... and this turned into 'return ret'.

I've tentatively applied this to the 5.1-fixes branch. Let me know if
you're OK with it.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

Re: [PATCH] KVM: arm/arm64: ensure vcpu target is unset on reset failure

[Andrew Jones]

On Fri, Apr 05, 2019 at 07:38:59AM +0100, Marc Zyngier wrote:
> On Thu, 04 Apr 2019 18:42:30 +0100,
> Andrew Jones <drjones@redhat.com> wrote:
> > 
> > A failed KVM_ARM_VCPU_INIT, should not set the vcpu target,
> > as the vcpu target is used by kvm_vcpu_initialized() to
> > determine if other vcpu ioctls may proceed. We need to set
> > the target before calling kvm_reset_vcpu(), but if that call
> > fails, we should then unset it.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  virt/kvm/arm/arm.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > index 99c37384ba7b..7e5724ae1efd 100644
> > --- a/virt/kvm/arm/arm.c
> > +++ b/virt/kvm/arm/arm.c
> > @@ -934,7 +934,7 @@ int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct kvm_irq_level *irq_level,
> >  static int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
> >  			       const struct kvm_vcpu_init *init)
> >  {
> > -	unsigned int i;
> > +	unsigned int i, ret;
> >  	int phys_target = kvm_target_cpu();
> >  
> >  	if (init->target != phys_target)
> > @@ -969,9 +969,15 @@ static int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
> >  	vcpu->arch.target = phys_target;
> >  
> >  	/* Now we know what it is, we can reset it. */
> > -	return kvm_reset_vcpu(vcpu);
> > -}
> > +	ret = kvm_reset_vcpu(vcpu);
> > +	if (ret) {
> > +		vcpu->arch.target = -1;
> > +		bitmap_zero(vcpu->arch.features, KVM_VCPU_MAX_FEATURES);
> > +		return ret;
> 
> This return could trivially be removed...
> 
> > +	}
> >  
> > +	return 0;
> > +}
> 
> ... and this turned into 'return ret'.
> 
> I've tentatively applied this to the 5.1-fixes branch. Let me know if
> you're OK with it.
>

Either way is fine by me. I actually did it this way on purpose though
because I preferred the way the explicit 'return 0' at the bottom
documented that we were sure at that point of success, so no longer
needed to be concerned that target should be reset to -1.

Thanks,
drew