Re: [PATCH v2 1/4] KVM: arm64: nv: Keep reference on stage-2 MMU when scheduled out

[Sean Christopherson <seanjc@google.com>]

On Mon, Oct 07, 2024, Oliver Upton wrote:
> On Mon, Oct 07, 2024 at 12:52:48PM -0700, Sean Christopherson wrote:
> > > That wouldn't be an optimization, it'd render the halt polling loop
> > > useless.
> 
> <snip>
> 
> > Hmm, but what happens if the wakeup event arrives before IN_WFI is set?  Ah, IIUC,
> > GICR_VPENDBASER_PendingLast tracks if there's a pending event, KVM makes sure to
> > read PendingLast after making vPE non-resident, and hardware is required to ensure
> > either PendingLast=1 or a doorbell is signaled.
> > 
> > Niave question time: why not query GICR_VPENDBASER_PendingLast directly in
> > kvm_vgic_vcpu_pending_irq() instead of putting the vGIC?
> 
> The GIC is only required to update GICR_VPENDBASER.PendingLast when
> software deschedules the loaded vPE, meaning GICR_VPENDBASER.Valid goes
> from 1 => 0.

Ugh, that sucks.  I wish we could magically combine Intel+AMD+ARM implementations
for direct injection.  They all do things just slightly differently, and each one
has some behaviors that I really like, and each one has at least one behavior that
makes me sad.

> > > The most recent view of the guest's CPU interface is sitting in
> > > hardware at this point, so we need to synchronize KVM's view of
> > > the CPUIF with it to determine if a pending interrupt exceeds the
> > > priority mask.
> 
> </paste>
> 
> The guest's enable bits and priority mask are in ICH_VMCR_EL2, and we
> need to read that out of the CPU in order for this loop to work:
> 
> int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu)
> {
> 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> 	struct vgic_irq *irq;
> 	bool pending = false;
> 	unsigned long flags;
> 	struct vgic_vmcr vmcr;
> 
> 	if (!vcpu->kvm->arch.vgic.enabled)
> 		return false;
> 
> 	if (vcpu->arch.vgic_cpu.vgic_v3.its_vpe.pending_last)
> 		return true;
> 
> 	vgic_get_vmcr(vcpu, &vmcr);
> 
> 	raw_spin_lock_irqsave(&vgic_cpu->ap_list_lock, flags);
> 
> 	list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list) {
> 		raw_spin_lock(&irq->irq_lock);
> 		pending = irq_is_pending(irq) && irq->enabled &&
> 			  !irq->active &&
> 			  irq->priority < vmcr.pmr;		<====== Need latest PMR
> 		raw_spin_unlock(&irq->irq_lock);
> 
> 		if (pending)
> 			break;
> 	}
> 
> 	raw_spin_unlock_irqrestore(&vgic_cpu->ap_list_lock, flags);
> 
> 	return pending;
> }
> 
> KVM's in-memory view of VMCR is updated in vgic_put().

Ah, vgic_get_vmcr() is just unpacking the software-cached VMCR.  I missed that
on the first few reads.

Thanks for being patient :-)

> So even w/o GICv4 direct injection, the seemingly premature GIC save/restore
> is essential for halt polling to actually do something useful.