Re: [Xen-devel] [PATCH v3 26/39] ARM: new VGIC: Add SGIPENDR register handlers

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 28 Mar 2018, Andre Przywara wrote:
> >> +static void vgic_mmio_write_sgipends(struct vcpu *vcpu,
> >> +                                     paddr_t addr, unsigned int len,
> >> +                                     unsigned long val)
> >> +{
> >> +    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 8);
> >> +    unsigned int i;
> >> +    unsigned long flags;
> >> +
> >> +    ASSERT(intid < VGIC_NR_SGIS);
> >> +
> >> +    for ( i = 0; i < len; i++ )
> >> +    {
> >> +        struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + 
> >> i);
> >> +
> >> +        spin_lock_irqsave(&irq->irq_lock, flags);
> >> +
> >> +        irq->source |= (val >> (i * 8)) & 0xff;
> >> +
> >> +        if ( irq->source )
> >> +        {
> >> +            irq->pending_latch = true;
> >> +            vgic_queue_irq_unlock(vcpu->domain, irq, flags);
> >> +        }
> >> +        else
> >> +        {
> >> +            spin_unlock_irqrestore(&irq->irq_lock, flags);
> >> +        }
> > 
> > NIT: it should be safe to call vgic_queue_irq_unlock regardless, right?
> 
> I don't think vgic_queue_irq_unlock() and subsequent functions can deal
> with the IRQ being pending, but not having a source bit set:
> http://www.linux-arm.org/git?p=xen-ap.git;a=blob;f=xen/arch/arm/vgic/vgic-v2.c;h=6a84e741ee0a#l205
> 
> Besides, this scheme of:
>       if ( needs to be injected )
>       {
>               make_pending();
>               vgic_queue_irq_unlock();
>       }
>       else
>       {
>               spin_unlock_irqrestore();
>       }
> 
> is all over the place, and I don't want to deviate from that and the KVM
> implementation needlessly.

OK

Acked-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel