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

[Andre Przywara <andre.przywara@xxxxxxxxxx>]

Hi,

On 07/03/18 11:02, Julien Grall wrote:
> 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.

I see what you mean, and actually had it first this way, but:

vgic/vgic.c:250:14: error: switch condition has boolean value
[-Werror=switch-bool]
     switch ( irq->config )

I will add comments to document both cases.

>> +}
>> +
>> +/**
>> + * 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?

True, thanks for catching this.

>> + */
>> +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.

Hah, you found the one ;-)
It's a shame we don't have checkpatch, as those things are caught with
checkpatch --strict in Linux.

Cheers,
Andre.

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

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