Re: [PATCH v4 28/47] hw/arm/npcm7xx: use qemu_configure_nic_device, allow emc0/emc1 as aliases

[Thomas Huth <thuth@redhat.com>]

On 26/01/2024 18.25, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> Also update the test to specify which device to attach the test socket
> to, and remove the comment lamenting the fact that we can't do so.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   hw/arm/npcm7xx.c               | 16 +++++++++-------
>   tests/qtest/npcm7xx_emc-test.c | 18 ++++--------------
>   2 files changed, 13 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
> index 15ff21d047..ee395864e4 100644
> --- a/hw/arm/npcm7xx.c
> +++ b/hw/arm/npcm7xx.c
> @@ -655,8 +655,9 @@ static void npcm7xx_realize(DeviceState *dev, Error **errp)
>   
>       /*
>        * EMC Modules. Cannot fail.
> -     * The mapping of the device to its netdev backend works as follows:
> -     * emc[i] = nd_table[i]
> +     * Use the available NIC configurations in order, allowing 'emc0' and
> +     * 'emc1' to by used as aliases for the model= parameter to override.
> +     *
>        * This works around the inability to specify the netdev property for the
>        * emc device: it's not pluggable and thus the -device option can't be
>        * used.
> @@ -664,12 +665,13 @@ static void npcm7xx_realize(DeviceState *dev, Error **errp)
>       QEMU_BUILD_BUG_ON(ARRAY_SIZE(npcm7xx_emc_addr) != ARRAY_SIZE(s->emc));
>       QEMU_BUILD_BUG_ON(ARRAY_SIZE(s->emc) != 2);
>       for (i = 0; i < ARRAY_SIZE(s->emc); i++) {
> -        s->emc[i].emc_num = i;
>           SysBusDevice *sbd = SYS_BUS_DEVICE(&s->emc[i]);
> -        if (nd_table[i].used) {
> -            qemu_check_nic_model(&nd_table[i], TYPE_NPCM7XX_EMC);
> -            qdev_set_nic_properties(DEVICE(sbd), &nd_table[i]);
> -        }
> +        char alias[6];
> +
> +        s->emc[i].emc_num = i;
> +        snprintf(alias, sizeof(alias), "emc%u", i);
> +        qemu_configure_nic_device(DEVICE(sbd), true, alias);
> +
>           /*
>            * The device exists regardless of whether it's connected to a QEMU
>            * netdev backend. So always instantiate it even if there is no
> diff --git a/tests/qtest/npcm7xx_emc-test.c b/tests/qtest/npcm7xx_emc-test.c
> index b046f1d76a..f7646fae2c 100644
> --- a/tests/qtest/npcm7xx_emc-test.c
> +++ b/tests/qtest/npcm7xx_emc-test.c
> @@ -225,21 +225,11 @@ static int *packet_test_init(int module_num, GString *cmd_line)
>       g_assert_cmpint(ret, != , -1);
>   
>       /*
> -     * KISS and use -nic. We specify two nics (both emc{0,1}) because there's
> -     * currently no way to specify only emc1: The driver implicitly relies on
> -     * emc[i] == nd_table[i].
> +     * KISS and use -nic. The driver accepts 'emc0' and 'emc1' as aliases
> +     * in the 'model' field to specify the device to match.
>        */
> -    if (module_num == 0) {
> -        g_string_append_printf(cmd_line,
> -                               " -nic socket,fd=%d,model=" TYPE_NPCM7XX_EMC " "
> -                               " -nic user,model=" TYPE_NPCM7XX_EMC " ",
> -                               test_sockets[1]);
> -    } else {
> -        g_string_append_printf(cmd_line,
> -                               " -nic user,model=" TYPE_NPCM7XX_EMC " "
> -                               " -nic socket,fd=%d,model=" TYPE_NPCM7XX_EMC " ",
> -                               test_sockets[1]);
> -    }
> +    g_string_append_printf(cmd_line, " -nic socket,fd=%d,model=emc%d ",
> +                           test_sockets[1], module_num);
>   
>       g_test_queue_destroy(packet_test_clear, test_sockets);
>       return test_sockets;

I like the idea to use the alias to configure a certain on-board NIC :-)

Reviewed-by: Thomas Huth <thuth@redhat.com>