Re: [PATCH V6 2/3] xl: Add support to parse generic virtio device

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

On 08.11.22 13:23, Viresh Kumar wrote:


Hello Viresh

[sorry for the possible format issues if any]

> This patch adds basic support for parsing generic Virtio backend.
>
> An example of domain configuration for mmio based Virtio I2C device is:
> virtio = ["type=virtio,device22,transport=mmio"]
>
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---
>   tools/ocaml/libs/xl/genwrap.py       |  1 +
>   tools/ocaml/libs/xl/xenlight_stubs.c |  1 +
>   tools/xl/xl_parse.c                  | 84 ++++++++++++++++++++++++++++
>   3 files changed, 86 insertions(+)
>
> diff --git a/tools/ocaml/libs/xl/genwrap.py b/tools/ocaml/libs/xl/genwrap.py
> index 7bf26bdcd831..b188104299b1 100644
> --- a/tools/ocaml/libs/xl/genwrap.py
> +++ b/tools/ocaml/libs/xl/genwrap.py
> @@ -36,6 +36,7 @@ DEVICE_LIST =      [ ("list",           ["ctx", "domid", "t 
> list"]),
>   functions = { # ( name , [type1,type2,....] )
>       "device_vfb":     DEVICE_FUNCTIONS,
>       "device_vkb":     DEVICE_FUNCTIONS,
> +    "device_virtio":     DEVICE_FUNCTIONS,
>       "device_disk":    DEVICE_FUNCTIONS + DEVICE_LIST +
>                         [ ("insert",         ["ctx", "t", "domid", "?async:'a", "unit", 
> "unit"]),
>                           ("of_vdev",        ["ctx", "domid", "string", "t"]),
> diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c 
> b/tools/ocaml/libs/xl/xenlight_stubs.c
> index 45b8af61c74a..8e54f95da7c7 100644
> --- a/tools/ocaml/libs/xl/xenlight_stubs.c
> +++ b/tools/ocaml/libs/xl/xenlight_stubs.c
> @@ -707,6 +707,7 @@ DEVICE_ADDREMOVE(disk)
>   DEVICE_ADDREMOVE(nic)
>   DEVICE_ADDREMOVE(vfb)
>   DEVICE_ADDREMOVE(vkb)
> +DEVICE_ADDREMOVE(virtio)
>   DEVICE_ADDREMOVE(pci)
>   _DEVICE_ADDREMOVE(disk, cdrom, insert)
>   
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 1b5381cef033..c6f35c069d2a 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -1208,6 +1208,87 @@ static void parse_vkb_list(const XLU_Config *config,
>       if (rc) exit(EXIT_FAILURE);
>   }
>   
> +static int parse_virtio_config(libxl_device_virtio *virtio, char *token)
> +{
> +    char *oparg;
> +    int rc;
> +
> +    if (MATCH_OPTION("backend", token, oparg)) {
> +        virtio->backend_domname = strdup(oparg);
> +    } else if (MATCH_OPTION("type", token, oparg)) {
> +        virtio->type = strdup(oparg);
> +    } else if (MATCH_OPTION("transport", token, oparg)) {
> +        rc = libxl_virtio_transport_from_string(oparg, &virtio->transport);
> +        if (rc) return rc;
> +    } else if (MATCH_OPTION("irq", token, oparg)) {
> +        virtio->irq = strtoul(oparg, NULL, 0);
> +    } else if (MATCH_OPTION("base", token, oparg)) {
> +        virtio->base = strtoul(oparg, NULL, 0);

Interesting, I see you allow user to configure virtio-mmio params (irq 
and base), as far as I remember for virtio-disk these are internal only 
(allocated by tools/libs/light/libxl_arm.c).
I am not really sure why we need to configure virtio "base", could you 
please clarify? But if we really want/need to be able to configure 
virtio "irq" (for example to avoid possible clashing with physical one), 
I am afraid, this will require more changes that current patch does. 
Within current series saving virtio->irq here doesn't have any effect as 
it will be overwritten in 
libxl__arch_domain_prepare_config()->alloc_virtio_mmio_params() anyway. 
I presume the code in libxl__arch_domain_prepare_config() shouldn't try 
to allocate virtio->irq if it is already configured by user, also the 
allocator should probably take into the account of what is already 
configured by user, to avoid allocating the same irq for another device 
assigned for the same guest.
Also doc change in the subsequent patch doesn't mention about irq/base 
configuration.

So maybe we should just drop for now?
+    } else if (MATCH_OPTION("irq", token, oparg)) {
+        virtio->irq = strtoul(oparg, NULL, 0);
+    } else if (MATCH_OPTION("base", token, oparg)) {
+        virtio->base = strtoul(oparg, NULL, 0);



> +    } else {
> +        fprintf(stderr, "Unknown string \"%s\" in virtio spec\n", token);
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +static void parse_virtio_list(const XLU_Config *config,
> +                              libxl_domain_config *d_config)
> +{
> +    XLU_ConfigList *virtios;
> +    const char *item;
> +    char *buf = NULL, *oparg, *str = NULL;
> +    int rc;
> +
> +    if (!xlu_cfg_get_list (config, "virtio", &virtios, 0, 0)) {
> +        int entry = 0;
> +        while ((item = xlu_cfg_get_listitem(virtios, entry)) != NULL) {
> +            libxl_device_virtio *virtio;
> +            char *p;
> +
> +            virtio = ARRAY_EXTEND_INIT(d_config->virtios, 
> d_config->num_virtios,
> +                                       libxl_device_virtio_init);
> +
> +            buf = strdup(item);
> +
> +            p = strtok(buf, ",");
> +            while (p != NULL)
> +            {
> +                while (*p == ' ') p++;
> +
> +                // Type may contain a comma, do special handling.
> +                if (MATCH_OPTION("type", p, oparg)) {
> +                    if (!strncmp(oparg, "virtio", strlen("virtio"))) {
> +                        char *p2 = strtok(NULL, ",");
> +                        str = malloc(strlen(p) + strlen(p2) + 2);
> +
> +                        strcpy(str, p);
> +                        strcat(str, ",");
> +                        strcat(str, p2);
> +                        p = str;
> +                    }
> +                }
> +
> +                rc = parse_virtio_config(virtio, p);
> +                if (rc) goto out;
> +
> +                free(str);
> +                str = NULL;
> +                p = strtok(NULL, ",");
> +            }
> +
> +            entry++;
> +            free(buf);
> +        }
> +    }
> +
> +    return;
> +
> +out:
> +    free(buf);
> +    if (rc) exit(EXIT_FAILURE);
> +}
> +
>   void parse_config_data(const char *config_source,
>                          const char *config_data,
>                          int config_len,
> @@ -2309,8 +2390,10 @@ void parse_config_data(const char *config_source,
>   
>       d_config->num_vfbs = 0;
>       d_config->num_vkbs = 0;
> +    d_config->num_virtios = 0;
>       d_config->vfbs = NULL;
>       d_config->vkbs = NULL;
> +    d_config->virtios = NULL;
>   
>       if (!xlu_cfg_get_list (config, "vfb", &cvfbs, 0, 0)) {
>           while ((buf = xlu_cfg_get_listitem (cvfbs, d_config->num_vfbs)) != 
> NULL) {
> @@ -2752,6 +2835,7 @@ void parse_config_data(const char *config_source,
>       }
>   
>       parse_vkb_list(config, d_config);
> +    parse_virtio_list(config, d_config);
>   
>       xlu_cfg_get_defbool(config, "xend_suspend_evtchn_compat",
>                           &c_info->xend_suspend_evtchn_compat, 0);