Re: [Xen-devel] [RFC PATCH 38/49] ARM: new VGIC: handle hardware mapped IRQs

[Andre Przywara <andre.przywara@xxxxxxxxxx>]

Hi,

On 19/02/18 12:19, Julien Grall wrote:
> Hi,
> 
> On 09/02/18 14:39, Andre Przywara wrote:
>> The VGIC supports virtual IRQs to be connected to a hardware IRQ, so
>> when a guest EOIs the virtual interrupt, it affects the state of that
>> corresponding interrupt on the hardware side at the same time.
>> Implement the interface that the Xen arch/core code expects to connect
>> the virtual and the physical world.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
>> ---
>>   xen/arch/arm/vgic/vgic.c | 63
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 63 insertions(+)
>>
>> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
>> index dc5e011fa3..8d5260a7db 100644
>> --- a/xen/arch/arm/vgic/vgic.c
>> +++ b/xen/arch/arm/vgic/vgic.c
>> @@ -693,6 +693,69 @@ void vgic_kick_vcpus(struct domain *d)
>>       }
>>   }
>>   +struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, struct vcpu
>> *v,
>> +                                      unsigned int virq)
>> +{
>> +    struct irq_desc *desc = NULL;
>> +    struct vgic_irq *irq = vgic_get_irq(d, v, virq);
>> +    unsigned long flags;
>> +
>> +    if ( !irq )
>> +        return NULL;
>> +
>> +    spin_lock_irqsave(&irq->irq_lock, flags);
>> +    if ( irq->hw )
>> +        desc = irq_to_desc(irq->hwintid);
> 
> This is not going to work well for PPIs. We should consider to add at
> least an ASSERT(...) in the code to prevent bad use of it.

Yeah, done. But I wonder if we eventually should extend the
irq_to_desc() function to take the vCPU, since we will need it anyway
once we use hardware mapped timer IRQs (PPIs) in the future. But this
should not be in this series, I guess.

>> +    spin_unlock_irqrestore(&irq->irq_lock, flags);
>> +
>> +    vgic_put_irq(d, irq);
>> +
>> +    return desc;
>> +}
>> +
>> +/*
>> + * was:
>> + *      int kvm_vgic_map_phys_irq(struct vcpu *vcpu, u32 virt_irq,
>> u32 phys_irq)
>> + *      int kvm_vgic_unmap_phys_irq(struct vcpu *vcpu, unsigned int
>> virt_irq)
>> + */
>> +int vgic_connect_hw_irq(struct domain *d, struct vcpu *vcpu,
>> +            unsigned int virt_irq, struct irq_desc *desc,
>> +            bool connect)
> 
> Indentation.
> 
>> +{
>> +    struct vgic_irq *irq = vgic_get_irq(d, vcpu, virt_irq);
>> +    unsigned long flags;
>> +    int ret = 0;
>> +
>> +    if ( !irq )
>> +        return -EINVAL;
>> +
>> +    spin_lock_irqsave(&irq->irq_lock, flags);
>> +
>> +    if ( connect )                      /* assign a mapped IRQ */
>> +    {
>> +        /* The VIRQ should not be already enabled by the guest */
>> +        if ( !irq->hw && !irq->enabled )
>> +        {
>> +            irq->hw = true;
>> +            irq->hwintid = desc->irq;
>> +        }
>> +        else
>> +        {
>> +            ret = -EBUSY;
>> +        }
> 
> I know that it should not matter for SPIs today. But aren't you meant to
> get a reference on that interrupt if you connect it?

No, the refcount feature is strictly for the pointer to the structure,
not for everything related to this virtual IRQ.
We store only the virtual IRQ number in the dev_id/info members, we will
get the struct vgic_irq pointer via the vIRQ number on do_IRQ().
Does that make sense?

>> +    }
>> +    else                                /* remove a mapped IRQ */
>> +    {
>> +        irq->hw = false;
>> +        irq->hwintid = 0;
> 
> Here you blindly remove the interrupt without been sure it was the
> correct physical one. We should have a check like in the current vGIC
> version.

Fixed.

Cheers,
Andre.

>> +    }
>> +
>> +    spin_unlock_irqrestore(&irq->irq_lock, flags);
>> +    vgic_put_irq(d, irq);
>> +
>> +    return ret;
>> +}
>> +
>>   /*
>>    * Local variables:
>>    * mode: C
>>
> 
> Cheers,
> 

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