Re: [Qemu-devel] [PATCH 2/4] qtest: Support named interrupts

[Paolo Bonzini]

- Original Message -
> From: "Alastair D'Silva" 
> To: "Paolo Bonzini" , "Cédric Le Goater" 
> , qemu-...@nongnu.org
> Cc: qemu-devel@nongnu.org, "Peter Maydell" , 
> "Andrew Jeffery" , "Joel
> Stanley" 
> Sent: Wednesday, November 23, 2016 12:19:50 AM
> Subject: Re: [PATCH 2/4] qtest: Support named interrupts
> 
> On Tue, 2016-11-22 at 23:39 +0100, Paolo Bonzini wrote:
> > On 22/11/2016 23:31, Alastair D'Silva wrote:
> > > > 
> > > > This seems wrong.  The IRQ should not be modifiable by the
> > > > test.
> > > 
> > > Thanks Paolo, could you please advise as to why that is?
> 
> Could you answer this please? I would like to understand why.

Well, I didn't know yet until I knew what the GPIO line was for. :)

But in general, the idea is that the qtest acts as the CPU.  The test
cases can control the passing of time precisely, and they can _observe_
additional events (such as interrupts or GPIO lines) but they don't inject
anything that the CPU cannot inject.  The reason is to make the tests
more like small programs.  It does mean that you are limited by the
connections of the board.

> > > The situation I am addressing is that I device under test that
> > > changes
> > > behaviour when a GPIO line is raised. Is there another way I should
> > > be
> > > raising that line from within qtest?
> > 
> > What causes the GPIO line to be raised in the normal emulated case?
>
> It would be wired to a GPIO line from the host microcontroller, under
> software control.

Note I said the normal emulated case, not the real hardware case.
Is there a GPIO controller that is not yet part of the ASpeed models?

If the emulated ASpeed board cannot yet work with FOut, I would just
leave it out of the testcases.

Paolo

> In this test case, the device is connected to a "borrowed" board via
> the command line:
>     snprintf(args, sizeof(args), "-display none -machine imx25-pdk "
> "-device rx8900,bus=i2c.0,address=0x%x,id=%s",
> RX8900_ADDR, RX8900_TEST_ID);
> 
> I couldn't see a way to wire in the the GPIO to the host via the
> command line, but even if there was, manipulating it would require
> manipulating the host CPU, which would broaden the scope of the test.

> At the moment, the test has no dependency on/interaction with the host
> CPU, it's just using it to provide an I2C bus.

Re: [Qemu-devel] [PATCH 2/4] qtest: Support named interrupts

[Alastair D'Silva]

On Tue, 2016-11-22 at 23:39 +0100, Paolo Bonzini wrote:

> On 22/11/2016 23:31, Alastair D'Silva wrote:
> > 
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > > 
> > > > > > > > +if (irq == NULL) {
> > > > > > > > +qtest_send_prefix(chr);
> > > > > > > > +qtest_send(chr, "FAIL Unknown IRQ\n");
> > > > > > > > +return;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +qemu_set_irq(irq, level);
> > > > 
> > > > This seems wrong.  The IRQ should not be modifiable by the
> > > > test.
> > > > 
> > > > Paolo
> > > > 
> > Thanks Paolo, could you please advise as to why that is?

Could you answer this please? I would like to understand why.

> > The situation I am addressing is that I device under test that
> > changes
> > behaviour when a GPIO line is raised. Is there another way I should
> > be
> > raising that line from within qtest?
> 
> What causes the GPIO line to be raised in the normal emulated case?
> 
> 

It would be wired to a GPIO line from the host microcontroller, under
software control.

In this test case, the device is connected to a "borrowed" board via
the command line:
    snprintf(args, sizeof(args), "-display none -machine imx25-pdk "
"-device rx8900,bus=i2c.0,address=0x%x,id=%s",
RX8900_ADDR, RX8900_TEST_ID);

I couldn't see a way to wire in the the GPIO to the host via the
command line, but even if there was, manipulating it would require
manipulating the host CPU, which would broaden the scope of the test.
At the moment, the test has no dependency on/interaction with the host
CPU, it's just using it to provide an I2C bus.


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

Re: [Qemu-devel] [PATCH 2/4] qtest: Support named interrupts

[Paolo Bonzini]

On 22/11/2016 23:31, Alastair D'Silva wrote:
 > > > +if (irq == NULL) {
 > > > +qtest_send_prefix(chr);
 > > > +qtest_send(chr, "FAIL Unknown IRQ\n");
 > > > +return;
 > > > +}
 > > > +
 > > > +qemu_set_irq(irq, level);
>> > 
>> > This seems wrong.  The IRQ should not be modifiable by the test.
>> > 
>> > Paolo
>> > 
> Thanks Paolo, could you please advise as to why that is?
> 
> The situation I am addressing is that I device under test that changes
> behaviour when a GPIO line is raised. Is there another way I should be
> raising that line from within qtest?

What causes the GPIO line to be raised in the normal emulated case?

Paolo

Re: [Qemu-devel] [PATCH 2/4] qtest: Support named interrupts

[Alastair D'Silva]

On Tue, 2016-11-22 at 18:24 +0100, Paolo Bonzini wrote:

> 
> On 22/11/2016 18:22, Cédric Le Goater wrote:
> > 
> > > 
> > > +
> > > +g_assert(words[1]); /* device */
> > > +g_assert(words[2]); /* gpio list */
> > > +g_assert(words[3]); /* gpio line in list */
> > > +g_assert(words[4]); /* level */
> > > +dev = DEVICE(object_resolve_path(words[1], NULL));
> > > +if (!dev) {
> > > +qtest_send_prefix(chr);
> > > +qtest_send(chr, "FAIL Unknown device\n");
> > > +return;
> > > +}
> > > +
> > > +irq_num = atoi(words[3]);
> > > +level = atoi(words[4]);
> > > +
> > > +QLIST_FOREACH(ngl, >gpios, node) {
> > > +if (strcmp(words[2], ngl->name) == 0 && ngl->num_in
> > > > irq_num) {
> > > +irq = ngl->in[irq_num];
> > > +}
> > > +}
> > > +
> > > +if (irq == NULL) {
> > > +qtest_send_prefix(chr);
> > > +qtest_send(chr, "FAIL Unknown IRQ\n");
> > > +return;
> > > +}
> > > +
> > > +qemu_set_irq(irq, level);
> 
> This seems wrong.  The IRQ should not be modifiable by the test.
> 
> Paolo
> 

Thanks Paolo, could you please advise as to why that is?

The situation I am addressing is that I device under test that changes
behaviour when a GPIO line is raised. Is there another way I should be
raising that line from within qtest?

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

Re: [Qemu-devel] [PATCH 2/4] qtest: Support named interrupts

[Cédric Le Goater]

On 11/17/2016 05:36 AM, Alastair D'Silva wrote:
> From: Alastair D'Silva 
> 
> 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 

This looks OK to me but I am clearly not an expert in this area.
Maybe Paolo has some comments to add.

Reviewed-by: Cédric Le Goater 

Thanks,

C. 

> ---
>  qtest.c  | 98 
> ++--
>  tests/libqtest.c | 87 ++---
>  tests/libqtest.h | 59 ++
>  3 files changed, 216 insertions(+), 28 deletions(-)
> 
> diff --git a/qtest.c b/qtest.c
> index 46b99ae..a56503b 100644
> --- 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
>   *
>   * 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)
>   */
>  
>  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 = _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,69 @@ static void qtest_process_command(CharBackend *chr, 
> gchar **words)
>  } else {
>  qtest_send(chr, "OK\n");
>  }
> - return;
> +return;
>  }
>  
>  QLIST_FOREACH(ngl, >gpios, node) {
> -/* We don't support intercept of named GPIOs yet */
> -if (ngl->name) {
> -continue;
> -}
>  if (words[0][14] == 'o') {
>  int i;
>  for (i = 0; i < ngl->num_out; ++i) {
> -qemu_irq *disconnected = g_new0(qemu_irq, 1);
> -qemu_irq icpt = qemu_allocate_irq(qtest_irq_handler,
> -  disconnected, i);
> +qtest_irq *data = g_new0(qtest_irq, 1);
> +data->name = ngl->name;
> +qemu_irq icpt = qemu_allocate_irq(qtest_irq_handler, 
> data,
> +i);
>  
> -*disconnected = qdev_intercept_gpio_out(dev, icpt,
> +data->old_irq = qdev_intercept_gpio_out(dev, icpt,
>  ngl->name, i);
>  }
>  } else {
> -qemu_irq_intercept_in(ngl->in, qtest_irq_handler,
> -  ngl->num_in);
> +qtest_irq *data = g_new0(qtest_irq, 1);
> +data->old_irq = *ngl->in;
> +data->name = ngl->name;
> +qemu_irq_intercept_in(ngl->in, qtest_irq_handler, 
> ngl->num_in);
>  }
>  }
>  irq_intercept_dev = dev;
>  qtest_send_prefix(chr);
>  qtest_send(chr, 

Re: [Qemu-devel] [PATCH 2/4] qtest: Support named interrupts

[Paolo Bonzini]

On 22/11/2016 18:22, Cédric Le Goater wrote:
>> +
>> +g_assert(words[1]); /* device */
>> +g_assert(words[2]); /* gpio list */
>> +g_assert(words[3]); /* gpio line in list */
>> +g_assert(words[4]); /* level */
>> +dev = DEVICE(object_resolve_path(words[1], NULL));
>> +if (!dev) {
>> +qtest_send_prefix(chr);
>> +qtest_send(chr, "FAIL Unknown device\n");
>> +return;
>> +}
>> +
>> +irq_num = atoi(words[3]);
>> +level = atoi(words[4]);
>> +
>> +QLIST_FOREACH(ngl, >gpios, node) {
>> +if (strcmp(words[2], ngl->name) == 0 && ngl->num_in > irq_num) {
>> +irq = ngl->in[irq_num];
>> +}
>> +}
>> +
>> +if (irq == NULL) {
>> +qtest_send_prefix(chr);
>> +qtest_send(chr, "FAIL Unknown IRQ\n");
>> +return;
>> +}
>> +
>> +qemu_set_irq(irq, level);

This seems wrong.  The IRQ should not be modifiable by the test.

Paolo

>> +
>> +qtest_send_prefix(chr);
>> +qtest_send(chr, "OK\n");