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

[Peter Maydell <peter.maydell@linaro.org>]

On 2 December 2016 at 05:46, Alastair D'Silva <alastair@au1.ibm.com> wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> 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 <alastair@d-silva.org>

> --- 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.

>   *
>   * 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.)

(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.


> 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.

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

Stray whitespace change.

>  #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.

> +    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 ':').

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