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

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel