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

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@linaro.org

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);