KVM: arm64: nv: Fix MI line level calculation in vgic_v3_nested_update_mi()

[PATCH] KVM: arm64: nv: Fix MI line level calculation in vgic_v3_nested_update_mi()

Posted by Wei-Lin Chang 3 months, 2 weeks ago

The state of the vcpu's MI line should be asserted when its
ICH_HCR_EL2.En is set and ICH_MISR_EL2 is non-zero. Using bitwise AND
(&=) directly for this calculation will not give us the correct result
when the LSB of the vcpu's ICH_MISR_EL2 isn't set. Correct this by first
adjusting the return value of vgic_v3_get_misr() into 1 if it is
non-zero.

Signed-off-by: Wei-Lin Chang <r09922117@csie.ntu.edu.tw>
---
 arch/arm64/kvm/vgic/vgic-v3-nested.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/vgic/vgic-v3-nested.c b/arch/arm64/kvm/vgic/vgic-v3-nested.c
index 4f6954c30674..ebffad632fd2 100644
--- a/arch/arm64/kvm/vgic/vgic-v3-nested.c
+++ b/arch/arm64/kvm/vgic/vgic-v3-nested.c
@@ -400,7 +400,7 @@ void vgic_v3_nested_update_mi(struct kvm_vcpu *vcpu)
 
 	level  = __vcpu_sys_reg(vcpu, ICH_HCR_EL2) & ICH_HCR_EL2_En;
 	if (level)
-		level &= vgic_v3_get_misr(vcpu);
+		level &= !!vgic_v3_get_misr(vcpu);
 	kvm_vgic_inject_irq(vcpu->kvm, vcpu,
 			    vcpu->kvm->arch.vgic.mi_intid, level, vcpu);
 }
-- 
2.49.0

Re: [PATCH] KVM: arm64: nv: Fix MI line level calculation in vgic_v3_nested_update_mi()

Posted by Marc Zyngier 3 months, 2 weeks ago

On Wed, 25 Jun 2025 16:47:09 +0800, Wei-Lin Chang wrote:
> The state of the vcpu's MI line should be asserted when its
> ICH_HCR_EL2.En is set and ICH_MISR_EL2 is non-zero. Using bitwise AND
> (&=) directly for this calculation will not give us the correct result
> when the LSB of the vcpu's ICH_MISR_EL2 isn't set. Correct this by first
> adjusting the return value of vgic_v3_get_misr() into 1 if it is
> non-zero.
> 
> [...]

Applied to fixes, thanks!

[1/1] KVM: arm64: nv: Fix MI line level calculation in vgic_v3_nested_update_mi()
      commit: af040a9a296044fd4b748786c2516f172a7617f1

Cheers,

	M.
-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] KVM: arm64: nv: Fix MI line level calculation in vgic_v3_nested_update_mi()

Posted by Marc Zyngier 3 months, 2 weeks ago

On Wed, 25 Jun 2025 09:47:09 +0100,
Wei-Lin Chang <r09922117@csie.ntu.edu.tw> wrote:
> 
> The state of the vcpu's MI line should be asserted when its
> ICH_HCR_EL2.En is set and ICH_MISR_EL2 is non-zero. Using bitwise AND
> (&=) directly for this calculation will not give us the correct result
> when the LSB of the vcpu's ICH_MISR_EL2 isn't set. Correct this by first
> adjusting the return value of vgic_v3_get_misr() into 1 if it is
> non-zero.
> 
> Signed-off-by: Wei-Lin Chang <r09922117@csie.ntu.edu.tw>
> ---
>  arch/arm64/kvm/vgic/vgic-v3-nested.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-v3-nested.c b/arch/arm64/kvm/vgic/vgic-v3-nested.c
> index 4f6954c30674..ebffad632fd2 100644
> --- a/arch/arm64/kvm/vgic/vgic-v3-nested.c
> +++ b/arch/arm64/kvm/vgic/vgic-v3-nested.c
> @@ -400,7 +400,7 @@ void vgic_v3_nested_update_mi(struct kvm_vcpu *vcpu)
>  
>  	level  = __vcpu_sys_reg(vcpu, ICH_HCR_EL2) & ICH_HCR_EL2_En;
>  	if (level)
> -		level &= vgic_v3_get_misr(vcpu);
> +		level &= !!vgic_v3_get_misr(vcpu);
>  	kvm_vgic_inject_irq(vcpu->kvm, vcpu,
>  			    vcpu->kvm->arch.vgic.mi_intid, level, vcpu);
>  }

Very well spotted, once more. Where were you when I posted all these
patches? ;-)

We could make it even clearer with this:

diff --git a/arch/arm64/kvm/vgic/vgic-v3-nested.c b/arch/arm64/kvm/vgic/vgic-v3-nested.c
index a50fb7e6841f7..679aafe77de2e 100644
--- a/arch/arm64/kvm/vgic/vgic-v3-nested.c
+++ b/arch/arm64/kvm/vgic/vgic-v3-nested.c
@@ -401,9 +401,7 @@ void vgic_v3_nested_update_mi(struct kvm_vcpu *vcpu)
 {
 	bool level;
 
-	level  = __vcpu_sys_reg(vcpu, ICH_HCR_EL2) & ICH_HCR_EL2_En;
-	if (level)
-		level &= vgic_v3_get_misr(vcpu);
+	level = (__vcpu_sys_reg(vcpu, ICH_HCR_EL2) & ICH_HCR_EL2_En) && vgic_v3_get_misr(vcpu);
 	kvm_vgic_inject_irq(vcpu->kvm, vcpu,
 			    vcpu->kvm->arch.vgic.mi_intid, level, vcpu);
 }

If you're OK with it, I'll use this, keeping your authorship of
course.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] KVM: arm64: nv: Fix MI line level calculation in vgic_v3_nested_update_mi()

Posted by Wei-Lin Chang 3 months, 2 weeks ago

On Wed, Jun 25, 2025 at 05:45:06PM +0100, Marc Zyngier wrote:
> On Wed, 25 Jun 2025 09:47:09 +0100,
> Wei-Lin Chang <r09922117@csie.ntu.edu.tw> wrote:
> > 
> > The state of the vcpu's MI line should be asserted when its
> > ICH_HCR_EL2.En is set and ICH_MISR_EL2 is non-zero. Using bitwise AND
> > (&=) directly for this calculation will not give us the correct result
> > when the LSB of the vcpu's ICH_MISR_EL2 isn't set. Correct this by first
> > adjusting the return value of vgic_v3_get_misr() into 1 if it is
> > non-zero.
> > 
> > Signed-off-by: Wei-Lin Chang <r09922117@csie.ntu.edu.tw>
> > ---
> >  arch/arm64/kvm/vgic/vgic-v3-nested.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/kvm/vgic/vgic-v3-nested.c b/arch/arm64/kvm/vgic/vgic-v3-nested.c
> > index 4f6954c30674..ebffad632fd2 100644
> > --- a/arch/arm64/kvm/vgic/vgic-v3-nested.c
> > +++ b/arch/arm64/kvm/vgic/vgic-v3-nested.c
> > @@ -400,7 +400,7 @@ void vgic_v3_nested_update_mi(struct kvm_vcpu *vcpu)
> >  
> >  	level  = __vcpu_sys_reg(vcpu, ICH_HCR_EL2) & ICH_HCR_EL2_En;
> >  	if (level)
> > -		level &= vgic_v3_get_misr(vcpu);
> > +		level &= !!vgic_v3_get_misr(vcpu);
> >  	kvm_vgic_inject_irq(vcpu->kvm, vcpu,
> >  			    vcpu->kvm->arch.vgic.mi_intid, level, vcpu);
> >  }
> 
> Very well spotted, once more. Where were you when I posted all these
> patches? ;-)

Thanks ;)

> 
> We could make it even clearer with this:
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-v3-nested.c b/arch/arm64/kvm/vgic/vgic-v3-nested.c
> index a50fb7e6841f7..679aafe77de2e 100644
> --- a/arch/arm64/kvm/vgic/vgic-v3-nested.c
> +++ b/arch/arm64/kvm/vgic/vgic-v3-nested.c
> @@ -401,9 +401,7 @@ void vgic_v3_nested_update_mi(struct kvm_vcpu *vcpu)
>  {
>  	bool level;
>  
> -	level  = __vcpu_sys_reg(vcpu, ICH_HCR_EL2) & ICH_HCR_EL2_En;
> -	if (level)
> -		level &= vgic_v3_get_misr(vcpu);
> +	level = (__vcpu_sys_reg(vcpu, ICH_HCR_EL2) & ICH_HCR_EL2_En) && vgic_v3_get_misr(vcpu);
>  	kvm_vgic_inject_irq(vcpu->kvm, vcpu,
>  			    vcpu->kvm->arch.vgic.mi_intid, level, vcpu);
>  }
> 
> If you're OK with it, I'll use this, keeping your authorship of
> course.

Yes this is more straightforward, with this the commit message should be
rephrased too. I'm ok with keeping my authorship but really feel free to
adjust if you think some other credit is more appropriate.
I'm just glad that I'm able to help a little :)

Thanks,
Wei-Lin Chang

> 
> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.