Re: [PATCH] KVM: arm64: nv: Fix s_cpu_if->vgic_lr[] indexing in vgic_v3_put_nested()

[Marc Zyngier <maz@kernel.org>]

On Tue, 17 Jun 2025 05:53:20 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Mon, Jun 16, 2025 at 11:54:57AM +0100, Marc Zyngier wrote:
> > On Sat, 14 Jun 2025 15:57:21 +0100,
> > Wei-Lin Chang <r09922117@csie.ntu.edu.tw> wrote:
> > > 
> > > s_cpu_if->vgic_lr[] is filled continuously from index 0 to
> > > s_cpu_if->used_lrs - 1, but vgic_v3_put_nested() is indexing it using
> > > the positions of the set bits in shadow_if->lr_map. So correct it.
> > > 
> > > Signed-off-by: Wei-Lin Chang <r09922117@csie.ntu.edu.tw>
> > > ---
> > >  arch/arm64/kvm/vgic/vgic-v3-nested.c | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kvm/vgic/vgic-v3-nested.c b/arch/arm64/kvm/vgic/vgic-v3-nested.c
> > > index 4f6954c30674..29741e3f077b 100644
> > > --- a/arch/arm64/kvm/vgic/vgic-v3-nested.c
> > > +++ b/arch/arm64/kvm/vgic/vgic-v3-nested.c
> > > @@ -343,7 +343,7 @@ void vgic_v3_put_nested(struct kvm_vcpu *vcpu)
> > >  	struct shadow_if *shadow_if = get_shadow_if();
> > >  	struct vgic_v3_cpu_if *s_cpu_if = &shadow_if->cpuif;
> > >  	u64 val;
> > > -	int i;
> > > +	int i, index = 0;
> > >  
> > >  	__vgic_v3_save_vmcr_aprs(s_cpu_if);
> > >  	__vgic_v3_deactivate_traps(s_cpu_if);
> > > @@ -368,10 +368,11 @@ void vgic_v3_put_nested(struct kvm_vcpu *vcpu)
> > >  		val = __vcpu_sys_reg(vcpu, ICH_LRN(i));
> > >  
> > >  		val &= ~ICH_LR_STATE;
> > > -		val |= s_cpu_if->vgic_lr[i] & ICH_LR_STATE;
> > > +		val |= s_cpu_if->vgic_lr[index] & ICH_LR_STATE;
> > >  
> > >  		__vcpu_sys_reg(vcpu, ICH_LRN(i)) = val;
> > > -		s_cpu_if->vgic_lr[i] = 0;
> > > +		s_cpu_if->vgic_lr[index] = 0;
> > > +		index++;
> > >  	}
> > >  
> > >  	shadow_if->lr_map = 0;
> > 
> > Nice catch, thanks a lot for tracking it down.
> > 
> > However, I think we should get rid of this double-indexing altogether,
> > or at least make it less error-prone. This thing is extremely fragile,
> > and it isn't the first time we are getting bitten with it.
> > 
> > Looking at the code, it becomes pretty obvious that the shadow index
> > is always the number of bits set in lr_map, and that we could
> > completely drop the 'index' thing if we simply counted these bits
> > (which isn't that expensive).
> > 
> > I came up with the (admittedly much bigger) following fix.
> > 
> > Thoughts?
> > 
> > 	M.
> > 
> > From 2484950b8fc3b36cca32bf5e86ffe7975a43e0e7 Mon Sep 17 00:00:00 2001
> > From: Marc Zyngier <maz@kernel.org>
> > Date: Sun, 15 Jun 2025 16:11:38 +0100
> > Subject: [PATCH] KVM: arm64: nv: Fix tracking of shadow list registers
> > 
> > Wei-Lin reports that the tracking of shadow list registers is
> > majorly broken when resync'ing the L2 state after a run, as
> > we confuse the guest's LR index with the host's, potentially
> > losing the interrupt state.
> > 
> > While this could be fixed by adding yet another side index to
> > track it (Wei-Lin's fix), it may be better to refactor this
> > code to avoid having a side index altogether, limiting the
> > risk to introduce this class of bugs.
> > 
> > A key observation is that the shadow index is always the number
> > of bits in the lr_map bitmap. With that, the parallel indexing
> > scheme can be completely dropped.
> > 
> > While doing this, introduce a couple of helpers that abstract
> > the index conversion and some of the LR repainting, making the
> > whole exercise much simpler.
> > 
> > Reported-by: Wei-Lin Chang <r09922117@csie.ntu.edu.tw>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > Link: https://lore.kernel.org/r/20250614145721.2504524-1-r09922117@csie.ntu.edu.tw
> 
> Besides Wei-Lin's comments, LGTM.
> 
> Reviewed-by: Oliver Upton <oliver.upton@linux.dev>

Thanks!

For the record, I've since amended the patch to use hweight16()
instead of hweight64(), which saves us a MUL instruction. We can do
that since there is a hard limit of 16 LRs in the architecture.

	M.

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