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
[-Werror=switch-bool]
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)
{
foo(true);
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.
Cheers,
Andre.
static inline void vgic_get_irq_kref(struct vgic_irq *irq)
{
Cheers,
[1] https://lkml.org/lkml/2015/5/27/941
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
