Re: [Qemu-arm] [PATCH v3 4/7] qtest: Support named interrupts

[Alastair D'Silva]

On Wed, 2016-12-14 at 18:29 +0000, Peter Maydell wrote:
> On 2 December 2016 at 05:46, Alastair D'Silva <address@hidden>
> wrote:
> > From: Alastair D'Silva <address@hidden>
> > 
> > The QTest framework cannot work with named interrupts. This patch
> > adds support for them, as well as the ability to manipulate them
> > from within a test.
> > 
> > Read actions are via callbacks, which allows for pulsed interrupts
> > to be read (the polled method used for the unnamed interrupts
> > cannot read pulsed interrupts as the value is reverted before the
> > test sees the changes).
> > 
> > Signed-off-by: Alastair D'Silva <address@hidden>
> > --- a/qtest.c
> > +++ b/qtest.c
> > @@ -40,7 +40,6 @@ static DeviceState *irq_intercept_dev;
> >  static FILE *qtest_log_fp;
> >  static CharBackend qtest_chr;
> >  static GString *inbuf;
> > -static int irq_levels[MAX_IRQ];
> >  static qemu_timeval start_time;
> >  static bool qtest_opened;
> > 
> > @@ -160,10 +159,16 @@ static bool qtest_opened;
> >   *
> >   *  IRQ raise NUM
> >   *  IRQ lower NUM
> > + *  IRQ_NAMED NAME NUM LEVEL
> 
> I think we should be consistent about the protocol here:
> unnamed IRQs get 'raise' and 'lower' messages, so we should
> do the same for named IRQs.
> 

Ok

> >   *
> >   * where NUM is an IRQ number.  For the PC, interrupts can be
> > intercepted
> >   * simply with "irq_intercept_in ioapic" (note that IRQ0 comes out
> > with
> >   * NUM=0 even though it is remapped to GSI 2).
> > + *
> > + *  > irq_set NAME NUM LEVEL
> > + *  < OK
> > + *
> > + *  Set the named input IRQ to the level (0/1)
> 
> I think adding support for raising and lowering device IRQs
> should be a separate patch, as it's a different feature.
> (I'm also not sure we should need it -- devices will be
> wired into the system, and qtest is testing the whole
> system from the point of view of the CPU. The CPU can't
> arbitrarily reach in and assert a device's outgoing
> interrupt line, so I'm not sure the tests should be able
> to do it either.)
> 

Unfortunately, from what I can see, the concepts of GPIO lines & IRQs
are a bit mixed up in Qemu. The use case I have is that an input line
to the chip (not an output) needs to be asserted during the test to
change it's behaviour.

> (Everything else below here is trivial fixes.)
> 
> >   */
> > 
> >  static int hex2nib(char ch)
> > @@ -243,17 +248,31 @@ static void GCC_FMT_ATTR(2, 3)
> > qtest_sendf(CharBackend *chr,
> >      va_end(ap);
> >  }
> > 
> > +typedef struct qtest_irq {
> > +    qemu_irq old_irq;
> > +    char *name;
> > +    bool last_level;
> > +} qtest_irq;
> > +
> >  static void qtest_irq_handler(void *opaque, int n, int level)
> >  {
> > -    qemu_irq old_irq = *(qemu_irq *)opaque;
> > -    qemu_set_irq(old_irq, level);
> > +    qtest_irq *data = (qtest_irq *)opaque;
> > +    level = !!level;
> > +
> > +    qemu_set_irq(data->old_irq, level);
> > 
> > -    if (irq_levels[n] != level) {
> > +    if (level != data->last_level) {
> >          CharBackend *chr = &qtest_chr;
> > -        irq_levels[n] = level;
> >          qtest_send_prefix(chr);
> > -        qtest_sendf(chr, "IRQ %s %d\n",
> > -                    level ? "raise" : "lower", n);
> > +
> > +        if (data->name) {
> > +            qtest_sendf(chr, "IRQ_NAMED %s %d %d\n",
> > +                    data->name, n, level);
> > +        } else {
> > +            qtest_sendf(chr, "IRQ %s %d\n", level ? "raise" :
> > "lower", n);
> > +        }
> > +
> > +        data->last_level = level;
> >      }
> >  }
> > 
> > @@ -289,7 +308,7 @@ static void qtest_process_command(CharBackend
> > *chr, gchar **words)
> >          if (!dev) {
> >              qtest_send_prefix(chr);
> >              qtest_send(chr, "FAIL Unknown device\n");
> > -           return;
> > +            return;
> >          }
> > 
> >          if (irq_intercept_dev) {
> > @@ -299,33 +318,73 @@ static void qtest_process_command(CharBackend
> > *chr, gchar **words)
> >              } else {
> >                  qtest_send(chr, "OK\n");
> >              }
> > -           return;
> > +            return;
> >          }
> 
> Fixing whitespace issues is generally best done in a separate patch.
> 

Ok

> 
> > diff --git a/tests/libqtest.c b/tests/libqtest.c
> > index 6f69752..36b3960 100644
> > --- a/tests/libqtest.c
> > +++ b/tests/libqtest.c
> > @@ -21,12 +21,16 @@
> >  #include <sys/wait.h>
> >  #include <sys/un.h>
> > 
> > +#include <glib.h>
> > +
> 
> osdep.h pulls in glib for you.
> 

Ok

> >  #include "qapi/qmp/json-parser.h"
> >  #include "qapi/qmp/json-streamer.h"
> >  #include "qapi/qmp/qjson.h"
> > 
> > +
> 
> Stray whitespace change.

Ok

> 
> >  #define MAX_IRQ 256
> >  #define SOCKET_TIMEOUT 50
> > +#define IRQ_KEY_LENGTH 64
> > 
> >  QTestState *global_qtest;
> 
> 
> > @@ -346,6 +408,17 @@ redo:
> >          g_strfreev(words);
> >      }
> > 
> > +/* Defer processing of IRQ actions until all communications have
> > been handled,
> > + * otherwise, interrupt handler that cause further communication
> > can disrupt
> > + * the communication stream
> > + */
> 
> Bad indent on this comment.

Ok

> 
> > +    for (action_index = 0; action_index < action_count;
> > action_index++) {
> > +        irq_action *action = actions[action_index];
> > +        action->cb(action->opaque, action->name, action->n,
> > +                action_raise[action_index]);
> > +        action->level = action_raise[action_index];
> > +    }
> > +
> >      return words;
> >  }
> > 
> >  /**
> > + * irq_attach:
> > + * @name: the name of the gpio list containing the IRQ
> > + * @irq: The IRQ to attach to
> > + * @irq_cb: The callback to execute when the interrupt changes
> > + * @opaque: opaque info to pass to the callback
> > + *
> > + * Attach a callback to an intecepted interrupt
> > + */
> > +static inline void irq_attach(const char *name, int irq,
> > +        void (*irq_cb)(void *opaque, const char *name, int irq,
> > bool level),
> > +        void *opaque)
> > +{
> > +    qtest_irq_attach(global_qtest, name, irq, irq_cb, opaque);
> > +}
> > +
> > +/**
> > + * qtest_irq_set
> 
> Comment doesn't match function name (and missing ':').
> 

Ok

> > + * Set an interrupt level
> > + * @id: the device to inject interrupts for
> > + * @gpiolist: the GPIO list containing the line to seh
> > + * @n: the line to set within the list
> > + * @level: the IRQ level
> > + */
> > +static inline void irq_set(const char *id, const char *gpiolist,
> > int n,
> > +        bool level)
> > +{
> > +    qtest_irq_set(global_qtest, id, gpiolist, n, level);
> > +}
> > +
> > +
> > +/**
> >   * outb:
> >   * @addr: I/O port to write to.
> >   * @value: Value being written.
> > --
> > 2.9.3
> 
> thanks
> -- PMM
> 

-- 
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819