Re: [PATCH v16 3/6] tests/qtest: Creating qtest for GMAC Module

[Peter Maydell]

On Wed, 31 Jan 2024 at 00:28, Nabih Estefan <nabihestefan@google.com> wrote:
>
> From: Nabih Estefan Diaz <nabihestefan@google.com>
>
>  - Created qtest to check initialization of registers in GMAC Module.
>  - Implemented test into Build File.
>
> Change-Id: I8b2fe152d3987a7eec4cf6a1d25ba92e75a5391d
> Signed-off-by: Nabih Estefan <nabihestefan@google.com>
> Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
> ---
>  tests/qtest/meson.build      |   1 +
>  tests/qtest/npcm_gmac-test.c | 212 +++++++++++++++++++++++++++++++++++
>  2 files changed, 213 insertions(+)
>  create mode 100644 tests/qtest/npcm_gmac-test.c

I've just noticed some issues with this patch, though it is already
upstream:

> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index 84a055a7d9..016cd77d20 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -230,6 +230,7 @@ qtests_aarch64 = \
>    (config_all_devices.has_key('CONFIG_RASPI') ? ['bcm2835-dma-test'] : []) + 
>  \
>    (config_all_accel.has_key('CONFIG_TCG') and                                
>             \
>     config_all_devices.has_key('CONFIG_TPM_TIS_I2C') ? ['tpm-tis-i2c-test'] : 
> []) + \
> +  (config_all_devices.has_key('CONFIG_NPCM7XX') ? qtests_npcm7xx : []) + \

This adds all the qtests_npcm7xx to qtests_aarch64. We deliberately
don't do this, because those tests are tested under qtests_arm,
and we don't want to use the CI minutes repeating them all again
with the qtest-system-aarch64 binary.

>    ['arm-cpu-features',
>     'numa-test',
>     'boot-serial-test',

The patch is missing anything that adds npcm_gmac-test to
qtests_npcm7xx, so in fact nothing ever builds or runs this test.

If you fix that, then you run into:

> diff --git a/tests/qtest/npcm_gmac-test.c b/tests/qtest/npcm_gmac-test.c
> new file mode 100644
> index 0000000000..72c68874df
> --- /dev/null

> +/* Check that GMAC registers are reset to default value */
> +static void test_init(gconstpointer test_data)
> +{
> +    const TestData *td = test_data;
> +    const GMACModule *mod = td->module;
> +    QTestState *qts = qtest_init("-machine npcm845-evb");

This machine type doesn't exist. How is this test supposed to work?

I tried:
--- a/tests/qtest/npcm_gmac-test.c
+++ b/tests/qtest/npcm_gmac-test.c
@@ -24,7 +24,7 @@
 #define TYPE_NPCM_GMAC "npcm-gmac"

 /* Address of the PCS Module */
-#define PCS_BASE_ADDRESS 0xf0780000
+#define PCS_BASE_ADDRESS 0xf0802000
 #define NPCM_PCS_IND_AC_BA 0x1fe

 typedef struct GMACModule {
@@ -196,7 +196,7 @@ static void test_init(gconstpointer test_data)
 {
     const TestData *td = test_data;
     const GMACModule *mod = td->module;
-    QTestState *qts = qtest_init("-machine npcm845-evb");
+    QTestState *qts = qtest_init("-machine quanta-gsj");

 #define CHECK_REG32(regno, value) \
     do { \

and then it passes some of the tests, but fails on

ERROR:../../tests/qtest/npcm_gmac-test.c:262:test_init: assertion
failed (pcs_read(qts, mod, (NPCM_PCS_SR_CTL_ID1)) == (0x699e)):
(0x00000000 == 0x0000699e)

And the gmac_module_list[] array in the test claims
"Values extracted from hw/arm/npcm8xx.c", which is a file
that doesn't exist.

Basically this is a mess. Please can you submit a patch
that fixes this test so that:
 * it actually runs
 * it passes
 * it's testing the device in the machine that's in upstream
   QEMU, not some other device in a machine that's presumably
   in your downstream repo

thanks
-- PMM