Re: [Xen-devel] [PATCH 29/57] ARM: new VGIC: Implement virtual IRQ injection

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

Overall this patch looks good. Few comments below.

On 03/05/2018 04:03 PM, Andre Przywara wrote:
> +/*
> + * Only valid injection if changing level for level-triggered IRQs or for a
> + * rising edge.
> + */
> +static bool vgic_validate_injection(struct vgic_irq *irq, bool level)
> +{
> +    if ( irq->config == VGIC_CONFIG_LEVEL )
> +        return irq->line_level != level;
> +
> +    return level;
TBH, I would have preferred to keep the switch here. It was much clearer 
the second case is for edge. I would be ok with the if providing comment 
explaining "return level" is for edge.
> +}
> +
> +/**
> + * vgic_queue_irq_unlock() - Queue an IRQ to a VCPU, to be injected to a guest.
> + * @d:        The domain the virtual IRQ belongs to.
> + * @irq:      A pointer to the vgic_irq of the virtual IRQ, with the lock held.
> + * @flags:    The flags used when having grabbed the IRQ lock.
> + *
> + * Check whether an IRQ needs to (and can) be queued to a VCPU's ap list.
> + * Do the queuing if necessary, taking the right locks in the right order.
> + *
> + * Needs to be entered with the IRQ lock already held, but will return
> + * with all locks dropped.
> + *
> + * Returns: True when the IRQ was queued, false otherwise.
The function is now returning void. It sounds like a left-over from the 
previous version?
> + */
> +void vgic_queue_irq_unlock(struct domain *d, struct vgic_irq *irq,
> +                           unsigned long flags)
> +{
[...]

> diff --git a/xen/arch/arm/vgic/vgic.h b/xen/arch/arm/vgic/vgic.h
> index a3befd386b..3430955d9f 100644
> --- a/xen/arch/arm/vgic/vgic.h
> +++ b/xen/arch/arm/vgic/vgic.h
> @@ -17,9 +17,19 @@
>   #ifndef __XEN_ARM_VGIC_VGIC_H__
>   #define __XEN_ARM_VGIC_VGIC_H__
>   
> +static inline bool irq_is_pending(struct vgic_irq *irq)
> +{
> +    if ( irq->config == VGIC_CONFIG_EDGE )
> +        return irq->pending_latch;
> +    else
> +        return irq->pending_latch || irq->line_level;
> +}
> +
>   struct vgic_irq *vgic_get_irq(struct domain *d, struct vcpu *vcpu,
>                                 u32 intid);
>   void vgic_put_irq(struct domain *d, struct vgic_irq *irq);
> +void vgic_queue_irq_unlock(struct domain *d, struct vgic_irq *irq,
> +               unsigned long flags);
The indentation looks wrong here.

>   
>   static inline void vgic_get_irq_kref(struct vgic_irq *irq)
>   {
Cheers,

--
Julien Grall

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