Hi Andre,

On 03/07/2018 11:22 AM, Andre Przywara wrote:
On 07/03/18 11:02, Julien Grall wrote:
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
      switch ( irq->config )

What is your compiler version? I am using GCC 6.3 (x86 from Debian) and wasn't able to get the error message in this small snippet:

#include <stdio.h>
#include <stdint.h>
#include <stdbool.h>

int foo(bool f)
    switch (f)
    case true:
        return 1;
    case false:
        return 0;

int main(void)

    return 0;

42sh> gcc -Wall -Werror -Wswitch-bool -std=c99 test.c

This sounds a bit stupid to me to forbid switch boolean. It sometimes help when using define and more expressive. Sounds like, there was similar discussion on the kernel ML (see [1]).

I will add comments to document both cases.

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

AFAIK, someone is working on a checkpatch for Xen. Thought I haven't seen anything on xen-devel so far. Hopefully, it will appear soon.


     static inline void vgic_get_irq_kref(struct vgic_irq *irq)


[1] https://lkml.org/lkml/2015/5/27/941

Julien Grall

Xen-devel mailing list

Reply via email to