Re: [PATCH v2 3/3] tests/qtest: Add test for Aspeed HACE

[Joel Stanley]

On Fri, 12 Mar 2021 at 05:53, Thomas Huth <thuth@redhat.com> wrote:
>
> On 12/03/2021 00.47, Joel Stanley wrote:
> > This adds a test for the Aspeed Hash and Crypto (HACE) engine. It tests
> > the currently implemented behavior of the hash functionality.
> >
> > The tests are similar, but are cut/pasted instead of broken out into a
> > common function so the assert machinery produces useful output when a
> > test fails.
> >
> > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > ---
> >   tests/qtest/aspeed_hace-test.c | 215 +++++++++++++++++++++++++++++++++
> >   MAINTAINERS                    |   1 +
> >   tests/qtest/meson.build        |   3 +
> >   3 files changed, 219 insertions(+)
> >   create mode 100644 tests/qtest/aspeed_hace-test.c
> >
> > diff --git a/tests/qtest/aspeed_hace-test.c b/tests/qtest/aspeed_hace-test.c
> > new file mode 100644
> > index 000000000000..52501ee37afb
> > --- /dev/null
> > +++ b/tests/qtest/aspeed_hace-test.c
> > @@ -0,0 +1,215 @@
> > +/*
> > + * QTest testcase for the ASPEED Hash and Crypto Engine
> > + *
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + * Copyright 2021 IBM Corp.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +
> > +#include "libqtest-single.h"
>
> Bonus points for writing the test without "libqtest-single.h" and using only
> "libqtest.h" instead. We should try to avoid libqtest-single.h in new code
> since such code can not be shared with tests that run multiple instances of
> QEMU. But if it's too cumbersome, I do not insist.

This wasn't too hard, so I went ahead and did it. I'll send a new version.

>
> > diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> > index 2688e1bfad7f..2eec06b10094 100644
> > --- a/tests/qtest/meson.build
> > +++ b/tests/qtest/meson.build
> > @@ -156,12 +156,15 @@ qtests_npcm7xx = \
> >      'npcm7xx_timer-test',
> >      'npcm7xx_watchdog_timer-test'] + \
> >      (slirp.found() ? ['npcm7xx_emc-test'] : [])
> > +qtests_aspeed = \
> > +  ['aspeed_hace-test']
>
> Do you plan to add more aspeed tests later? If not, I think you could simply
> do it without this variable and use ['aspeed_hace-test'] below.

I intend on sending more, but they are not ready yet. I will leave it
like this in readiness for those future tests if that's okay.

Thanks for the review.

Cheers,

Joel