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

[Julien Grall <julien.grall@xxxxxxx>]

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.
> +    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?
> +    }
> +    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.
> +    }
> +
> +    spin_unlock_irqrestore(&irq->irq_lock, flags);
> +    vgic_put_irq(d, irq);
> +
> +    return ret;
> +}
> +
>   /*
>    * Local variables:
>    * mode: C
Cheers,

--
Julien Grall

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